-
Notifications
You must be signed in to change notification settings - Fork 352
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
Check if tier 2 targets build in the nightly cron job #3260
Conversation
.github/workflows/ci.yml
Outdated
@@ -128,11 +128,53 @@ jobs: | |||
- name: rustdoc | |||
run: RUSTDOCFLAGS="-Dwarnings" ./miri cargo doc --document-private-items | |||
|
|||
sysroots: |
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.
Would it make sense to use a separate workflow (in a separate yml file) for this? It doesn't seem to share anything with the main workflow except the time of the cronjob.
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 tested this PR in my fork and ironed out a few issues. Let me see if I can cut-and-paste without breaking this...
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 see any evidence that GitHub has picked up the new workflow. But it seems to have been picked up in my own repo? saethlin#1
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.
It may be the case that it only gets picked up once it exists in master.
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.
That's what I thought too, but in my fork the workflow doesn't exist on master.
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 believe for your own repo it will apply every workflow found on a local branch, but for a PR it will only apply those found on the target branch.
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 you click the link you'll see the sysroots job running in a PR, no?
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.
You are the owner of that repo, so your PRs are not regular PRs.
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 did not know that. That is very annoying.
ci/scrape-targets.py
Outdated
from bs4 import BeautifulSoup | ||
import requests | ||
|
||
url = "https://doc.rust-lang.org/nightly/rustc/platform-support.html" |
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.
Is there a way we can get this from the installed rustc rather than an online source?
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.
It looks like we can install the docs and scrape a local HTML file? Trying this, it's a bit fiddly to test locally because ./miri toolchain -c rust-docs
doesn't add them to the toolchain, but if the miri
toolchain doesn't exist yet it looks like the component is installed.
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.
Ah... well if it's complicated we can also stick to the URL and hope we don't get spurious failures from 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.
./miri toolchain -c rust-docs doesn't add them to the toolchain
So this worked after all?
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.
The CI job works because there is no pre-existing miri
toolchain when we run ./miri toolchain -c rust-docs
. It's functional, but awkward to test manually.
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.
Ah that's what you mean.
I am not sure if RTIM supports adding components to an already installed toolchain.
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.
Apart from scraping platform-support.html you could also look in https://static.rust-lang.org/dist/channel-rust-nightly.toml to find all targets for which we ship libstd (all tier 1 and tier 2 targets) In case of a rustup installed toolchain it is available locally under $(rustc --print sysroot)/lib/rustlib/multirust-channel-manifest.toml
. This doesn't distinguish between tier 1 and tier 2 though, but you could distinguish between with and without host tools by looking if rustc is available for the target or not.
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.
Or we just test tier 1 as well, it should only take a few extra minutes.
That would probably be more reliable than scraping html, yeah. Not a blocker to landing the initial experiment though.
ci/scrape-targets.py
Outdated
# The tables are: | ||
# Tier 1 | ||
# Tier 2 with host tools | ||
# Tier 2 without host tools <-- we want this one |
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.
Why only tier 2 without host tools, and not also tier 2 with host tools?
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.
🤔 At one point I was thinking that the "with host tools" part of the build would ensure that the sysroot builds, but I suppose since we don't test the host tools, the lazily-built sysroot might not build.
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.
Also this doesn't imply that the sysroot builds in Miri.
LGTM, r=me after squashing. |
ec05110
to
cb10a73
Compare
@bors r=RalfJung |
☀️ Test successful - checks-actions |
This PR adds a CI job that only runs nightly which will install Miri built from the latest commit, and try to build every Tier 2 without host tools target, as documented on https://doc.rust-lang.org/nightly/rustc/platform-support.html.
I'm not really excited about the idea of scraping the tier 2 without host tools list, but also keeping the list up-to-date by hand seems prone to forgetting to update it. And that update seems like the sort of manual maintenance we should automate.