Don't rewrite Project.toml on resolve/instantiate#4713
Conversation
|
@KristofferC: Could you please review this? |
|
This should probably be backported to 1.13. |
`compile_products` previously copied the entire project tree into a temporary directory so `Pkg.instantiate` could write a manifest even when the source was read-only. That copy is harmful because it kills reproducibility: - An app which is part of a workspace loses its Manifest by the copy. - Therefore, the `instantiate` will be a `resolve` (already bad because any version changes can happen within the compatible set). But as the source entries from the top-level project are missing, too, it will in general also resolve to totally different branches from different URLs (very bad!). This PR : - Resolve the manifest in place. If no manifest applies and the directory is read-only, error out instead of silently resolving into a throwaway, non-reproducible manifest. - Inject HostCPUFeatures' `freeze_cpu_target` preference through a throwaway load-path overlay environment rather than writing a `LocalPreferences.toml` into the project. A leading empty JULIA_LOAD_PATH entry expands to Julia's default load path, so JuliaC no longer hardcodes it. - Drop the now-unused `TOML` and `Preferences` imports. - Add WorkspaceProject test fixtures and cover the in-place workspace-member build. This PR is ideally combined with [Pkg.jl#4713](JuliaLang/Pkg.jl#4713) Claude Opus 4.8 helped in creating this PR.
|
This change seems reasonable to me, but I am confused: Is there any reason to set Does |
|
That kwarg seems to have originated from #2541. Not sure what it does.CI failures might explain something of it? |
|
You want to set
Update: I am not sure whether julia> Pkg.resolve |> methods
# 2 methods for generic function "resolve" from Pkg.API:
[1] resolve(; io, kwargs...)
@ ~/.julia/juliaup/julia-1.12.6+0.x64.linux.gnu/share/julia/stdlib/v1.12/Pkg/src/API.jl:454
[2] resolve(ctx::Pkg.Types.Context; skip_writing_project, kwargs...)
@ ~/.julia/juliaup/julia-1.12.6+0.x64.linux.gnu/share/julia/stdlib/v1.12/Pkg/src/API.jl:455 |
|
@KristofferC, @topolarity: So what do you propose?
Having this PR merged would help correctly supporting workspaces in JuliaC. |
|
I think I agree with the argument and I'm generally in support - What's the cause of the CI failures though? |
ba70b61 to
4d9e3e4
Compare
4d9e3e4 to
a10f6ae
Compare
|
@PatrickHaecker Can you update the master branch in your fork? It looks like it's behind a few commits. |
Project.toml is a file edited by the user and Manifest.toml is the derived output, but resolve/instantiate shared the write_env path with add/rm and could rewrite Project.toml as a side effect of producing the manifest. With a top-level workspace [sources] entry this normalized the source path and dropped comments and formatting. Producing the manifest should never edit the project, and the project serializer is lossy, so the rewrite is destructive rather than just noisy. This bug kind of kills the reproducibility story: When you check out a repo with a Manifest.toml and a Project.toml and you `instantiate` it, you really don't want changes to your source tree! Make resolve default to skip_writing_project=true, and have instantiate pass it in both of its `up` fallbacks (missing manifest and update_on_mismatch); the normal instantiate path never wrote the project. Add a workspace regression test and a CHANGELOG entry. Claude Opus 4.8 supported in creating this PR
a10f6ae to
ea89c18
Compare
Thanks, @DilumAluthge. Seems to have been a VS Code Git problem: Fetching from upstream did not show any changes, but pulling updated now anyway) |
|
Thanks! |
Thanks for the hint. I was to much in "contribute to a package" mode and ignored issues, because they were only in nightly, which (in retrospect) obviously are the only useful tests for There is one test which previously tested the implicit rewrite of The other test should be fixed by rebasing on recent |
|
The windows test error is a disk space problem on the CI runner. So tests look good. |
Project.tomlis a file edited by the user andManifest.tomlis the derived output, butresolve/instantiateshared thewrite_envpath withadd/rmand could rewriteProject.tomlas a side effect of producing the manifest. With a top-level workspace[sources]entry this normalized the source path and dropped comments and formatting.Producing the manifest should never edit the project, and the project serializer is lossy, so the rewrite is destructive rather than just noisy. This bug kind of kills the reproducibility story: When you check out a repo with a
Manifest.tomland aProject.tomland youinstantiateit, you really don't want changes to your source tree!Make resolve default to
skip_writing_project=true, and haveinstantiatepass it in both of itsupfallbacks (missing manifest andupdate_on_mismatch); the normal instantiate path never wrote the project. Add a workspace regression test and a CHANGELOG entry.Claude Opus 4.8 supported in creating this PR