-
-
Notifications
You must be signed in to change notification settings - Fork 15
Add --trim-mode (compile-time) preference #31 (fixed) #65
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
base: main
Are you sure you want to change the base?
Conversation
|
please review! |
|
@topolarity please take a look! |
src/compiling.jl
Outdated
| # read a compile-time preference (e.g. Preferences.load_preference(Preferences, "_trim_enabled")). | ||
| env_overrides = Dict{String,Any}("JULIA_LOAD_PATH"=>nothing) | ||
| tmp_depot = nothing | ||
| if is_trim_enabled(recipe) |
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.
Does this cause Julia to re-precompile the whole environment for every run? Or just the packages that have read that preference
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 assume this is working around JuliaPackaging/Preferences.jl#86
We should probably fix that bug instead
|
Sorry to have hijacked your PR slightly |
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.
Pull request overview
This PR adds compile-time preference support for trim mode, enabling packages to detect during precompilation whether trimming is enabled. The implementation creates a temporary Julia environment with a LocalPreferences.toml file that sets trim_enabled = true, making this preference available to packages during the precompilation subprocess.
Key Changes:
- Modified precompilation flow to inject trim preferences via a temporary environment in
JULIA_LOAD_PATH - Added test project
TrimPrefsProjectthat reads the preference and validates propagation - Added integration test to verify the preference affects compiled executable behavior
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
src/compiling.jl |
Adds temporary environment creation with LocalPreferences.toml for trim mode preference injection during precompilation |
test/programatic.jl |
Adds integration test verifying trim preference propagation to compiled executables |
test/runtests.jl |
Adds constant for new test project path |
test/TrimPrefsProject/src/TrimPrefsProject.jl |
New test module that reads trim_enabled preference at compile time |
test/TrimPrefsProject/Project.toml |
Project definition for trim preference test |
.gitignore |
Updates pattern to ignore additional Manifest file variants |
Critical Issues Found:
- Undefined variables in cleanup code - The cleanup block (lines 148-160) references
local_prefs_fileandlocal_prefs_backupwhich are never defined, causing this code to fail at runtime - Windows compatibility issue - JULIA_LOAD_PATH uses Unix path separator
:which won't work on Windows (needs;) - Misleading test assertion - Test checks for cleanup of a file that's never created in that location
- Naming inconsistency - PR description mentions
_trim_enabledbut implementation usestrim_enabled
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
@gbaraldi No problem at all - thanks for taking a look! I’m happy to update anything if needed. |
src/compiling.jl
Outdated
| end | ||
| # Prepend the temp env to JULIA_LOAD_PATH | ||
|
|
||
| env_overrides["JULIA_LOAD_PATH"] = join([tmp_prefs_env, "@", "@stdlib"], load_path_sep) |
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.
| env_overrides["JULIA_LOAD_PATH"] = join([tmp_prefs_env, "@", "@stdlib"], load_path_sep) | |
| env_overrides["JULIA_LOAD_PATH"] = tmp_prefs_env * load_path_sep |
since JULIA_LOAD_PATH=":foo" expands to: ["@", "@v#.#", "@stdlib", "foo"]
src/compiling.jl
Outdated
| spinner_done[] = true | ||
| wait(spinner_task) | ||
| # Cleanup temporary preferences environment if created | ||
| if tmp_prefs_env !== nothing |
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.
mktempdepot(f, trim_mode) = trim_mode ? mktempdir(f) : f(nothing)
mktempdepot(trim_mode) do tmp_depot
...
endmight be a more standard pattern for this
…D_PATH for trim preferences injection
|
@topolarity @gbaraldi i have address all the reviewed changes. |
Title: Add compile-time
_trim_enabledpreference during--trimprecompilationSummary
When building with
--trim, some packages need to know at precompile time that trimming is enabled so they can register / specialize code accordingly (see SciML/LinearSolve.jl#778). This PR ensures a compile-time preference_trim_enabledis visible to packages during thePkg.instantiate(); Pkg.precompile()step invoked by JuliaC when trimming is requested.What I changed
src/compiling.jlBehavior
ImageRecipehas trimming enabled (i.e.recipe.trim_mode !== nothing && recipe.trim_mode != "no"), the precompile subprocess will:config/LocalPreferences.tomlcontaining:Pkg.instantiate(); Pkg.precompile()withJULIA_DEPOT_PATHpointed to the temporary depot soPreferences.load_preference(Preferences, "_trim_enabled")will returntruefor packages during precompilation.Why this approach
LocalPreferences.tomlhack so packages can call:Preferences.load_preference(Preferences, "_trim_enabled")and see a compile-time flag during precompilation.
Files edited
src/compiling.jl— add logic to create temporary depot, writeLocalPreferences.toml, setJULIA_DEPOT_PATHfor the precompile subprocess, and cleanup afterward.Testing & verification
julia --project -e "using Pkg; Pkg.test()"— all tests passed locally on Windows.Preferences.load_preference(Preferences, "_trim_enabled")in a top-level file and checks the behavior during thePkg.precompile()invoked by the compile pipeline.Caveats & notes
Preferences.jland readingLocalPreferences.tomlviaPreferences.load_preference. Packages that obtain preferences from other sources will not be affected.Pkg.instantiate(); Pkg.precompile()subprocess and is removed afterwards; user's depot is not modified.How to try locally
Run the existing test-suite (this was used to validate the change):
julia --project -e "using Pkg; Pkg.test()"