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.

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

Update LOAD_PATH in dump_preferences to include @stdlibs #878

Merged

Conversation

sloede
Copy link
Collaborator

@sloede sloede commented Oct 26, 2023

Hopefully fixes #873.

@sloede
Copy link
Collaborator Author

sloede commented Oct 26, 2023

@fredrikekre it would be great if you could have a look if this is what you had in mind

@sloede sloede mentioned this pull request Oct 26, 2023
@fredrikekre
Copy link
Member

Should be possible to always just use @stdlib as the complete load path. --project will set the active projet that Pkg will look at, but it isn't a problem that this isn't in the path, I think. The only thing I am not sure of is how preferences in other load path entries are handled then.

@sloede
Copy link
Collaborator Author

sloede commented Oct 26, 2023

The only thing I am not sure of is how preferences in other load path entries are handled then.

Yeah, exactly. This is what has me worried as well, since the whole point of using an external process for getting the preferences is to make sure that we do not miss anything (or mix it up with another project).

Instead of using LOAD_PATH for it, I could also just set

new_load_path = get(ENV, "JULIA_LOAD_PATH", "") * "$(splitter)@stdlib"

That would prevent the LOAD_PATH as set for the current PC process to spill into the new process, but respect whatever the user has set in JULIA_LOAD_PATH.

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Merging #878 (7c4acf1) into master (037a72e) will decrease coverage by 0.02%.
Report is 3 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #878      +/-   ##
==========================================
- Coverage   84.28%   84.26%   -0.02%     
==========================================
  Files           3        3              
  Lines         802      801       -1     
==========================================
- Hits          676      675       -1     
  Misses        126      126              
Files Coverage Δ
src/PackageCompiler.jl 92.57% <ø> (ø)

... and 1 file with indirect coverage changes

@sloede sloede marked this pull request as ready for review October 26, 2023 14:48
@fredrikekre
Copy link
Member

To be completely idiot-proof I think you should always put @stdlib as the first entry, and then in the script, just after using Pkg, TOML pop this entry from the load path or something like that.

@sloede
Copy link
Collaborator Author

sloede commented Nov 3, 2023

To be completely idiot-proof I think you should always put @stdlib as the first entry, and then in the script, just after using Pkg, TOML pop this entry from the load path or something like that.

Are you sure that is necessary? In that case I would need to track if I added @stdlib manually, and if yes, remove it again. I feel like this is overkill.

But which situation did you have in mind that would be guarded against using this approach with @stdlib as the first entry and then removing it again?

@fredrikekre
Copy link
Member

fredrikekre commented Nov 6, 2023

In that case I would need to track if I added @stdlib manually, and if yes, remove it again.

You should be able to do it unconditionally.

But which situation did you have in mind [...]

A user could theoretically have another package named Pkg or TOML in their project (with different UUID). I also think the patch becomes smaller with this approach too:

diff --git a/src/PackageCompiler.jl b/src/PackageCompiler.jl
index 3624b35..572a7e1 100644
--- a/src/PackageCompiler.jl
+++ b/src/PackageCompiler.jl
@@ -1536,7 +1536,9 @@ function dump_preferences(io::IO, project_dir)
     # Note: in `command` we cannot just use `Base.get_preferences()`, since this API was
     #       only introduced in Julia v1.8
     command = """
+    pushfirst!(LOAD_PATH, "@stdlib")
     using TOML, Pkg
+    popfirst!(LOAD_PATH)
     # For each dependency pair (UUID => PackageInfo), store preferences in Dict
     prefs = Dict{String,Any}(last(dep).name => Base.get_preferences(first(dep)) for dep in Pkg.dependencies())
     # Filter out packages without preferences

@fredrikekre
Copy link
Member

Please remove the @-mention from the squash later, looks like Github still pings when such commits are pushed...

@sloede sloede merged commit c2f433e into JuliaLang:master Nov 8, 2023
24 of 45 checks passed
@sloede sloede deleted the msl/update-load-path-in-dump-preferences branch November 8, 2023 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Required TOML.jl
2 participants