-
Notifications
You must be signed in to change notification settings - Fork 392
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.
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
fix: copy sandboxes should copy symlinks as well #9282
fix: copy sandboxes should copy symlinks as well #9282
Conversation
src/dune_engine/sandbox.ml
Outdated
loop (Directory { perms }) ~src ~dst | ||
| { Unix.st_kind; _ } -> loop (Other st_kind) ~src ~dst | ||
| exception Unix.Unix_error (ENOENT, _, _) -> | ||
let contents = Unix.readlink (Path.to_string src) in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this case doing? Trying to recreate a broken link in the sandbox? Shouldn't this be an error instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packages may create their own broken links for their own tests for example. I think its better to ignore this rather than let dune get stuck on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that ends up being an issue in practice. Especially for packages that require broken symlinks to run their test suites. We could fork the few packages that do this, but package builds are complete black boxes to us so I think it will be easier to just support it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should also note that we don't even support broken symlinks in data_only_dirs which is something I looked into before. It's not clear how to handle that case in fs_memo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we force the users to request this behaviour rather than it being the default? Such automagic, I think, always comes back to bite later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In other words, I think seeing a failure and reacting to it by picking a special sandboxing mode wastes a lot less time compared to the build silently succeeding with unexpected results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Umm, how about I'll split this behavior from the current PR if it's controversial.
Can we force the users to request this behaviour rather than it being the default?
Sure, let's think about how. Would it be a sandboxing option? Seems like it's a bit more general and can apply to non sandboxed rules that produce directory targets. E.g. "allow_broken_symlinks_in_target_directories".
f78fd95
to
988e84e
Compare
Broken symlinks support is gone until we rework it. |
Signed-off-by: Rudi Grinberg <me@rgrinberg.com> <!-- ps-id: fac470a3-c036-414e-96ae-8aeb71153855 -->
988e84e
to
754971e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, this looks like a clear improvements, thanks! Agreed that we can consider the controversial bit separately.
CHANGES: - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Directory targets can now be caches. (ocaml/dune#9535, @rleshchinskiy) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon)
CHANGES: ### Added - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Directory targets can now be cached. (ocaml/dune#9535, @rleshchinskiy) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon) ### Changed - Check that package names in `(depends)` and related fields in `dune-project` are well-formed. (ocaml/dune#9472, fixes ocaml/dune#9270, @ElectreAAS) ### Fixed - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg)
CHANGES: ### Added - Add command `dune cache clear` to completely delete all traces of the Dune cache. (ocaml/dune#8975, @nojb) - Allow to disable Coq 0.8 deprecation warning (ocaml/dune#9439, @ejgallego) - Allow `OCAMLFIND_TOOLCHAIN` to be set per context in the workspace file through the `env` stanza. (ocaml/dune#9449, @rgrinberg) - Menhir: generate `.conflicts` file by default. Add new field to the `(menhir)` stanza to control the generation of this file: `(explain <blang expression>)`. Introduce `(menhir (flags ...) (explain ...))` field in the `(env)` stanza, delete `(menhir_flags)` field. All changes are guarded under a new version of the Menhir extension, 3.0. (ocaml/dune#9512, @nojb) - Directory targets can now be cached. (ocaml/dune#9535, @rleshchinskiy) - It is now possible to use special forms such as `(:include)` and variables `%{read-lines:}` in `(modules)` and similar fields. Note that the dependencies introduced in this way (ie the files being read) must live in a different directory than the stanza making use of them. (ocaml/dune#9578, @nojb) - Remove warning 30 from default set for projects where dune lang is at least 3.13 (ocaml/dune#9568, @gasche) - Add `coqdoc_flags` field to `coq` field of `env` stanza allowing the setting of workspace-wide defaults for `coqdoc_flags`. (ocaml/dune#9280, fixes ocaml/dune#9139, @Alizter) - ctypes: fix an error where `(ctypes)` with no `(function_description)` would cause an error trying refer to a nonexistent `_stubs.a` dependency (ocaml/dune#9302, fix ocaml/dune#9300, @emillon) ### Changed - Check that package names in `(depends)` and related fields in `dune-project` are well-formed. (ocaml/dune#9472, fixes ocaml/dune#9270, @ElectreAAS) ### Fixed - Do not ignore `(formatting ..)` settings in context or workspace files (ocaml/dune#8447, @rgrinberg) - Fixed a bug where Dune was incorrectly parsing the output of coqdep when it was escaped, as is the case on Windows. (ocaml/dune#9231, fixes ocaml/dune#9218, @Alizter) - Copying mode for sandboxes will now follow symbolic links (ocaml/dune#9282, @rgrinberg) - Forbid the empty `(binaries ..)` field in the `env` stanza in the workspace file unless language version is at least 3.2. - [coq] Fix bug in computation of flags when composed with boot theories. (ocaml/dune#9347, fixes ocaml/dune#7909, @ejgallego) - Fixed a bug where the `(select)` field of the `(libraries)` field of the `(test)` stanza wasn't working properly. (ocaml/dune#9387, fixes ocaml/dune#9365, @Alizter) - Fix handling of the `PATH` argument to `dune init proj NAME PATH`. An intermediate directory called `NAME` is no longer created if `PATH` is supplied, so `dune init proj my_project .` will now initialize a project in the current working directory. (ocaml/dune#9447, fixes ocaml/dune#9209, @shonfeder) - Experimental doc rules: Correctly handle the case when a package depends upon its own sublibraries (ocaml/dune#9461, fixes ocaml/dune#9456, @jonludlam) - Resolve various public binaries to their build location, rather than to where they're copied in the `_build/install` directory (ocaml/dune#9496, fixes ocaml/dune#7908, @rgrinberg). - Correctly ignore warning flags in vendored projects (ocaml/dune#9515, @rgrinberg) - Use watch exclusions in watch mode on MacOS (ocaml/dune#9643, fixes ocaml/dune#9517, @PoorlyDefinedBehaviour) - Fix merlin configuration for `(include_subdirs qualified)` modules (ocaml/dune#9659, fixes ocaml/dune#8297, @rgrinberg) - Fix handling of `enabled_if` in binary install stanzas. Previously, we'd ignore the result of `enabled_if` when evaluating `%{bin:..}` (ocaml/dune#9707, @rgrinberg)
Add support for copying sandboxes with symlinks. We do this just by copying whatever the symlink is pointing to. We also copy broken symlinks.