-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add support for new build layout in 24.1 #384
Conversation
Mac OS build fails with:
I have no idea how to fix that and don't have a mac build machine. @zakkak probably something for you? I'm looking at the windows build failure... |
Windows build failure seems to be:
|
or maybe not. A different run has:
|
graalvm/mx@f49c56f seems to be the mx commit which caused this new build failure. |
de8f1c6
to
294f441
Compare
Not sure what's wrong with the windows PR tester build, but it built fine on windows here: |
On it. |
The MacOS issue is caused by adoptium/temurin-build#3602, note however that even after manually renaming the folder the build still fails because in darwin I also see some formatting issues (tabs vs spaces) causing indentation inconsistencies. |
This looks like a build using The actual issue seems to indeed be some failure to create a directory.
I see that you use a different mandrel-packaging commit in the passing run (294f441 instead of cec4782) the only difference however appears to be the exclusion of |
Thanks. I'll fix it.
OK. I'll fix those. |
OK. Ignore that then.
Yes, the failing build without
Yes. Only |
Do you have a patch for the rename until adoptium/temurin-build#3602 is fixed? |
No, I only tested it locally by doing the rename by hand. |
Mac build is OK now. https://github.com/graalvm/mandrel/actions/runs/7460492845/job/20298676477 is a run verifying the windows build (which looks good so far). Something is strange on the PR tester, but I'm not sure what it is so I'll stop there and let somebody with a windows box look into it. I'll clean up the commits, re-test and would then like to merge so as to unbreak the windows build in CI. @zakkak How does that sound? |
Sounds good. FWIW, some differences I see in the mandrel-packaging CI vs the mandrel CI:
|
The new location is, for example, linux-amd64/glibc/liblibchelper.a when it used to be amd64/liblibchelper.a This also removes an MX 5.313 conditional which is useless in master of mandrel-packaging which is supposed to build latest graal/master (needing mx 6+).
7ad23bd
to
5780fef
Compare
@zakkak Please review, thanks! |
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.
LGTM, thanks for taking care of this @jerboaa
FWIW, https://github.com/jerboaa/graal/actions/runs/7461810722/job/20302721226 is a build with python 3.10.11 (as the PR tester uses). It built fine, so that's not it. |
We now also have PR tester runs with mx version from |
The new location is, for example,
linux-amd64/glibc/liblibchelper.a
when it used to beamd64/liblibchelper.a
.This also removes an MX 5.313 conditional which is useless in master of mandrel-packaging which is supposed to build latest graal/master (needing mx 6+).
This fixes build issues seen in CI such as this one:
https://github.com/graalvm/mandrel/actions/runs/7442144672/job/20245208110#step:8:475