Skip to content
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

Build Fuchsia in CI #117771

Merged
merged 1 commit into from
Dec 9, 2023
Merged

Build Fuchsia in CI #117771

merged 1 commit into from
Dec 9, 2023

Conversation

tmandry
Copy link
Member

@tmandry tmandry commented Nov 10, 2023

Fittingly, when I first put this up it was failing due to discovering an ICE in clippy (looks like fixed in rust-lang/rust-clippy#11760), probably more fallout from recent type system changes. Other recent regressions this would have caught include

Originally we discussed basing this on cargotest, but they ended up not sharing anything. Fuchsia has its own tool to manage checkouts and its own build system. What it requires is a fully "install"ed toolchain with a host and fuchsia target. We share logic from the dist-various-2 builder to build the fuchsia target.

Right now this runs clippy and skips linking a bunch of targets, since most issues we catch are in the frontend. In theory we could probably get the build CPU time down quite a bit with this approach, but right now some linked targets are creeping into the dependencies anyway and we don't have a good way of preventing that yet.

The approach is basically to get a checkout at a pinned commit and then run a script at a predetermined location. I would like to update that pin every few weeks. Partial checkouts are used to minimize clone time, but we don't filter out prebuilt packages.

r? @Mark-Simulacrum

Based on discussion in this Zulip thread.

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Nov 10, 2023
@rust-log-analyzer

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

It would also be nice if we could test it on actual CI builders.

Feel free to add a builder to try or PR CI (whichever works, try might be needed for docker caches to work, at least once).

In general I'm personally happy to approve this once it's working as an experiment -- if we run into problems with it (e.g., flakiness, too long of a builder, etc.) then we can reconsider. I think if it works the relatively wide dependency graph and yet stable builds such are sufficient benefits to make Fuchsia a worthwhile sample for now, though I expect long-term we'll want to avoid blessing any particular project (e.g., by making this a distributed problem via a Crater-like solution). One option there might be to use a self-hosted GH runner which perhaps would even be run by third parties, but marked as optional in our builds, or similar. I'm personally not yet convinced that Crater is a good fit due to the non-scalable human triage element in it today.

@tmandry
Copy link
Member Author

tmandry commented Nov 10, 2023

Hopefully I did this right..

@bors try

@bors
Copy link
Contributor

bors commented Nov 10, 2023

⌛ Trying commit 234ade5 with merge 17146a2...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 10, 2023
Build Fuchsia in CI

This is very much in a draft state but I wanted to put it up now to get early feedback. It would also be nice if we could test it on actual CI builders.

Fittingly, it is failing right now due to discovering an ICE in clippy, probably more fallout from recent type system changes. Other recent regressions this would have caught include

- rust-lang#117455 and rust-lang#117493
- rust-lang#117602

This ends up not sharing very much at all with cargotest. Fuchsia has its own tool to manage checkouts and its own build system. What it requires is a fully "install"ed toolchain with a host and fuchsia target. We share logic from the dist-various-2 builder to build the fuchsia target.

Right now this runs clippy and skips linking a bunch of targets, since most issues we catch are in the frontend. In theory we could probably get the build CPU time down quite a bit with this approach, but right now some linked targets are creeping into the dependencies anyway and we don't have a good way of preventing that yet.

The approach is basically to get a checkout at a pinned commit and then run a [script](https://fuchsia-review.git.corp.google.com/c/fuchsia/+/943833/6/scripts/rust/build_fuchsia_from_rust_ci.sh) at a predetermined location. I would like to update that pin every few weeks. For now we cherry-pick some build changes but that's temporary. Partial checkouts are used to minimize clone time, but we don't filter out prebuilt packages.

r? `@Mark-Simulacrum`

Based on discussion in [this Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Putting.20Fuchsia.20in.20crater).
@rust-log-analyzer

This comment was marked as off-topic.

@bors
Copy link
Contributor

bors commented Nov 10, 2023

☀️ Try build successful - checks-actions
Build commit: 17146a2 (17146a280ee06a5870ba05d2032e4bb2c7b9d1ad)

@tmandry
Copy link
Member Author

tmandry commented Nov 11, 2023

@bors try

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Nov 11, 2023

⌛ Trying commit 057511f with merge c1cb7ea...

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 11, 2023
Build Fuchsia in CI

This is very much in a draft state but I wanted to put it up now to get early feedback. It would also be nice if we could test it on actual CI builders.

Fittingly, it is failing right now due to discovering an ICE in clippy, probably more fallout from recent type system changes. Other recent regressions this would have caught include

- rust-lang#117455 and rust-lang#117493
- rust-lang#117602

This ends up not sharing very much at all with cargotest. Fuchsia has its own tool to manage checkouts and its own build system. What it requires is a fully "install"ed toolchain with a host and fuchsia target. We share logic from the dist-various-2 builder to build the fuchsia target.

Right now this runs clippy and skips linking a bunch of targets, since most issues we catch are in the frontend. In theory we could probably get the build CPU time down quite a bit with this approach, but right now some linked targets are creeping into the dependencies anyway and we don't have a good way of preventing that yet.

The approach is basically to get a checkout at a pinned commit and then run a [script](https://fuchsia-review.git.corp.google.com/c/fuchsia/+/943833/6/scripts/rust/build_fuchsia_from_rust_ci.sh) at a predetermined location. I would like to update that pin every few weeks. For now we cherry-pick some build changes but that's temporary. Partial checkouts are used to minimize clone time, but we don't filter out prebuilt packages.

r? `@Mark-Simulacrum`

Based on discussion in [this Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Putting.20Fuchsia.20in.20crater).
@bors
Copy link
Contributor

bors commented Nov 11, 2023

☀️ Try build successful - checks-actions
Build commit: c1cb7ea (c1cb7eaca115bb89ab34af966e96de83837e9180)

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@tmandry tmandry force-pushed the fuchsia-gha branch 4 times, most recently from 64a03a2 to 2a9ed57 Compare November 17, 2023 20:27
@tmandry tmandry marked this pull request as ready for review November 17, 2023 20:30
@tmandry
Copy link
Member Author

tmandry commented Nov 17, 2023

This should be ready now, and hopefully passing now that #117979 is in.

@bors try

I decided to go with "integration" as the builder name, since it's essentially an integration test over the Fuchsia codebase. Open to suggestions though.

I'm personally not yet convinced that Crater is a good fit due to the non-scalable human triage element in it today.

I see what you mean when it comes to release runs. When running crater on individual PRs I think it would be useful. Maybe later on we can come up with a way to do this besides "add these lines to ci.yml and do a try run".

One option there might be to use a self-hosted GH runner which perhaps would even be run by third parties, but marked as optional in our builds, or similar.

Yeah, I think something like that would be good to have eventually!

@bors

This comment was marked as outdated.

bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 17, 2023
Build Fuchsia in CI

This is very much in a draft state but I wanted to put it up now to get early feedback. It would also be nice if we could test it on actual CI builders.

Fittingly, it is failing right now due to discovering an ICE in clippy (looks like fixed in rust-lang/rust-clippy#11760), probably more fallout from recent type system changes. Other recent regressions this would have caught include

- rust-lang#117455 and rust-lang#117493
- rust-lang#117602

This ends up not sharing very much at all with cargotest. Fuchsia has its own tool to manage checkouts and its own build system. What it requires is a fully "install"ed toolchain with a host and fuchsia target. We share logic from the dist-various-2 builder to build the fuchsia target.

Right now this runs clippy and skips linking a bunch of targets, since most issues we catch are in the frontend. In theory we could probably get the build CPU time down quite a bit with this approach, but right now some linked targets are creeping into the dependencies anyway and we don't have a good way of preventing that yet.

The approach is basically to get a checkout at a pinned commit and then run a [script](https://fuchsia-review.git.corp.google.com/c/fuchsia/+/943833/6/scripts/rust/build_fuchsia_from_rust_ci.sh) at a predetermined location. I would like to update that pin every few weeks. For now we cherry-pick some build changes but that's temporary. Partial checkouts are used to minimize clone time, but we don't filter out prebuilt packages.

r? `@Mark-Simulacrum`

Based on discussion in [this Zulip thread](https://rust-lang.zulipchat.com/#narrow/stream/242791-t-infra/topic/Putting.20Fuchsia.20in.20crater).
@rustbot
Copy link
Collaborator

rustbot commented Nov 29, 2023

Some changes occurred in src/tools/cargo resolved

cc @ehuss

@tmandry
Copy link
Member Author

tmandry commented Nov 29, 2023

I bumped the Fuchsia commit, and we no longer need any cherry picks to build it. Also updated the PR description.

Locally this builds fine, so I don't think another try is needed.

@tmandry
Copy link
Member Author

tmandry commented Dec 1, 2023

Rebased and still passing locally.

@tmandry tmandry force-pushed the fuchsia-gha branch 2 times, most recently from 513cb57 to e3f1827 Compare December 5, 2023 22:11
@tmandry
Copy link
Member Author

tmandry commented Dec 5, 2023

Rebased, bumped Fuchsia and squashed. Still passing locally.

@tmandry
Copy link
Member Author

tmandry commented Dec 8, 2023

Rebased and bumped Fuchsia; still passing locally.

@@ -288,6 +288,9 @@ jobs:
- name: x86_64-gnu-aux
os: ubuntu-20.04-4core-16gb
env: {}
- name: x86_64-gnu-integration
os: ubuntu-20.04-16core-64gb
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: We should try to reduce builder sizes where it's not adding value. Historically 16 core builders rarely make things that much faster -- once this lands and we collect a few dozen builds we can re-evaluate, perhaps at the next bump point for the pinned commit.

@Mark-Simulacrum
Copy link
Member

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Dec 9, 2023

📌 Commit a1b9a59 has been approved by Mark-Simulacrum

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 9, 2023
@bors
Copy link
Contributor

bors commented Dec 9, 2023

⌛ Testing commit a1b9a59 with merge c722d51...

@bors
Copy link
Contributor

bors commented Dec 9, 2023

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum
Pushing c722d51 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 9, 2023
@bors bors merged commit c722d51 into rust-lang:master Dec 9, 2023
12 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 9, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (c722d51): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.4% [0.4%, 0.4%] 1
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-1.7% [-2.5%, -0.6%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.1% [-2.5%, 0.4%] 4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-0.4% [-0.4%, -0.4%] 1
Improvements ✅
(secondary)
-5.8% [-5.8%, -5.8%] 1
All ❌✅ (primary) -0.4% [-0.4%, -0.4%] 1

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 669.584s -> 667.911s (-0.25%)
Artifact size: 314.15 MiB -> 314.09 MiB (-0.02%)

@tmandry tmandry deleted the fuchsia-gha branch December 11, 2023 23:15
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2023
Build Fuchsia on 8 cores instead of 16

Requested in rust-lang#117771 (comment).

r? `@tmandry`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 3, 2024
Build Fuchsia on 8 cores instead of 16

Requested in rust-lang#117771 (comment).

r? `@tmandry`
@nikic
Copy link
Contributor

nikic commented Jan 22, 2024

When I try to run this image locally, it always hangs during the jiri update step. Is there anything special one needs to do to run this image?

@nikic
Copy link
Contributor

nikic commented Jan 22, 2024

Nevermind, after adding -v to that command it's clear that it's not hanging, it's just downloading something like 2GB of data.

@tmandry
Copy link
Member Author

tmandry commented Jan 27, 2024

@nikic Yeah sorry, it's a big checkout with lots of prebuilts. We could look into minimizing the number of prebuilts downloaded if it's prohibitively large.

@workingjubilee workingjubilee added the O-fuchsia Operating system: Fuchsia label May 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. O-fuchsia Operating system: Fuchsia S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants