Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Jun 18, 2021
Merged

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

merged 3 commits into from
Jun 18, 2021

Conversation

nojb
Copy link
Collaborator

@nojb nojb commented Jun 16, 2021

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

dune/otherlibs/stdune-unstable/path.ml

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

@nojb nojb requested a review from voodoos June 16, 2021 20:37
voodoos
voodoos approved these changes Jun 17, 2021
View reviewed changes
Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

nojb added 2 commits June 17, 2021 14:33
@nojb
Path.Local: backslashes are not canonical on Windows
2532fbd 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb
Add doc
dea7510 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@ejgallego ejgallego added this to the 2.9 milestone Jun 17, 2021
@nojb
Copy link
Collaborator Author

nojb commented Jun 18, 2021

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

@nojb
CHANGES.md
9c16df5 
Signed-off-by: Nicolás Ojeda Bär <n.oje.bar@gmail.com>
@nojb nojb merged commit 904cbe5 into ocaml:main Jun 18, 2021
@nojb 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
@nojb
Path.Local: backslashes are not canonical on Windows (ocaml#4745)
4ca3db1
@nojb nojb mentioned this pull request Jun 18, 2021
Merged
nojb added a commit to nojb/dune that referenced this pull request Jun 18, 2021
@nojb
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
@nojb
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 ejgallego mentioned this pull request Jun 21, 2021
Merged
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jun 21, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
da0714f 
…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)
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jun 29, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
a13f842 
…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)
ejgallego added a commit to ejgallego/opam-repository that referenced this pull request Jul 1, 2021
@ejgallego
[new release] dune-action-plugin, dune-glob, dune-site, dune-configur…
96c14a5 
…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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@voodoos voodoos voodoos approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
2.9
Development

Successfully merging this pull request may close these issues.

merlin broken with (include_subdirs unqualified)
3 participants
@nojb @voodoos @ejgallego