Skip to content

Fix verification script and extended tests due to rustup changes #14990

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 10 commits into from
Mar 4, 2025
Merged

Fix verification script and extended tests due to rustup changes #14990

merged 10 commits into from
Mar 4, 2025

Conversation

@alamb
Copy link
Contributor

@alamb alamb commented Mar 3, 2025 •
edited
Loading

Which issue does this PR close?

Rationale for this change

rustup previously would automatically install the toolchain (version of rust) that
was needed if it wasn't already installed.

However, the recently released rustup version changes this behavior

https://github.com/rust-lang/rustup/blob/f00c3d1fbcbe8d3ae2411e63ca906bc9b69e43d1/CHANGELOG.md?plain=1#L9-L17

Thus to ensure we have the correct toolchain installed, we need to run rustup toolchain install

What changes are included in this PR?

  • Update the verification script to call rustup toolchain install per release notes
  • Update the extended test to use the standard builder setup rather than their own special thing

Are these changes tested?

  • I verified manually that this change fixes the verification script run for the 46.0.0 RC
  • I tested extended tests with CI in XXXX

Are there any user-facing changes?

@github-actions github-actions bot added the development-process Related to development process of DataFusion label Mar 3, 2025
@alamb alamb changed the title Alamb/fix rustup installation Fix verification script and extended tests due to rustup changes Mar 3, 2025
alamb
alamb commented Mar 3, 2025
View reviewed changes
alamb
alamb commented Mar 3, 2025
View reviewed changes
dev/release/verify-release-candidate.sh
# build and test rust

# install the needed version of rust defined in rust-toolchain.toml
rustup toolchain install
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the recommended fix from rustup

@alamb alamb mentioned this pull request Mar 3, 2025
Closed
@alamb alamb mentioned this pull request Mar 3, 2025
Closed
alamb
alamb commented Mar 3, 2025
View reviewed changes
.github/workflows/extended.yml Outdated
linux-build-lib:
name: linux build test
runs-on: ubuntu-latest
container:
Copy link
Contributor Author

@alamb alamb Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this makes this job consistent with all the other CI jobs that use the rust container

alamb
alamb commented Mar 3, 2025
View reviewed changes
@alamb
Copy link
Contributor Author

alamb commented Mar 3, 2025

I think this PR now has the right fix.

Next steps are:

  1. wait for the extended tests to complete
  2. Revert the change that triggers extended tests on all PRs
  3. Merge

I'll try and check later tonight but I have to run for a family event

@alamb alamb mentioned this pull request Mar 3, 2025
Closed
@alamb
Copy link
Contributor Author

alamb commented Mar 4, 2025

Here is an example CI run of the extended tests running cleanly:

@alamb alamb marked this pull request as ready for review March 4, 2025 01:04
phillipleblanc
phillipleblanc approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@phillipleblanc phillipleblanc left a comment

Choose a reason for hiding this comment

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

Thanks!

xudong963
xudong963 approved these changes Mar 4, 2025
View reviewed changes
Copy link
Member

@xudong963 xudong963 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you 💯

@jonahgao jonahgao merged commit d083a2f into apache:main Mar 4, 2025
24 checks passed
This was referenced Mar 4, 2025
Merged
Closed
xudong963 pushed a commit that referenced this pull request Mar 4, 2025
@alamb @xudong963
Fix verification script and extended tests due to rustup changes (#…
d5ca830 
…14990)

* Fix rustup toolchain errors

* Use standard builder setup in extended tests

* fix yaml

* sprinkle sudo

* Revert "sprinkle sudo"

This reverts commit 0ed0e0a.

* no contianer

* use rust container

* fix extended jobs

* fix

* Update .github/workflows/extended.yml
@alamb alamb deleted the alamb/fix_rustup_installation branch March 4, 2025 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@xudong963 xudong963 xudong963 approved these changes

+1 more reviewer

@phillipleblanc phillipleblanc phillipleblanc approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

development-process Related to development process of DataFusion

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

error: toolchain '1.85.0-x86_64-unknown-linux-gnu' is not installed on CI / verification script

4 participants

@alamb @phillipleblanc @xudong963 @jonahgao