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.

Sign up for GitHub

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

Jump to bottom

Fix JULIA_CPU_TARGET being propagated to workers precompiling stdlib pkgimages #54093

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fix JULIA_CPU_TARGET being propagated to workers precompiling stdlib pkgimages #54093

wants to merge 2 commits into from

Conversation

KristofferC
Copy link
Sponsor Member

@KristofferC KristofferC commented Apr 15, 2024 •
edited

Apparently (thanks ChatGPT) each line in a makefile is executed in a separate shell so adding an export line on one line does not propagate to the next line.

@topolarity
Copy link
Member

Is there any way we can make sure that this errors if we fail to propagate this again in the future?

IanButterworth
IanButterworth reviewed Apr 15, 2024
View reviewed changes
pkgimage.mk Outdated
@@ -25,8 +25,7 @@ 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
export JULIA_CPU_TARGET="$(JULIA_CPU_TARGET)"
@$(call PRINT_JULIA, $(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(), `--check-bounds=yes`=>Base.CacheFlags(;check_bounds=1)])')
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

@topolarity given the proximity it seems unnecessary. Otherwise I don't think it's valid to check inside precompilepkgs

Suggested change
@$(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 'haskey(ENV, "JULIA_CPU_TARGET") || error(); Base.Precompilation.precompilepkgs(;configs=[``=>Base.CacheFlags(), `--check-bounds=yes`=>Base.CacheFlags(;check_bounds=1)])')

@vchuravy
Copy link
Sponsor Member

I think the canonical way is something like f106bd9

@KristofferC
Copy link
Sponsor Member Author

Question, why not set the env variable "globally" together with JULIA_DEPOT_PATH and JULIA_LOAD_PATH?

topolarity
topolarity approved these changes May 10, 2024
View reviewed changes
Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

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

Should we merge this? I'm happy with it, as long as it fixes the issue for now

@KristofferC
Copy link
Sponsor Member Author

KristofferC commented May 10, 2024 •
edited

I'm ok with this but if someone has a better or more canonical way feel free to push to this branch. #54093 (comment) seems easiest to me IIUC though?

@giordano
Copy link
Contributor

With a build from this PR I get

julia> Base.parse_image_targets(Base.parse_cache_header("share/julia/compiled/v1.12/Pkg/tUTdb_fyEGR.ji")[7])
1-element Vector{Base.ImageTarget}:
 generic; flags=0; features_en=(cx16)

which seems to suggest JULIA_CPU_TARGET is still not being effectively propagated where needed?

KristofferC and others added 2 commits May 14, 2024 15:09
@KristofferC
Fix JULIA_CPU_TARGET being propagated to workers precompiling stdli…
830f23b 
…b pkgimages

Apparently (thanks ChatGPT) each line in a make file is executed in a
separate shell so adding an `export` line on one line does not propagate
to the next line.
show some printouts
0bc1842
@KristofferC KristofferC mentioned this pull request May 14, 2024
Open
@KristofferC
Copy link
Sponsor Member Author

KristofferC commented May 14, 2024 •
edited

The command to compile the pkgimage seems to include the CPU target properly:

julia_cmd(; cpu_target) = `julia-master/usr/bin/julia -C 'generic;sandybridge,-xsaveopt,clone_all;haswell,-rdrnd,base(1);x86-64-v4,-rdrnd,base(1)' -Jjulia-master/usr/lib/julia/sys.dylib -g1 --startup-file=no`

but yet

julia> cd(joinpath(Sys.BINDIR, "../share/julia/compiled/v1.12/Test"))

julia> Base.parse_image_targets(Base.parse_cache_header("JfdTE_tFidc.ji")[7])
1-element Vector{Base.ImageTarget}:
 generic; flags=0; features_en=(cx16)

@gbaraldi
Copy link
Member

gbaraldi commented May 14, 2024 •
edited

is Base.precompilepkgs ignoring this? It seems to take in a CacheFlags argument that is empty

@KristofferC
Copy link
Sponsor Member Author

is Base.precompilepkgs ignoring this? It seems to take in a CacheFlags argument that is empty

CacheFlags are AFAIU not related to the CPU target and even if it was, my latest commit prints the julia command which does include the target.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@IanButterworth IanButterworth IanButterworth left review comments

@topolarity topolarity topolarity approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@KristofferC @topolarity @vchuravy @giordano @gbaraldi @IanButterworth