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

fix precompile process flags #56214

Merged
merged 1 commit into from
Oct 18, 2024
Merged

fix precompile process flags #56214

merged 1 commit into from
Oct 18, 2024

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Oct 17, 2024

CacheFlags could get set, but were never propagated to the target process, so the result would be unusable. Additionally, the debug and optimization levels were not synchronized with the sysimg, causing a regression in pkgimage usability after moving out stdlibs.

Fixes #56207
Fixes #56054
Fixes #56206

@vchuravy is this the intent for debug flags, or is a stripped binary supposed to be a different cache entry? If not, would you accept it if we compiled with -gsplit-dwarf? https://gcc.gnu.org/wiki/DebugFission

@vtjnash vtjnash added the backport 1.11 Change should be backported to release-1.11 label Oct 17, 2024
@@ -25,7 +25,8 @@ print-depot-path:
@$(call PRINT_JULIA, $(call spawn,$(JULIA_EXECUTABLE)) --startup-file=no -e '@show Base.DEPOT_PATH')

$(BUILDDIR)/stdlib/%.image: $(JULIAHOME)/stdlib/Project.toml $(JULIAHOME)/stdlib/Manifest.toml $(INDEPENDENT_STDLIBS_SRCS) $(JULIA_DEPOT_PATH)/compiled
@$(call PRINT_JULIA, JULIA_CPU_TARGET="$(JULIA_CPU_TARGET)" $(call spawn,$(JULIA_EXECUTABLE)) --startup-file=no -e 'Base.Precompilation.precompilepkgs(;configs=[``=>Base.CacheFlags(), `--check-bounds=yes`=>Base.CacheFlags(;check_bounds=1)])')
@$(call PRINT_JULIA, JULIA_CPU_TARGET="$(JULIA_CPU_TARGET)" $(call spawn,$(JULIA_EXECUTABLE)) --startup-file=no -e \
'Base.Precompilation.precompilepkgs(configs=[``=>Base.CacheFlags(debug_level=2, opt_level=3), ``=>Base.CacheFlags(check_bounds=1, debug_level=2, opt_level=3)])')
Copy link
Member

@IanButterworth IanButterworth Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The left part of the pair is used to annotate the process configurations in the precompilepkgs status, so needs to be meaningfully set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Screenshot 2024-10-17 at 11 10 50 AM

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oof, that seems like a mistake. I'll update to fix the printing

@gbaraldi
Copy link
Member

Could this also be #56177?

@giordano giordano added the compiler:precompilation Precompilation of modules label Oct 17, 2024
@giordano
Copy link
Contributor

Could this also be #56177?

At a quick glance, that seems to be already fixed on master. I haven't been able to reproduce it with a local build from source yet, I suspect one needs to set JULIA_CPU_TARGET to trigger the issue.

CacheFlags could get set, but were never propagated to the target
process, so the result would be unusable. Additionally, the debug and
optimization levels were not synchronized with the sysimg, causing a
regression in pkgimage usability after moving out stdlibs. The printing
also failed to meaningfully represent the settings for the same reasons.

Also fixes a concurrency bug in loading.jl tests that was failing.

Fixes #56207
Fixes #56054
Fixes #56206
@KristofferC KristofferC mentioned this pull request Oct 18, 2024
41 tasks
@vtjnash vtjnash merged commit 82b1506 into master Oct 18, 2024
5 of 7 checks passed
@vtjnash vtjnash deleted the jn/pkgimage-missing-opts branch October 18, 2024 19:37
@IanButterworth
Copy link
Member

IanButterworth commented Oct 18, 2024

For review-sake it now looks like this after this PR

Screenshot 2024-10-18 at 4 16 01 PM

@@ -677,7 +705,7 @@ function precompilepkgs(pkgs::Vector{String}=String[];
n_print_rows = 0
while !printloop_should_exit
lock(print_lock) do
term_size = Base.displaysize_(io)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not needed because io is now inferred?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

correct, io is now concrete

KristofferC pushed a commit that referenced this pull request Oct 21, 2024
CacheFlags could get set, but were never propagated to the target
process, so the result would be unusable. Additionally, the debug and
optimization levels were not synchronized with the sysimg, causing a
regression in pkgimage usability after moving out stdlibs.

Fixes #56207
Fixes #56054
Fixes #56206

(cherry picked from commit 82b1506)
KristofferC pushed a commit that referenced this pull request Oct 21, 2024
CacheFlags could get set, but were never propagated to the target
process, so the result would be unusable. Additionally, the debug and
optimization levels were not synchronized with the sysimg, causing a
regression in pkgimage usability after moving out stdlibs.

Fixes #56207
Fixes #56054
Fixes #56206
maleadt pushed a commit that referenced this pull request Oct 21, 2024
CacheFlags could get set, but were never propagated to the target
process, so the result would be unusable. Additionally, the debug and
optimization levels were not synchronized with the sysimg, causing a
regression in pkgimage usability after moving out stdlibs.

Fixes #56207
Fixes #56054
Fixes #56206

(cherry picked from commit 82b1506)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.11 Change should be backported to release-1.11 compiler:precompilation Precompilation of modules
Projects
None yet
5 participants