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

fix: correctly handle absolute paths in dune ocaml top-module #8249

Merged
merged 1 commit into from
Jan 23, 2024
Merged

fix: correctly handle absolute paths in dune ocaml top-module #8249

merged 1 commit into from
Jan 23, 2024

Conversation

Alizter
Copy link
Collaborator

@Alizter Alizter commented Jul 23, 2023 •
edited
Loading

@Alizter Alizter force-pushed the ps/branch/fix__disallow_absolute_paths_in_dune_ocaml_top_module branch 2 times, most recently from 5811fed to 3411cef Compare July 24, 2023 11:17
@Alizter Alizter requested a review from emillon July 24, 2023 11:18
@rgrinberg
Copy link
Member

Instead of disallowing them, you could just strip the initial PWD and look for corresponding source file

@Alizter
Copy link
Collaborator Author

Alizter commented Jul 24, 2023

@rgrinberg What if the prefix isn't shared? I'll have to add some logic making sure its a prefix etc.

@rgrinberg
Copy link
Member

If the prefix isn't shared, then the path is outside the workspace and there's very little we can do for the user.

@Alizter
Copy link
Collaborator Author

Alizter commented Jul 24, 2023

Also I'm not sure the initial PWD is the correct thing to choose. You can imagine a project a/b/... where you call dune ocaml top-module inside b using the absolute path for a module inside a. I think we rather need the project root.

@rgrinberg
Copy link
Member

Okay sure, you can use the workspace root.

@Alizter Alizter force-pushed the ps/branch/fix__disallow_absolute_paths_in_dune_ocaml_top_module branch from 3411cef to 6cb50b8 Compare July 24, 2023 16:37
@Alizter Alizter changed the title fix: disallow absolute paths in dune ocaml top-module fix: correctly handle absolute paths in dune ocaml top-module Jul 24, 2023
@Alizter
Copy link
Collaborator Author

Alizter commented Jul 24, 2023

@rgrinberg I've implemented the prefix handling logic. There is some refactoring we should do in the future: Workspace_root should work with Path.t rather than string.

@Alizter Alizter force-pushed the ps/branch/fix__disallow_absolute_paths_in_dune_ocaml_top_module branch from 6cb50b8 to 96769ee Compare July 24, 2023 16:55
rgrinberg
rgrinberg reviewed Jul 24, 2023
View reviewed changes
bin/ocaml/top.ml Outdated
Path.Local.of_string module_path
else
let root =
(Common.root common).dir |> Path.of_string
Copy link
Member

Choose a reason for hiding this comment

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

Why don't you just use Path.root here?

@rgrinberg rgrinberg closed this Oct 20, 2023
@Alizter @rgrinberg
fix: correctly handle absolute paths in dune ocaml top-module
a8b63f7 
Signed-off-by: Ali Caglayan <alizter@gmail.com>
@rgrinberg rgrinberg added this to the 3.14.0 milestone Jan 23, 2024
@rgrinberg
Copy link
Member

I ran into this annoying issue and took the liberty to revive this PR.

@rgrinberg rgrinberg reopened this Jan 23, 2024
@rgrinberg rgrinberg force-pushed the ps/branch/fix__disallow_absolute_paths_in_dune_ocaml_top_module branch from 96769ee to a8b63f7 Compare January 23, 2024 00:22
@rgrinberg rgrinberg merged commit fc5d1d8 into ocaml:main Jan 23, 2024
26 of 27 checks passed
@emillon emillon mentioned this pull request Feb 5, 2024
Closed
15 tasks
emillon pushed a commit to emillon/dune that referenced this pull request Feb 5, 2024
@Alizter @emillon
fix: correctly handle absolute paths in dune ocaml top-module (ocaml#…
7a1a0e0 
…8249)

Signed-off-by: Ali Caglayan <alizter@gmail.com>
emillon added a commit that referenced this pull request Feb 5, 2024
@emillon @Alizter
fix: correctly handle absolute paths in dune ocaml top-module (#8249) (…
02b302c 
…#9922)

Signed-off-by: Ali Caglayan <alizter@gmail.com>
Co-authored-by: Ali Caglayan <alizter@gmail.com>
@emillon emillon mentioned this pull request Feb 5, 2024
Merged
emillon added a commit to emillon/opam-repository that referenced this pull request Feb 5, 2024
@emillon
[new release] dune (15 packages) (3.13.1)
4d0205d 
CHANGES:

- Fix performance regression for incremental builds (ocaml/dune#9769, fixes ocaml/dune#9738,
  @rgrinberg)

- Fix `dune ocaml top-module` to correctly handle absolute paths. (ocaml/dune#8249, fixes
  ocaml/dune#7370, @Alizter)

- subst: ignore broken symlinks when looking at source files (ocaml/dune#9810, fixes
  ocaml/dune#9593, @emillon)

- subst: do not fail on 32-bit systems when large files are encountered. Just
  log a warning in this case. (ocaml/dune#9811, fixes ocaml/dune#9538, @emillon)

- boot: sort directory entries in readdir. This makes the dune binary
  reproducible in terms of filesystem order. (ocaml/dune#9861, fixes ocaml/dune#9794, @emillon)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
@emillon @nberth
[new release] dune (15 packages) (3.13.1)
cc606c9 
CHANGES:

- Fix performance regression for incremental builds (ocaml/dune#9769, fixes ocaml/dune#9738,
  @rgrinberg)

- Fix `dune ocaml top-module` to correctly handle absolute paths. (ocaml/dune#8249, fixes
  ocaml/dune#7370, @Alizter)

- subst: ignore broken symlinks when looking at source files (ocaml/dune#9810, fixes
  ocaml/dune#9593, @emillon)

- subst: do not fail on 32-bit systems when large files are encountered. Just
  log a warning in this case. (ocaml/dune#9811, fixes ocaml/dune#9538, @emillon)

- boot: sort directory entries in readdir. This makes the dune binary
  reproducible in terms of filesystem order. (ocaml/dune#9861, fixes ocaml/dune#9794, @emillon)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgrinberg rgrinberg rgrinberg left review comments

@emillon emillon Awaiting requested review from emillon

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

Successfully merging this pull request may close these issues.

dune ocaml top-module crashes when given an absolute path
2 participants
@Alizter @rgrinberg