-
Notifications
You must be signed in to change notification settings - Fork 202
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
Switch CI to fully use github actions #167
Conversation
99aff3b
to
65b8869
Compare
.github/workflows/main.yml
Outdated
- name: Install clang (Windows) | ||
shell: cmd | ||
run: | | ||
powershell -Command "$ProgressPreference = 'SilentlyContinue'; iwr -outf %TEMP%\LLVM-8.0.0-win64.exe https://rust-lang-ci2.s3.amazonaws.com/rust-ci-mirror/LLVM-8.0.0-win64.exe" |
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.
Can you remember why we can't use releases.llvm.org for windows? I seem to remember there was a bug? I wonder if updating to 9.0.0 would fix the 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.
idk, im just trying to translate the stuff thats on azure right now. Lets try upgrading it to 9.0.0 after all this stuff works.
currently stuck on an error about llvm-ar not being found (i think?) on windows, but I have to run for the night before fixing that
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.
Ended up switching to the llvm release after all, but kept everything at 8.0.0 for now.
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.
Nice! Lets delete the azure stuff in the same PR.
fff52bb
to
41a3bf7
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.
Nice!
- name: Install clang (Windows) | ||
shell: bash | ||
run: | | ||
curl -fsSLO https://releases.llvm.org/8.0.0/LLVM-8.0.0-win64.exe |
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.
So this .exe is secretly a 7zip formatted file? Strange.
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 have no idea why this works. I copy-pasted it from somewhere on the internet, and appears to do just fine. I don't have a windows machine on which to even try it, so I only know that it works here :)
.github/workflows/main.yml
Outdated
shell: bash | ||
run: | | ||
# Add --no-self-update as a workaround for | ||
# https://github.com/microsoft/azure-pipelines-image-generation/issues/1224 |
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.
Looks like this issue is closed.. can we drop this step completely?
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.
fixed d0ea5ee
with: | ||
# Upload the sysroot folder. Give it a name according to the OS it was built for. | ||
name: ${{ format( 'sysroot-{0}.tgz', matrix.os) }} | ||
path: sysroot |
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.
Hmm, do we need this?
Firstly, it should be platform-independent so at least I would upload only from the linux builder.
Secondly I wonder if we should call it something other than sysroot (maybe wasi-libc-prebuilt?).
Thirdly, does any actually need/want this artifact. I'm not sure that very useful on its own without the rest of wasi-sdk (wasi-sdk already uploads its own complete sysroot for those who have their own clang).
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 think this is useful only for troubleshooting builds. It doesn't cost anything (well, it probably costs Microsoft something in storage, but they have the money) or publish the artifacts anywhere prominent, but authors and reviewers can use it to verify that the sysroot built what they expected on platforms they don't have access to.
Ideally we'd add a separate step that downloads all three artifacts and verifies that they're the same, since we expect the build to be the same on all operating systems.
We do the same thing in wasi-sdk PRs as well, for the same reasons.
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 don't mind uploading for CI/debugging purposes. I was just thinking this might not make sense in the github releases.. is that a separate thing?
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.
yeah, this is a separate thing from github releases. Agree that its confusing, I had to figure it out the hard way too
I started by porting the azure pipeline to github actions. I ended up changing how we install llvm on windows because some environment var stuff was behaving differently on windows with github vs azure.
Additionally, uploads the built sysroot as an artifact, so it can be retrieved from the "Checks" pane of each PR.