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

Small CI improvements #13060

Merged
merged 7 commits into from
Apr 22, 2024
Merged

Small CI improvements #13060

merged 7 commits into from
Apr 22, 2024

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Apr 22, 2024

Objective

  • Some CI jobs specifically use macos-14, as compared to the default macos-latest.
    • macos-latest is equivalent to macos-12, but may be updated in the future.
  • The CI job that tests on the minimum supported Rust version (MSRV) uses environmental variables to save the toolchain version.
    • This specific usage is what step outputs were designed for.
    • Both do the same thing, but step outputs can be checked by the Github Actions VSCode Extension.
  • Some workflows have a NIGHTLY_TOOLCHAIN variable that let us pin the nightly version, in case a new release breaks CI.

Solution

  • Document why certain actions required macos-14.
  • Switch MSRV step to use step outputs.
  • Add a small comment documenting the purpose of the NIGHTLY_TOOLCHAIN environmental variable.

BD103 added 3 commits April 21, 2024 22:43
…alculating msrv

While both technically do the same thing, using `$GITHUB_OUTPUT` is each easier to trace. (It also does not raise a warning when using the Github Actions VSCode Extension.)
@BD103 BD103 added A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change labels Apr 22, 2024
@Brezak
Copy link
Contributor

Brezak commented Apr 22, 2024

The docs and validation-jobs workflows also contain the NIGHTLY_TOOLCHAIN env variable. They could also use a comment explaining what it's there for.

@BD103
Copy link
Member Author

BD103 commented Apr 22, 2024

I have a suspicion that macos-latest is not in fact macos-14. From this table:

Virtual Machine Processor (CPU) Memory (RAM) Storage (SSD) OS (YAML workflow label) Notes
Linux 4 16 GB 14 GB ubuntu-latest, ubuntu-22.04, ubuntu-20.04 The ubuntu-latest label currently uses the Ubuntu 22.04 runner image.
Windows 4 16 GB 14 GB windows-latest, windows-2022, windows-2019 The windows-latest label currently uses the Windows 2022 runner image.
macOS 3 14 GB 14 GB macos-12 or macos-11 The macos-11 label has been deprecated and will no longer be available after 6/28/2024.
macOS 4 14 GB 14 GB macos-13 N/A
macOS 3 (M1) 7 GB 14 GB macos-latest or macos-14 The macos-latest label currently uses the macOS 14 runner image.

On the table, there are 4 different MacOS versions. run-examples-macos-metal depends on running on macos-14 with an M1 chip, but it most recently failed with the following message:

2024-04-22T11:28:56.378338Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "MacOS 12.7.4 ", kernel: "21.6.0", cpu: "Intel(R) Xeon(R) CPU E5-1650 v2 @ 3.50GHz", core_count: "3", memory: "14.0 GiB" }

thread 'main' panicked at crates/bevy_render/src/renderer/mod.rs:185:10:
Unable to find a GPU! Make sure you have installed required drivers!
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

The system info says that it's running MacOS 12! After looking at other runs, I think macos-latest still is set to macos-12 even though it is documented as macos-14.

I'm going to set this PR back to a draft while I work on a solution to this. Likely I'll switch everything to macos-13 and macos-14. (macos-13 has 4 cores, so we may even get performance improvements!)

@BD103 BD103 marked this pull request as draft April 22, 2024 12:21
@Brezak
Copy link
Contributor

Brezak commented Apr 22, 2024

The system info says that it's running MacOS 12! After looking at other runs, I think macos-latest still is set to macos-12 even though it is documented as macos-14.

Their docs are kind of a mess. Here it says macos-latest is macOS 12.

BD103 added 2 commits April 22, 2024 11:32
`macos-13` has one more CPU core than `macos-12`.
@BD103
Copy link
Member Author

BD103 commented Apr 22, 2024

Their docs are kind of a mess. Here it says macos-latest is macOS 12.

Lol that's fun.

I reverted the change, though I'm honestly not sure why Miri uses macos-14. @mockersf, can you speak about this?

I'm also trying to set all MacOS runners to macos-13, which may give some performance improvements with an extra CPU core.

@BD103 BD103 marked this pull request as ready for review April 22, 2024 15:40
@BD103
Copy link
Member Author

BD103 commented Apr 22, 2024

From the preliminary results, there don't seem to be any benefits to using macos-13 runners. I'm going to keep them as macos-latest, so we don't have to manually update them in the future.

@mockersf
Copy link
Member

mockersf commented Apr 22, 2024

not sure why Miri uses macos-14. @mockersf, can you speak about this?

Because it's faster. See #12133, miri on macOS-14 is 2.5 mins faster than on ubuntu-latest

@james7132 james7132 added this pull request to the merge queue Apr 22, 2024
@BD103
Copy link
Member Author

BD103 commented Apr 22, 2024

Because it's faster. See #12133, miri on macOS-14 is 2.5 mins faster than on ubuntu-latest

Thanks, good to know!

Merged via the queue into bevyengine:main with commit fcd87b2 Apr 22, 2024
27 checks passed
@BD103 BD103 deleted the small-ci-improvements branch April 22, 2024 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants