Path.Local: backslashes are not canonical on Windows by nojb · Pull Request #4745 · ocaml/dune

nojb · 2021-06-16T20:37:28Z

Currently, Path.Local_gen.parse_string_exn has a fast path for when the argument is already "canonical", see

Lines 341 to 347 in 26f5e57

    
           let parse_string_exn ~loc s = 
        
             match s with 
        
             | "" 
        
             | "." -> 
        
               root 
        
             | _ when is_canonicalized s -> make s 
        
             | _ -> relative root s ~error_loc:loc

The function is_canonicalized checks a number of conditions such as whether there are two slashes in a row, etc. However, I think on Windows it should also check for the presence of backslashes since these are not canonical (a value of type Path.Local.t is represented with forward slashes only).

This PR should fix #4717 which is blocking the use of merlin on Windows when using (include_subdirs unqualified). It would be fantastic if we could get this in 2.9.

Fixes #4717

voodoos

Ok so the idea is that if there is a backslash in the path the test will fail and we will call relative that rebuilds a canonicalized path (with slashes).

And it is important that slashes are used in the path because other Dune Path functions relies on it, like Path.parent(as mentionned in #4717).

That sounds good to me, but I am no Windows expert.
Maybe @emillon will be interested in also looking at this ?

otherlibs/stdune-unstable/path.ml

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb · 2021-06-18T04:59:33Z

I'm planning to merge this today, please speak up if you object!

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

@emillon

…ator, dune-private-libs, dune and dune-build-info (2.9-rc1) CHANGES: - Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon) - Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb) - Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos) - Allow to set up merlin in a variant of the default context (ocaml/dune#4145, @TheLortex, @voodoos) - Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon) - Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01) - Fix generation of merlin configuration when using `(include_subdirs unqualified)` on Windows (ocaml/dune#4745, @nojb) - Fix bug for the install of Coq native files when using `(include_subdirs qualified)` (ocaml/dune#4753, @ejgallego) - Allow users to specify install target directories for `doc` and `etc` sections. We add new options `--docdir` and `--etcdir` to both Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723, @ejgallego, thanks to @JasonGross for reporting this issue) - Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01) - Fix issue where Dune would ignore `(env ... (coq (flags ...)))` declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg) - Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego)

@emillon

…ator, dune-private-libs, dune and dune-build-info (2.9.0) CHANGES: - Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon) - Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb) - Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos) - Allow to set up merlin in a variant of the default context (ocaml/dune#4145, @TheLortex, @voodoos) - Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon) - Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01) - Fix generation of merlin configuration when using `(include_subdirs unqualified)` on Windows (ocaml/dune#4745, @nojb) - Fix bug for the install of Coq native files when using `(include_subdirs qualified)` (ocaml/dune#4753, @ejgallego) - Allow users to specify install target directories for `doc` and `etc` sections. We add new options `--docdir` and `--etcdir` to both Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723, @ejgallego, thanks to @JasonGross for reporting this issue) - Fix issue where Dune would ignore `(env ... (coq (flags ...)))` declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg) - Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego) - Fix a bug where instrumentation flags would be added even if the instrumentatation was disabled (@nojb, ocaml/dune#4770) - Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632) - Fix installation with opam of package with dune sites. The `.install` file is now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645, @bobot, reported by @kit-ty-kate ocaml/dune#4198) - Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)

@emillon

…ator, dune-private-libs, dune and dune-build-info (2.9.0) CHANGES: - Add `(enabled_if ...)` to `(mdx ...)` (ocaml/dune#4434, @emillon) - Add support for instrumentation dependencies (ocaml/dune#4210, fixes ocaml/dune#3983, @nojb) - Add the possibility to use `locks` with the cram tests stanza (ocaml/dune#4480, @voodoos) - Allow to set up merlin in a variant of the default context (ocaml/dune#4145, @TheLortex, @voodoos) - Add `(package ...)` to `(mdx ...)` (ocaml/dune#4691, fixes ocaml/dune#3756, @emillon) - Handle renaming of `coq.kernel` library to `coq-core.kernel` in Coq 8.14 (ocaml/dune#4713, @proux01) - Fix generation of merlin configuration when using `(include_subdirs unqualified)` on Windows (ocaml/dune#4745, @nojb) - Fix bug for the install of Coq native files when using `(include_subdirs qualified)` (ocaml/dune#4753, @ejgallego) - Allow users to specify install target directories for `doc` and `etc` sections. We add new options `--docdir` and `--etcdir` to both Dune's configure and `dune install` command. (ocaml/dune#4744, fixes ocaml/dune#4723, @ejgallego, thanks to @JasonGross for reporting this issue) - Fix issue where Dune would ignore `(env ... (coq (flags ...)))` declarations appearing in `dune` files (ocaml/dune#4749, fixes ocaml/dune#4566, @ejgallego @rgrinberg) - Disable some warnings on Coq 8.14 and `(lang coq (>= 0.3))` due to the rework of the Coq "native" compilation system (ocaml/dune#4760, @ejgallego) - Fix a bug where instrumentation flags would be added even if the instrumentatation was disabled (@nojb, ocaml/dune#4770) - Fix ocaml/dune#4682: option `-p` takes now precedence on environement variable `DUNE_PROFILE` (ocaml/dune#4730, ocaml/dune#4774, @bobot, reported by @dra27 ocaml/dune#4632) - Fix installation with opam of package with dune sites. The `.install` file is now produced by a local `dune install` during the build phase (ocaml/dune#4730, ocaml/dune#4645, @bobot, reported by @kit-ty-kate ocaml/dune#4198) - Fix multiple issues in the sites feature (ocaml/dune#4730, ocaml/dune#4645 @bobot, reported by @Lelio-Brun ocaml/dune#4219, by @Kakadu ocaml/dune#4325, by @toots ocaml/dune#4415)

nojb requested a review from voodoos June 16, 2021 20:37

voodoos approved these changes Jun 17, 2021

View reviewed changes

voodoos reviewed Jun 17, 2021

View reviewed changes

otherlibs/stdune-unstable/path.ml Show resolved Hide resolved

nojb force-pushed the fix_4717 branch from 26643f4 to f0ac4a6 Compare June 17, 2021 11:01

voodoos reviewed Jun 17, 2021

View reviewed changes

otherlibs/stdune-unstable/path.ml Outdated Show resolved Hide resolved

nojb added 2 commits June 17, 2021 14:33

Path.Local: backslashes are not canonical on Windows

2532fbd

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

Add doc

dea7510

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb force-pushed the fix_4717 branch from f0ac4a6 to dea7510 Compare June 17, 2021 12:33

ejgallego added this to the 2.9 milestone Jun 17, 2021

CHANGES.md

9c16df5

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb merged commit 904cbe5 into ocaml:main Jun 18, 2021

nojb deleted the fix_4717 branch June 18, 2021 11:05

nojb added a commit to nojb/dune that referenced this pull request Jun 18, 2021

Path.Local: backslashes are not canonical on Windows (ocaml#4745)

4ca3db1

nojb mentioned this pull request Jun 18, 2021

[2.9-backport] Path.Local: backslashes are not canonical on Windows (#4745) #4752

Merged

nojb added a commit to nojb/dune that referenced this pull request Jun 18, 2021

Path.Local: backslashes are not canonical on Windows (ocaml#4745)

149e482

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

nojb added a commit that referenced this pull request Jun 18, 2021

Path.Local: backslashes are not canonical on Windows (#4745) (#4752)

5c41338

Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>

ejgallego mentioned this pull request Jun 21, 2021

[new release] dune-action-plugin, dune-glob, dune-site, dune-configurator, dune-private-libs, dune and dune-build-info (2.9.0) ocaml/opam-repository#18904

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path.Local: backslashes are not canonical on Windows#4745

Path.Local: backslashes are not canonical on Windows#4745
nojb merged 3 commits intoocaml:mainfrom
nojb:fix_4717

nojb commented Jun 16, 2021

Uh oh!

voodoos left a comment

Uh oh!

Uh oh!

Uh oh!

nojb commented Jun 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	let parse_string_exn ~loc s =
	match s with
	\| ""
	\| "." ->
	root
	\| _ when is_canonicalized s -> make s
	\| _ -> relative root s ~error_loc:loc

Conversation

nojb commented Jun 16, 2021

Uh oh!

voodoos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nojb commented Jun 18, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants