Create cookie file for toolchain packages #10782
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gridbugs
force-pushed
the
toolchain-cookies
branch
from
36aa52d to
88c87a5
Compare
rgrinberg
approved these changes
Jul 30, 2024
rgrinberg
reviewed
Jul 30, 2024
src/dune_rules/pkg_rules.ml
Outdated
| match build_command with | ||
| | None | Some Dune -> Memo.return build_command | ||
| | Some (Action action) -> | ||
| let+ installed = Fs_memo.dir_exists prefix in |
There was a problem hiding this comment.
Note that doing this check at rule generation time at is wrong as I've mentioned. Doesn't prevent this PR from being merged, but you should work on pushing these checks to action execution.
rgrinberg
reviewed
Jul 30, 2024
src/dune_rules/pkg_rules.ml
Outdated
| (* If the toolchain is already installed, just create an empty | ||
| config.cache file so other packages see that the compiler package | ||
| is installed. *) | ||
| Dune_lang.Action.Run |
There was a problem hiding this comment.
Any reason why you moved this from Pkg_toolchain? I'd really prefer to keep this feature as isolated as possible. I realize, we can't fully accomplish that, but we should at least try.
There was a problem hiding this comment.
Not a strong reason, no. I'll move it back.
This fixes two issues with cookies for toolchain packages: Prior to this change, dune expected toolchain cookies to be created inside the toolchains directory, ie. outside of the project. The machinery for creating cookies only works within the build directory, so cookies for toolchain packages were never created. The location of the cookie had to be the same as the location of the installed package, in what dune referred to as the package's "target_dir". The fix is to decouply these two locations by adding a new path "prefix" to packages, corresponding to the "--prefix" argument of most configure scripts. This allows toolchain packages to have a "target_dir" within the build directory, and for a cookie file to be created within its "target_dir", while still allowing the package to be installed outside the project inside its "prefix", inside the toolchains directory. The second issue is that the contents of the cookie was based on the package's "target_dir", and thus cookies for toolchain packages would be empty (toolchain packages are now installed in the package's "prefix" dir instead). Dune makes the distinction between the "paths" and "write_paths" of a package, the latter being the unsandboxed final install paths of the package, and it's the "target_dir" within a package's "write_paths" that is traversed (in the absence of a <package>.install file) to determine the contents of the package's cookie. The fix was to detect the case where the "paths" of a package contains a prefix referring outside the build directory, and in such a case, traverse the directory at that path rather than the one at "target_dir". Toolchains still don't completely work, as in some cases build tools are unable to find binaries installed by the compiler package, however manual inspection shows that the cookie is being created for toolchain packages and contains the expected contents. Fixes for the remaining issues will come in later changes. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
gridbugs
force-pushed
the
toolchain-cookies
branch
from
88c87a5 to
cccb665
Compare
anmonteiro
pushed a commit
to anmonteiro/dune
that referenced
this pull request
Nov 17, 2024
This fixes two issues with cookies for toolchain packages: Prior to this change, dune expected toolchain cookies to be created inside the toolchains directory, ie. outside of the project. The machinery for creating cookies only works within the build directory, so cookies for toolchain packages were never created. The location of the cookie had to be the same as the location of the installed package, in what dune referred to as the package's "target_dir". The fix is to decouply these two locations by adding a new path "prefix" to packages, corresponding to the "--prefix" argument of most configure scripts. This allows toolchain packages to have a "target_dir" within the build directory, and for a cookie file to be created within its "target_dir", while still allowing the package to be installed outside the project inside its "prefix", inside the toolchains directory. The second issue is that the contents of the cookie was based on the package's "target_dir", and thus cookies for toolchain packages would be empty (toolchain packages are now installed in the package's "prefix" dir instead). Dune makes the distinction between the "paths" and "write_paths" of a package, the latter being the unsandboxed final install paths of the package, and it's the "target_dir" within a package's "write_paths" that is traversed (in the absence of a <package>.install file) to determine the contents of the package's cookie. The fix was to detect the case where the "paths" of a package contains a prefix referring outside the build directory, and in such a case, traverse the directory at that path rather than the one at "target_dir". Toolchains still don't completely work, as in some cases build tools are unable to find binaries installed by the compiler package, however manual inspection shows that the cookie is being created for toolchain packages and contains the expected contents. Fixes for the remaining issues will come in later changes. Signed-off-by: Stephen Sherratt <stephen@sherra.tt>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes two issues with cookies for toolchain packages:
Prior to this change, dune expected toolchain cookies to be created inside the toolchains directory, ie. outside of the project. The machinery for creating cookies only works within the build directory, so cookies for toolchain packages were never created. The location of the cookie had to be the same as the location of the installed package, in what dune referred to as the package's "target_dir". The fix is to decouply these two locations by adding a new path "prefix" to packages, corresponding to the "--prefix" argument of most configure scripts. This allows toolchain packages to have a "target_dir" within the build directory, and for a cookie file to be created within its "target_dir", while still allowing the package to be installed outside the project inside its "prefix", inside the toolchains directory.
The second issue is that the contents of the cookie was based on the package's "target_dir", and thus cookies for toolchain packages would be empty (toolchain packages are now installed in the package's "prefix" dir instead). Dune makes the distinction between the "paths" and "write_paths" of a package, the latter being the unsandboxed final install paths of the package, and it's the "target_dir" within a package's "write_paths" that is traversed (in the absence of a .install file) to determine the contents of the package's cookie. The fix was to detect the case where the "paths" of a package contains a prefix referring outside the build directory, and in such a case, traverse the directory at that path rather than the one at "target_dir".
Toolchains still don't completely work, as in some cases build tools are unable to find binaries installed by the compiler package, however manual inspection shows that the cookie is being created for toolchain packages and contains the expected contents. Fixes for the remaining issues will come in later changes.