-
Notifications
You must be signed in to change notification settings - Fork 191
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
Fix windows SDK builds #126
Conversation
Fixed tar_from_installation to not hardcode the source build dir. The final build on Windows is going to be much larger, because until recently, Windows didn't really support a useful form of symlinks so the llvm build process (probably due to cmake) just makes multiple copies to clang++, clang-cpp, clang-cl etc. This can be fixed on recent versions of Windows with developer mode enabled, but I don't know of any archive format that knows how to deal with this on Windows (ideally you could but symlinks in the archive, and it would create symlinks if they're supported otherwise just create a copy of the file). |
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.
This is awesome! Thanks for working on it.
I think its fine if the windows build is larger in roughly the same way the a normal clang/llvm distribution is larger. In other words we shouldn't be trying to fix that problem downstream if they haven't yet addressed it upstream. |
So the only issue is that my last CI run died strangely (extra newlines added to log snippet):
I have no idea what happened here. It's almost as if c:/wasi-sdk just .. went away or otherwise got screwed? Each job time limit is 6 hours, so we're nowhere near that. Let's see what happens with the new one. |
Let's try without -j4. All that stuff in potentially parallel looks strange. |
Oh wait, that broken CI run was I think one of my own. This one succeeded! https://github.com/WebAssembly/wasi-sdk/actions/runs/82251119 (it was for the "trigger on push" commit that I just nuked) |
This is awesome, thank you for working on it! Do you mind getting rid of the |
Done! |
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.
Thanks so much! Will merge as soon as CI completes.
I think the one place there the top level -j flag is useful was the $(MAKE) command for building wasi-libc. This change would cause the build of wasi-libc to only use a single core. Maybe thats fine since it so small compared to llvm? Maybe we should has a |
Ah, i forgot about that, and you are correct. I'll merge this and make a PR restoring that -j4. |
just looking that sizes, I wonder how mac manages to be so much smaller: dist-macos-latest 42.3 MB |
If you look at the build times with and without -j4: I'm not sure if it's doing much of anything, which makes sense because the the Makefile is largely serial. I'm guessing there's 1 or 2 steps that make decides it can parallelize, which causes problems because I /think/ it starts a build of some code that wants things like stddef.h before those things have actually finished copying over. |
Ah yeah -- compiler-rt and libc seem like they can run in parallel, and the end of compiler-rt does:
which is going to overwrite files that clang is going to be reading from while libc is being built. It'll overwrite them with themselves so should be okay, but on Windows you can race on reading a file (permission denied) while it's being replaced, which matches the error I was seeing in CI. |
I'm not suggesting we use -j4 on the top level make command, just on the inner libc make command |
* Add an entrypoint for calling open that bypasses the varargs. * Add an entrypoint for calling openat that bypasses the varargs.
This brings in the following changes: f645f49 Update signal macros after upgrade to snapshot1 (WebAssembly#144) 8b3266d github actions: pin checkout action to v1 (WebAssembly#145) 410c660 Use constructor functions for optional init routines. (WebAssembly#142) fe13053 c header generation updated for reorganized witx ast (WebAssembly#139) cd74e1d Correct the version of WebAssembly#136 on master (WebAssembly#141) 446cb3f Wasi snapshot preview1 (WebAssembly#140) 54102f0 Ignore rights in libpreopen. (WebAssembly#129) 8c9e1c6 Make the `__original_main` definition weak, fixing -flto. (WebAssembly#138) cf81683 Optimize `fmin`, `fmax`, etc. (WebAssembly#120) deb8eae Don't pre-check capabilities in `openat`. (WebAssembly#130) ca9046d Use consistent style for wasi-libc C source files. (WebAssembly#131) 951cc3e Fix unintended recursion in __wasilibc_register_preopened_fd. (WebAssembly#133) 5216983 Avoid a `strdup` call in `__wasilibc_populate_libpreopen`. (WebAssembly#128) 70099d4 Don't link in libpreopen initialization code when it isn't needed. (WebAssembly#127) ec4549d Temporarily disable the use of `__heap_base`. (WebAssembly#132) a214f1c Use __heap_base by dlmalloc (WebAssembly#114) a94d2d0 Avoid varargs conventions when calling open (WebAssembly#126) 7fcc4f2 Revamp and simplify the libpreopen code. (WebAssembly#110) eb7230c Remove more unsupported headers. (WebAssembly#123)
Tweak the Makefile to make things work under Windows. The assumption is that this will run under msys2 bash (i.e. git-bash), which seems to be where github CI will run it anyway. Removing that assumption would probably require not using Makefiles, and isn't really worth it.
There's also a "trigger on push to all branches" workflow thing in here that I can nuke before this lands, I needed it to test (and my final run is still running).
Fixes #39