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
ARROW-3868: [Rust] Switch to nightly Rust for required build, stable is now allowed to fail #3034
Conversation
@andygrove changes will be needed to windows CI also, right? |
@paddyhoran @sunchao This is ready for review again. The build failed on the R build but the Rust one passed. |
Codecov Report
@@ Coverage Diff @@
## master #3034 +/- ##
=========================================
- Coverage 87.07% 87% -0.08%
=========================================
Files 496 496
Lines 70600 70602 +2
=========================================
- Hits 61477 61427 -50
- Misses 9024 9074 +50
- Partials 99 101 +2
Continue to review full report at Codecov.
|
ci/travis_script_rust_stable.sh
Outdated
|
||
# raises on any formatting errors | ||
#rustup component add rustfmt-preview | ||
#cargo fmt --all -- --check |
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.
Instead of having a separate file, can we use env variable such as RUSTUP_TOOLCHAIN
to check this?
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.
Done
ci/rust-build-main.bat
Outdated
@echo =================================== | ||
|
||
rustup default nightly | ||
rustup default stable |
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.
Curious: will the build fail if it fails to build in stable? how is this done?
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.
I think it will right now (failure in stable will fail the build). @andygrove I don't think you needed to add the || exit /B
below. This is the mechanism that fails the build AFAIK. Actually, I think we need to add the || exit /B
to each of the examples above so that the build fails if they fail to run.
@kszucs could you comment on this?
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.
I have addressed this now. I had originally swapped the order of stable/nightly and that made the PR confusing to review. I added the || exit /B
to the examples so it is consistent.
No description provided.