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

ci: Set shell: bash as a default, remove duplicates #74418

Merged
merged 3 commits into from
Jul 18, 2020

Conversation

rye
Copy link
Contributor

@rye rye commented Jul 16, 2020

A follow-up to #74406, this commit merely removes the shell: bash lines where they were added in favor of setting defaults for all "run" steps in the jobs that run the tests.

The changes in #74406 were needed because of an upstream change to the windows-2019 GitHub Actions image. Previously, the configuration worked fine without specifying shell: bash, but for some reason this broke with a new change that was deployed today. The preceding PR was a hotfix to get CI passing, but there was a slightly less duplicative way to specify the default shell for the jobs, which was to set the defaults.run option.

This change applies to the pr, try, auto, and auto-fallible jobs, which are derived from the YAML-anchor base-ci-job. I did not apply these changes to the master, try-success, try-failure, auto-success, or auto-failure jobs because they have only a few steps.

cc/r? @Mark-Simulacrum

A follow-up to rust-lang#74406, this commit merely removes the `shell: bash`
lines where they are explicitly added in favor of setting defaults for
*all* "run" steps.

Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 16, 2020
@Mark-Simulacrum
Copy link
Member

This sounds good to me, but let's r? @pietroalbini to make sure that we get some independent eyes on this (after all I self-approved the initial addition here).

@pietroalbini
Copy link
Member

So, we already had a defaults.run.shell, which called the src/ci/exec-with-shell.py script. The script then called either bash or the contents of the CI_OVERRIDE_SHELL environment variable as the shell.

I implemented this because we were installing our own copy of msys2 and we needed to switch to its bash after it was installed. Since we're not installing our own msys2 anymore we can remove the exec-with-shell.py script, remove the CI_OVERRIDE_SHELL environment variable from src/ci/scripts/install-msys2.sh and switch the top-level defaults.run.shell to bash.

rye added 2 commits July 17, 2020 08:29
This will render the src/ci/exec-with-shell.py script more or less
useless, but we're going to replace that by just using the system bash
instead.

Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
Also, promote defaults.run.shell from inside only the primary jobs to
the top level.

The src/ci/exec-with-shell.py wrapper script was formerly used to change
out the shell mid-job by intercepting a CI_OVERRIDE_SHELL environment
variable.  Now, instead, we just set `bash` as the global default across
all jobs, and we also delete the exec-with-shell.py script.

Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
@rye
Copy link
Contributor Author

rye commented Jul 17, 2020

Ah, perfect, thanks! I hadn't caught that the entire override step was no longer necessary.

I have pushed the changes you requested.

@pietroalbini
Copy link
Member

@bors r+ rollup=iffy

Thanks!

@bors
Copy link
Contributor

bors commented Jul 17, 2020

📌 Commit 586629c has been approved by pietroalbini

@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 Jul 17, 2020
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…roalbini

ci: Set `shell: bash` as a default, remove duplicates

A follow-up to rust-lang#74406, this commit merely removes the `shell: bash` lines where they were added in favor of setting defaults for *all* "run" steps in the jobs that run the tests.

The changes in rust-lang#74406 were needed because of an upstream change to the `windows-2019` GitHub Actions image. Previously, the configuration worked fine without specifying `shell: bash`, but for some reason this broke with a new change that was deployed today. The preceding PR was a hotfix to get CI passing, but there was a slightly less duplicative way to specify the default shell for the jobs, which was to set the `defaults.run` option.

This change applies to the `pr`, `try`, `auto`, and `auto-fallible` jobs, which are derived from the YAML-anchor `base-ci-job`.  I did not apply these changes to the `master`, `try-success`, `try-failure`, `auto-success`, or `auto-failure` jobs because they have only a few steps.

cc/r? @Mark-Simulacrum
Manishearth added a commit to Manishearth/rust that referenced this pull request Jul 18, 2020
…roalbini

ci: Set `shell: bash` as a default, remove duplicates

A follow-up to rust-lang#74406, this commit merely removes the `shell: bash` lines where they were added in favor of setting defaults for *all* "run" steps in the jobs that run the tests.

The changes in rust-lang#74406 were needed because of an upstream change to the `windows-2019` GitHub Actions image. Previously, the configuration worked fine without specifying `shell: bash`, but for some reason this broke with a new change that was deployed today. The preceding PR was a hotfix to get CI passing, but there was a slightly less duplicative way to specify the default shell for the jobs, which was to set the `defaults.run` option.

This change applies to the `pr`, `try`, `auto`, and `auto-fallible` jobs, which are derived from the YAML-anchor `base-ci-job`.  I did not apply these changes to the `master`, `try-success`, `try-failure`, `auto-success`, or `auto-failure` jobs because they have only a few steps.

cc/r? @Mark-Simulacrum
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 18, 2020
…arth

Rollup of 11 pull requests

Successful merges:

 - rust-lang#72414 ( Add lazy initialization primitives to std)
 - rust-lang#74069 (Compare tagged/niche-filling layout and pick the best one)
 - rust-lang#74418 (ci: Set `shell: bash` as a default, remove duplicates)
 - rust-lang#74441 (bootstrap.py: patch RPATH on NixOS to handle the new zlib dependency.)
 - rust-lang#74444 (Add regression test for rust-lang#69414)
 - rust-lang#74448 (improper_ctypes_definitions: allow `Box`)
 - rust-lang#74449 (Test codegen of compare_exchange operations)
 - rust-lang#74450 (Fix `Safety` docs for `from_raw_parts_mut`)
 - rust-lang#74453 (Use intra-doc links in `str` and `BTreeSet`)
 - rust-lang#74457 (rustbuild: drop tool::should_install)
 - rust-lang#74464 (Use pathdiff crate)

Failed merges:

r? @ghost
@bors bors merged commit 7b66c66 into rust-lang:master Jul 18, 2020
ehuss pushed a commit to ehuss/rust that referenced this pull request Jul 22, 2020
…roalbini

ci: Set `shell: bash` as a default, remove duplicates

A follow-up to rust-lang#74406, this commit merely removes the `shell: bash` lines where they were added in favor of setting defaults for *all* "run" steps in the jobs that run the tests.

The changes in rust-lang#74406 were needed because of an upstream change to the `windows-2019` GitHub Actions image. Previously, the configuration worked fine without specifying `shell: bash`, but for some reason this broke with a new change that was deployed today. The preceding PR was a hotfix to get CI passing, but there was a slightly less duplicative way to specify the default shell for the jobs, which was to set the `defaults.run` option.

This change applies to the `pr`, `try`, `auto`, and `auto-fallible` jobs, which are derived from the YAML-anchor `base-ci-job`.  I did not apply these changes to the `master`, `try-success`, `try-failure`, `auto-success`, or `auto-failure` jobs because they have only a few steps.

cc/r? @Mark-Simulacrum
@ehuss ehuss mentioned this pull request Jul 22, 2020
@ehuss ehuss mentioned this pull request Jul 23, 2020
@rye rye deleted the gha-dedup-shell-setting branch July 30, 2021 12:36
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants