JuliaLang / PackageCompiler.jl

Compile your Julia Package
https://julialang.github.io/PackageCompiler.jl/dev/
MIT License
1.4k stars 186 forks source link

Update LOAD_PATH in `dump_preferences` to include `@stdlibs` #878

Closed sloede closed 8 months ago

sloede commented 9 months ago

Hopefully fixes #873.

sloede commented 9 months ago

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

fredrikekre commented 9 months ago

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 commented 9 months ago

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[bot] commented 9 months ago

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

fredrikekre commented 9 months ago

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 commented 9 months ago

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 commented 8 months ago

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 commented 8 months ago

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