use the correct path to include Compiler.jl in release builds #56601
Conversation
base/Base_compiler.jl
Outdated
| @@ -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")) | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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":
Line 322 in 4ed8814
which could be passed in as an additional argument?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
With the latest commit I can build a sysimage on both a release and source with JuliaLang/PackageCompiler.jl#1002
|
This will also need updating, or better yet making more robust to these kind of changes julia/stdlib/Profile/src/Profile.jl Line 540 in 4ed8814 |
|
Indeed this broke #56515 It's not great to ignore a review comment and leave no explanation for doing so.
|
|
I had no idea this would break that... I don't see why since it is included from the same path. I also thought it was already broken in non source builds (where there is no Your comment didn't feel like a review comment about the content of the PR, it felt like a future work note. Better to use the actual github review system with a "request changes" note if you mean that, to make things clear. Also, just saying, a regression test in #56515 would presumably have caught this. |
|
Sorry, I should've been clearer. |
…ang#56601) JuliaLang#56409 broke PackageCompiler (or other use cases where you want to compile a new core compiler from a release build) since it hardcoded the relative path `../usr/` from Base to the `shared` directory but this is not true in releases where it is at `..`.
#56409 broke PackageCompiler (or other use cases where you want to compile a new core compiler from a release build) since it hardcoded the relative path
../usr/from Base to theshareddirectory but this is not true in releases where it is at...The workaround here is ugly but I couldn't come up with anything better to compute that relative path based only on
BUILDROOT. Maybe it can work as a nerd snipe for someone to improve it.