Skip to content

use the correct path to include Compiler.jl in release builds #56601

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

Merged
merged 3 commits into from
Nov 20, 2024
Merged

use the correct path to include Compiler.jl in release builds #56601

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
deaeb98
use the correct path to include Compiler.jl in release builds
KristofferC Nov 19, 2024
cfdcd0d
use `access` instead
Nov 20, 2024
100a9df
use datadir as well
Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion base/Base_compiler.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,11 @@ function strcat(x::String, y::String)
return out
end

function file_exists(path::String)::Bool
result = ccall(:access, Cint, (Cstring, Cint), path, 0 #=F_OK=#)
return result == 0 # If result is 0, the file exists
end

global BUILDROOT::String = ""

baremodule BuildSettings end
Expand All @@ -288,7 +293,11 @@ process_sysimg_args!()

function isready end

include(strcat(BUILDROOT, "../usr/share/julia/Compiler/src/Compiler.jl"))
if file_exists(strcat(BUILDROOT, "../usr/share/julia/Compiler/src/Compiler.jl"))
Copy link
Member

@Keno Keno Nov 20, 2024

Choose a reason for hiding this comment

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

I don't understand why the usr/share, resp share portion of this can't be folded into the definition of BUILDROOT. We're fully in control of that path both in the Makefile and in PackageCompiler. That seems like the right place for this complexity.

Copy link
Member Author

@KristofferC KristofferC Nov 20, 2024

Choose a reason for hiding this comment

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

I tried to make a "local" fix.

I don't think it can be folded into BUILDROOT because you then hit the same thing where you don't know the path to base based on BUILDROOT.

I think this shouldn't use BUILDROOT at all but instead the "datadir":

julia/Make.inc

Line 322 in 4ed8814

build_datarootdir := $(build_prefix)/share

which could be passed in as an additional argument?

Copy link
Member

@Keno Keno Nov 20, 2024

Choose a reason for hiding this comment

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

Well, there's two options I think. One is to DATAROOTDIR that instead of BUILDROOT, since currently, I believe we always link DATAROOTDIR/julia/base to BUILDROOT/base, so that was what I meant above. However, on second thought, I'm not convinced that that's right at all. Various downstream packages expect the full base sources to be available in usr/share, which I imagine is broken right now for an out of tree build. That's out of the scope of this PR, but if you want to use two arguments to be robust for potential rearrangement there, I think that's fine.

Copy link
Member Author

@KristofferC KristofferC Nov 20, 2024

Choose a reason for hiding this comment

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

With the latest commit I can build a sysimage on both a release and source with JuliaLang/PackageCompiler.jl#1002

include(strcat(BUILDROOT, "../usr/share/julia/Compiler/src/Compiler.jl"))
else
include(strcat(BUILDROOT, "../share/julia/Compiler/src/Compiler.jl"))
end

const _return_type = Compiler.return_type

Expand Down