-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update cli Dockerfile to a newer ubuntu release, newer rust release #10638
Conversation
@@ -15,7 +15,7 @@ | |||
# specific language governing permissions and limitations | |||
# under the License. | |||
|
|||
FROM rust:1.73-bullseye as builder | |||
FROM rust:1.78-bookworm as builder |
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.
The workspace.package in the Cargo.toml
is still 1.73. https://github.com/apache/datafusion/blob/main/Cargo.toml#L53
I am fine with updating the Debian version, but updating the Rust version might require more thoughts or having more extensive implications cc @alamb @andygrove
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.
The Rust version update is only for the build of the CLI. Nothing more. It's not related to the MSRV of DataFusion unless the build of the CLI is the litmus test that the MSRV we have actually builds and runs.
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.
The Rust version update is only for the build of the CLI. Nothing more. It's not related to the MSRV of DataFusion unless the build of the CLI is the litmus test that the MSRV we have actually builds and runs.
I think the MSRV is verified via a CI job:
datafusion/.github/workflows/rust.yml
Lines 556 to 584 in 4b7b5ab
msrv: | |
name: Verify MSRV (Min Supported Rust Version) | |
runs-on: ubuntu-latest | |
container: | |
image: amd64/rust | |
steps: | |
- uses: actions/checkout@v4 | |
- name: Setup Rust toolchain | |
uses: ./.github/actions/setup-builder | |
- name: Install cargo-msrv | |
run: cargo install cargo-msrv | |
- name: Check datafusion | |
working-directory: datafusion/core | |
run: | | |
# If you encounter an error with any of the commands below | |
# it means some crate in your dependency tree has a higher | |
# MSRV (Min Supported Rust Version) than the one specified | |
# in the `rust-version` key of `Cargo.toml`. Check your | |
# dependencies or update the version in `Cargo.toml` | |
cargo msrv verify | |
- name: Check datafusion-substrait | |
working-directory: datafusion/substrait | |
run: cargo msrv verify | |
- name: Check datafusion-proto | |
working-directory: datafusion/proto | |
run: cargo msrv verify | |
- name: Check datafusion-cli | |
working-directory: datafusion-cli | |
run: cargo msrv verify |
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.
Which issue does this PR close?
Closes #10472
Rationale for this change
The current Dockerfile is based on a pretty old ubuntu release which doesn't build when run on a newer ubuntu release. I can see no good reason why the cli should be built using old ubuntu and rust versions.
What changes are included in this PR?
Dockerfile update.
Are these changes tested?
Built and tested locally on wsl2/ubuntyu 22.04.4
Are there any user-facing changes?
No