-
-
Notifications
You must be signed in to change notification settings - Fork 328
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
Use ubuntu-22.04-arm
for the ARM64 test-fast
job
#1802
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is to investigate the problem on the `test-fast` job with the new ARM64 runner described in GitoxideLabs#1790. This experiment does not produce useful results yet, because it has no way to distinguish happenstance from correlation. To do that, I need either to rerun each job repeatedly, or further parameterize the matrix to do that. I'll be doing the latter, but right now this dimension has size 1 (i.e., the only value of `number` is `0`) so I don't start a large number of jobs when something is broken due to a mistake in the workflows.
This makes two changes, with the intent of producing a usable test: - Removes `nightly`, since a test is currently failing on it. It can be tested later in case it fixes the SIGSEGV bug, if other changes don't help. - Have `number` take on 16 values instead of just one. This is to make it possible to figure something out about how often the failure happens with the other variables and whether the other variables make a difference. This is needed because the failures are nondeterministic, may not even usually happen, or may happen less often but still happen for some combination of the other variables. (See GitoxideLabs#1790 for context.)
The previous experiment[1][2] didn't have enough of memory-related errors to clearly show which values of the variables have an effect, though it *looked* like the memory-related errors in `rustc` only happened in Ubuntu 24.04 (not 22.04) and only happened on the stable channel (not beta). That's one reason to increase the total number of jobs in the experiment. Another reason is that the memory-related errors are more varied. Not all were true memory errors involving SIGSEGV and SIGBUS anymore. Some were, same as reported in [3]. But some others were panics, looking like this (the index and slice vary but, in each, the start index is much larger than the length): thread 'rustc' panicked at /rustc/9fc6b43126469e3858e2fe86cafb4f0fd5068869/compiler/rustc_serialize/src/opaque.rs:269:45: range start index 159846347648097871 out of range for slice of length 39963722 Since the distribution of errors across jobs might also have related to the order and times in which jobs started, for example if there are inadvertent differences between different hosts (the ARM64 Linux runners are in preview, so this seems plausible, though fairly unlikely), this now expresses the repetition with two variables: a high-order one, listed first in the matrix, and a low-order one, listed last in the matrix. Besides to allow more reps with the same values of the meaningful variables, the reason to stop testing with `RUST_MIN_STACK` is that it didn't seem to make a difference other than to change the message shown, which suggests setting it to an even higher value. [1]: e71b0cf [2]: https://github.com/EliahKagan/gitoxide/actions/runs/12903958398 [3]: GitoxideLabs#1790
When using `dtolnay/rust-toolchain` with the `toolchain` key to specify a channel, the action version should be given as `@master`. But I accidentally kept it at `@stable`! This caused `beta` and `nightly` to refer to the most recent beta and nightly builds *prior* to the current stable version. That made the conclucions about beta and nightly builds inaccurate. This rectifies that error and repeats the experiment. See e71b0cf (1f3f6b5), GitoxideLabs#1790, and rust-lang/rust#135867 for context. (I made this mistake in both experiment 1 and experiment 2, having wrongly thought I'd changed `@stable` to `@master` for experiment 1. This commit just repeats experiment 1, but experiment 2 should also be repeated for the same reason.)
As noted in the preceding commit, when I ran experiments 1 and 2 the first time, I accidentally used `dtolnay/rust-toolchain@stable` instead of `dtolnay/rust-toolchain@master`, even though the latter is needed to use current values of the `toolchain` key rather than the builds they referred to at the time the most recent stable build was updated. The preceding commit redid experiment 1 with that fixed. This commit redoes experiment 2 with te same fix. See 5a71963 (1b3e2cd), GitoxideLabs#1790, and rust-lang/rust#135867 for context.
In case the installation method makes a difference. Also, this brings back testing of the unstable toolchain. This has just one job for each meaningful combination, so mistakes in the experiment workflow can be found before doing nine times as much work. The experiment this prepares should hopefully shed more light on GitoxideLabs#1790 (or increase confidence in the observations so far), but this is just preparation: variation across runs will likely be due to the bug being nondeterministic.
This varies: - `ubuntu-22.04-arm` vs. `ubuntu-24.04.arm` GHA runner. - Installing Rust via the `rust-toolchain` action vs. with curl.sh. - Installing the stable vs. beta Rust toolchain. - Installing nextest via `install-action` quickinstall/binstall. *If* this also confirms that the only fully consistent factor in whether errors happen is `ubuntu-22.04-arm` vs. `ubuntu-24.04.arm`, then that will make it clearer that the problem is likely specific to the `ubuntu-24.04.arm` runner. See GitoxideLabs#1790 and rust-lang/rust#135867 for context.
In the AArch64/ARM64 (64-bit, non-containerized) test-fast job, this uses the `ubuntu-22.04-arm` runner instead of the `ubuntu-24.04-arm` runner. This is to avoid the errors described in GitoxideLabs#1790, i.e., to work around rust-lang/rust#135867. Such problems have not been observed on the 22.04 runner, including in tests intended to find them, and switching to it seems to be a complete workaround for the problem. In contrast, continuing to use the 24.04 runner, but attempting to work around the problem by switching from the stable to the beta channel, looks like it would greatly decrease the frequency of the errors but not eliminate them. A problem with `actions/checkout` failing is likewise observed on the 24.04 runner only, so using 22.04 avoids that too. Because that seems like a complete workaround, this also reverts 50da7cb (GitoxideLabs#1792). That is to say that the ARM64 test-fast job is again in the `test-fast` matrix. It is capable of cancelling or being cancelled by the other `test-fast` checks. Code duplication in the workflow is somewhat decreased. The job will again block PR auto-merge. Similar errors do not seem to have occurred in the `test-32bit` job that runs an arm32v7 Docker image in `ubuntu-24.04-arm`, and it is not clear that changing the runner image would help with GitoxideLabs#1780, nor even if that issue is still happening. Therefore, it is not changed there at this time. This affects only ARM Linux runners. The x86-64 runners continue to use `ubuntu-latest`, which is currently resolved to `ubuntu-24.04`, and that does not need to be changed. Likewise, the `macos-latest` runners use ARM processors (Apple Silicon) and they are fine. Various experiments were done in a separate workflow. This commit also removes that workflow, because it is not actively needed anymore, and because, if kept, it would have to be modified to avoid running hundreds of extra checks on each and every push.
This was referenced Jan 24, 2025
Byron
approved these changes
Jan 24, 2025
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 great, and a huge improvement! Thank you!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #1790
In the AArch64/ARM64 (64-bit, non-containerized) test-fast job, this uses the
ubuntu-22.04-arm
runner instead of theubuntu-24.04-arm
runner. This is to avoid the errors described in #1790, i.e., to work around rust-lang/rust#135867.Such problems have not been observed on the 22.04 runner, including in tests intended to find them, and switching to it seems to be a complete workaround for the problem. In contrast, continuing to use the 24.04 runner, but attempting to work around the problem by switching from the stable to the beta channel, looks like it would greatly decrease the frequency of the errors but not eliminate them. A problem with
actions/checkout
failing is likewise observed on the 24.04 runner only, so using 22.04 avoids that too.Because that seems like a complete workaround, this also reverts 50da7cb (#1792). That is to say that the ARM64 test-fast job is again in the
test-fast
matrix. It is capable of cancelling or being cancelled by the othertest-fast
checks. Code duplication in the workflow is somewhat decreased. The job will again block PR auto-merge.Similar errors do not seem to have occurred in the
test-32bit
job that runs an arm32v7 Docker image inubuntu-24.04-arm
, and it is not clear that changing the runner image would help with #1780, nor even if that issue is still happening. Therefore, it is not changed there at this time.This affects only ARM Linux runners. The x86-64 runners continue to use
ubuntu-latest
, which is currently resolved toubuntu-24.04
, and that does not need to be changed. Likewise, themacos-latest
runners use ARM processors (Apple Silicon) and they are fine.Various experiments were done in a separate workflow. Those experiments comprise all but the last commit here. I often drop or squash such things, but here they seem sufficiently valuable and interesting to keep. However, if that is not preferred, another option is to squash all but the last commit into one commit. The last experiment is the most useful and comprehensive, and thus the most important to have in the history. My preference for keeping the workflow details for all of them, rather than just that last one, is slight. So I would be pleased to squash those if desired.
The last commit includes the removal of the experiment workflow. Irrespective of what is done with its history, I don't think there is value in having that file in current commits. In addition, if it were kept, it would have to be modified to avoid running hundreds of extra checks on each and every push.