Skip to content

Use sorted inputs in the Makefile #399

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
abrown merged 1 commit into from
Mar 3, 2023
Merged

Use sorted inputs in the Makefile #399

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 5 additions & 5 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -488,13 +488,13 @@ $(SYSROOT_LIB)/libwasi-emulated-signal.a: $(LIBWASI_EMULATED_SIGNAL_OBJS) $(LIBW
%.a:
@mkdir -p "$(@D)"
# On Windows, the commandline for the ar invocation got too long, so it needs to be split up.
$(AR) crs $@ $(wordlist 1, 199, $^)
$(AR) crs $@ $(wordlist 200, 399, $^)
$(AR) crs $@ $(wordlist 400, 599, $^)
$(AR) crs $@ $(wordlist 600, 799, $^)
$(AR) crs $@ $(wordlist 1, 199, $(sort $^))
$(AR) crs $@ $(wordlist 200, 399, $(sort $^))
$(AR) crs $@ $(wordlist 400, 599, $(sort $^))
$(AR) crs $@ $(wordlist 600, 799, $(sort $^))
# 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 $^))
Copy link
Member

@sbc100 sbc100 Mar 1, 2023

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.

Copy link
Contributor Author

@PieroV PieroV Mar 1, 2023

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.

Copy link
Member

@sbc100 sbc100 Mar 1, 2023

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.

Copy link
Contributor Author

@PieroV PieroV Mar 2, 2023

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 Makefiles), 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 🙂 ).


$(MUSL_PRINTSCAN_OBJS): CFLAGS += \
-D__wasilibc_printscan_no_long_double \
Expand Down