-
Notifications
You must be signed in to change notification settings - Fork 200
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
Use sorted inputs in the Makefile #399
Conversation
# This might eventually overflow again, but at least it'll do so in a loud way instead of | ||
# silently dropping the tail. | ||
$(AR) crs $@ $(wordlist 800, 100000, $^) | ||
$(AR) crs $@ $(wordlist 800, 100000, $(sort $^)) |
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.
Isn't it enough to simply sort when calling AR here?
I don't see why the sorting needs to happen up in LIBC_TOP_HALF_MUSL_SOURCES
. IIUC those all get built in parallel anyway so it won't dictate the order of the building of the object files?
Also we should probably just switch to using a response file here rather than using multiple calls to AR.. but thats a different issue.
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.
No, I somehow get a different hash in the other machine (16 threads on my machine, vs. 64 threads on our build server):
a61935e89e28a8c8fc599db7a190cc729523eb1338c81fc8bf8cb8aca559a530 sysroot/lib/wasm32-wasi/libc.a
fe696f8696ce0e465beebdb26e226c0a8a08a73e530467fad962e8f53f464102 sysroot/lib/wasm32-wasi/libc.a
But interestingly enough, if I sort only the list of objects ($(sort $(LIBC_OBJS))
), I still get the same hash as a few hours ago.
That was actually my first idea, but for some reason it seemed to me that it wasn't enough, yesterday (the build path doesn't seem to influence the output).
I can change the PR to do only this (and maybe the other libraries, too, for the sake of completeness, even though I don't think they're used by the wasi-sdk).
Or maybe to keep only the changes on the %.a
part, if you prefer.
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.
Sorting the inputs to AR makes total sense. We can land that now.
I would like to know why the filelists need to be sorted though.
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.
I would like to know why the filelists need to be sorted though.
Sorting the objects was my first approach, but it didn't work, for some reason (maybe I missed one of the $^
).
So, I moved to sorting the filelists (I thought the object list was generated from them, I am not good at reading Makefile
s), and that was not enough, too.
Eventually I have tried to sort both, and it worked, so I have opened the PR with this change.
Only after your prompt I have checked again the sort applied only to the object list.
So, I think that the filelist doesn't need to be sorted after all (it doesn't hurt either, though 🙂 ).
59d8ad0
to
0c4f3e5
Compare
This makes builds reproducible.
0c4f3e5
to
ef21e4f
Compare
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.
If this more minimal version works to make the builds reproducible, I'm a +1!
I've tried the latest revision again with our build system and it seems that yes, it's enough to get a reproducible wasi-libc and even a reproducible wasi-sdk 😄 . |
This makes builds reproducible.
This change brings in several helpful PRs (e.g., WebAssembly/wasi-libc#397, WebAssembly/wasi-libc#399, WebAssembly/wasi-libc#401) in anticipation of creating a release based on LLVM 16.
This change brings in several helpful PRs (e.g., WebAssembly/wasi-libc#397, WebAssembly/wasi-libc#399, WebAssembly/wasi-libc#401) in anticipation of creating a release based on LLVM 16.
This makes builds reproducible (see #398).
I have tried in two separate Debian testing machines, with different hardware specs (in particular, I think the number of cores can influence the output), the repository cloned in
/tmp
and LLVM/Clang provided by Debian (currently, 14.0.6).I had this hash in both machines:
@abrown