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

Update cli Dockerfile to a newer ubuntu release, newer rust release #10638

Merged
merged 1 commit into from
May 24, 2024

Conversation

Omega359
Copy link
Contributor

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

@Omega359 Omega359 closed this May 23, 2024
@Omega359 Omega359 reopened this May 23, 2024
@Omega359 Omega359 marked this pull request as ready for review May 23, 2024 19:17
@@ -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
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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:

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

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good to me -- thanks @Omega359 and @edmondop

I tested locally that

docker build -f datafusion-cli/Dockerfile . --tag datafusion-cli

works and this looks good.

THanks again

@alamb alamb merged commit 3e4e09a into apache:main May 24, 2024
44 of 45 checks passed
jayzhan211 pushed a commit to jayzhan211/datafusion that referenced this pull request May 26, 2024
findepi pushed a commit to findepi/datafusion that referenced this pull request Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker CLI build fails in WSL2 - "Ubuntu 22.04.4 LTS"
3 participants