You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "mbrobbel (via GitHub)" <gi...@apache.org> on 2025/02/04 13:10:29 UTC
[PR] Add `Cargo.lock` [datafusion]
mbrobbel opened a new pull request, #14483:
URL: https://github.com/apache/datafusion/pull/14483
## Which issue does this PR close?
Closes #14135.
## Rationale for this change
See linked issue.
## What changes are included in this PR?
- Remove `Cargo.lock` from `.gitignore`
- Add `--locked` to cargo in CI
- Move `datafusion-cli` back to workspace
- Remove `cli` lock file check in CI
- Add `Cargo.lock` file for `dev/depcheck`
- Remove `cargo update` from release instructions
- Update lock files with `cargo update`
TODO
- [ ] Update dependabot config
## Are these changes tested?
CI.
## Are there any user-facing changes?
Added `Cargo.lock` file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942595648
##########
ci/scripts/rust_clippy.sh:
##########
@@ -18,6 +18,4 @@
# under the License.
set -ex
-cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests -- -D warnings
-cd datafusion-cli
-cargo clippy --all-targets --all-features -- -D warnings
Review Comment:
I agree. We could change this to:
```
cargo clippy --workspace --all-targets --all-features --exclude datafusion-benchmarks -- -Dwarnings
cargo clippy -p datafusion-benchmarks --all-targets -- -Dwarnings
```
What do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942582463
##########
benchmarks/bench.sh:
##########
@@ -34,7 +34,7 @@ COMMAND=
BENCHMARK=all
DATAFUSION_DIR=${DATAFUSION_DIR:-$SCRIPT_DIR/..}
DATA_DIR=${DATA_DIR:-$SCRIPT_DIR/data}
-CARGO_COMMAND=${CARGO_COMMAND:-"cargo run --release"}
+CARGO_COMMAND=${CARGO_COMMAND:-"cargo run --release --locked"}
Review Comment:
either remove --locked or add a code comment why it should be here
##########
ci/scripts/rust_clippy.sh:
##########
@@ -18,6 +18,4 @@
# under the License.
set -ex
-cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests -- -D warnings
-cd datafusion-cli
-cargo clippy --all-targets --all-features -- -D warnings
Review Comment:
Not being able to clippy with `--all-features` is a loss.
I know there are few mutually-excluding features (sigh), but we can do better than clippying 3 features and not clippying all the rest.
Let's follow-up on this.
In a follow-up we should also clippy (or check) with no-default-features.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942624629
##########
.github/workflows/extended.yml:
##########
@@ -87,7 +87,7 @@ jobs:
- name: Run tests
run: |
cd datafusion
- cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests
+ cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked
Review Comment:
Ok, I'll just keep it for https://github.com/apache/datafusion/blob/684c7df6ecf9c7579654b68cb733a943173961e2/.github/workflows/extended.yml#L37 ~which is required to pass before a bunch of additional jobs are started.
Edit: I meant to link: https://github.com/apache/datafusion/blob/684c7df6ecf9c7579654b68cb733a943173961e2/.github/workflows/rust.yml#L51
##########
.github/workflows/extended.yml:
##########
@@ -87,7 +87,7 @@ jobs:
- name: Run tests
run: |
cd datafusion
- cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests
+ cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked
Review Comment:
Ok, I'll just keep it for https://github.com/apache/datafusion/blob/684c7df6ecf9c7579654b68cb733a943173961e2/.github/workflows/extended.yml#L37 which is required to pass before a bunch of additional jobs are started.
Edit: I meant to link: https://github.com/apache/datafusion/blob/684c7df6ecf9c7579654b68cb733a943173961e2/.github/workflows/rust.yml#L51
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1946783959
##########
.github/workflows/rust.yml:
##########
@@ -60,7 +60,10 @@ jobs:
with:
rust-version: stable
- name: Prepare cargo build
- run: cargo check --profile ci --all-targets --features integration-tests
+ # Adding `--locked` here to assert that the `Cargo.lock` file is up to
+ # date with the manifest. When this fails, please make sure to commit
+ # the changes to `Cargo.lock` after building with the updated manifest.
+ run: cargo check --profile ci --workspace --all-targets --features integration-tests --locked
Review Comment:
when written like this the comments don't show up in the CI output (only what is run via `run` is
```suggestion
run: |
# Adding `--locked` here to assert that the `Cargo.lock` file is up to
# date with the manifest. When this fails, please make sure to commit
# the changes to `Cargo.lock` after building with the updated manifest.
cargo check --profile ci --workspace --all-targets --features integration-tests --locked
```
##########
.github/workflows/rust.yml:
##########
@@ -60,7 +60,10 @@ jobs:
with:
rust-version: stable
Review Comment:
this is a nice idea to only use `--locked` in one test 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942581503
##########
README.md:
##########
@@ -152,26 +152,13 @@ deprecate methods before removing them, according to the [deprecation guidelines
[deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html
-## Dependencies and a `Cargo.lock`
+## Dependencies and `Cargo.lock`
-`datafusion` is intended for use as a library and thus purposely does not have a
-`Cargo.lock` file checked in. You can read more about the distinction in the
-[Cargo book].
+Following the [updated guidance] on committing `Cargo.lock` files, this project
Review Comment:
"updated" makes sense from history perspective (eg for someone looking at the PR diff, or project history)
The (future) reader of this page doesn't need to care about what was the previous guidance.
_if we were writing this document anew today_, we would just say "guidance" (or "Rust guidance"). So let's say so.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#issuecomment-2645735179
I merged up from main to resolve a conflict and applied the suggestion. I plan to merge this once we get a clean CI run
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942615076
##########
README.md:
##########
@@ -152,26 +152,13 @@ deprecate methods before removing them, according to the [deprecation guidelines
[deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html
-## Dependencies and a `Cargo.lock`
+## Dependencies and `Cargo.lock`
-`datafusion` is intended for use as a library and thus purposely does not have a
-`Cargo.lock` file checked in. You can read more about the distinction in the
-[Cargo book].
+Following the [updated guidance] on committing `Cargo.lock` files, this project
Review Comment:
[32b2178](https://github.com/apache/datafusion/pull/14483/commits/32b21785018fe18486d829d6fa81e12b69442abd)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#issuecomment-2636298157
@mbrobbel many changes in this PR are about adding `--locked` to various cargo invocations.
I think it should be possible to verify dependencies in one place, and then keep eg running `cargo test` without `--locked`.
The benefit would be not only smaller PR diff, but also easier to comprehend cargo incantation on CI (when a build fails, the command line matters).
If, however, it's necessary to have `--locked` in cargo test, then i would recommend adding a code comment explaining why it's so. It's not obvious, at least to me.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942615574
##########
benchmarks/bench.sh:
##########
@@ -34,7 +34,7 @@ COMMAND=
BENCHMARK=all
DATAFUSION_DIR=${DATAFUSION_DIR:-$SCRIPT_DIR/..}
DATA_DIR=${DATA_DIR:-$SCRIPT_DIR/data}
-CARGO_COMMAND=${CARGO_COMMAND:-"cargo run --release"}
+CARGO_COMMAND=${CARGO_COMMAND:-"cargo run --release --locked"}
Review Comment:
[684c7df](https://github.com/apache/datafusion/pull/14483/commits/684c7df6ecf9c7579654b68cb733a943173961e2)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942726264
##########
.github/workflows/extended.yml:
##########
@@ -87,7 +87,7 @@ jobs:
- name: Run tests
run: |
cd datafusion
- cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests
+ cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked
Review Comment:
thanks!
can you please add a code comment there why `--locked` is useful?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#issuecomment-2640801109
Unless anyone else has comments about this PR I will plan to merge it tomrrow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942627197
##########
ci/scripts/rust_clippy.sh:
##########
@@ -18,6 +18,4 @@
# under the License.
set -ex
-cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests -- -D warnings
-cd datafusion-cli
-cargo clippy --all-targets --all-features -- -D warnings
Review Comment:
The above passes, except for one warning:
```
error: use of a disallowed method `tokio::task::spawn`
--> datafusion/sqllogictest/src/engines/postgres_engine/mod.rs:93:27
```
I think we can just add `#[allow(clippy::disallowed_methods)]` there.
But I agree, let's do this in a follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1941893809
##########
README.md:
##########
@@ -152,26 +152,13 @@ deprecate methods before removing them, according to the [deprecation guidelines
[deprecation guidelines]: https://datafusion.apache.org/library-user-guide/api-health.html
-## Dependencies and a `Cargo.lock`
+## Dependencies and `Cargo.lock`
-`datafusion` is intended for use as a library and thus purposely does not have a
-`Cargo.lock` file checked in. You can read more about the distinction in the
-[Cargo book].
+Following the [updated guidance] on committing `Cargo.lock` files, this project
+commits its `Cargo.lock` file.
-CI tests always run against the latest compatible versions of all dependencies
-(the equivalent of doing `cargo update`), as suggested in the [Cargo CI guide]
-and we rely on Dependabot for other upgrades. This strategy has two problems
-that occasionally arise:
+CI uses the committed `Cargo.lock` file, and dependencies are updated regularly
Review Comment:
❤️
##########
datafusion-cli/Cargo.toml:
##########
@@ -18,23 +18,22 @@
[package]
name = "datafusion-cli"
description = "Command Line Client for DataFusion query engine."
-version = "45.0.0"
-authors = ["Apache DataFusion <de...@datafusion.apache.org>"]
-edition = "2021"
-keywords = ["arrow", "datafusion", "query", "sql"]
-license = "Apache-2.0"
-homepage = "https://datafusion.apache.org"
-repository = "https://github.com/apache/datafusion"
-rust-version = "1.81.0"
readme = "README.md"
+version = { workspace = true }
Review Comment:
this is pretty sweet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942619187
##########
.github/workflows/extended.yml:
##########
@@ -87,7 +87,7 @@ jobs:
- name: Run tests
run: |
cd datafusion
- cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests
+ cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked
Review Comment:
Fail-fast is not unreasonable, i didn't think about this.
otoh one could say same thing about formatting issues. there is definitely tradeoff between conserving CI resources and maxing our single PR feedback signal.
let's put cargo.lock consistency in the "formatting bucket", ie reuse previous design decision for now. i am open to revisiting it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#issuecomment-2640802773
I will leave this open for another day or so for additional comments and then plan to merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#issuecomment-2645809871
Thanks @mbrobbel and @findepi
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942624629
##########
.github/workflows/extended.yml:
##########
@@ -87,7 +87,7 @@ jobs:
- name: Run tests
run: |
cd datafusion
- cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests
+ cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked
Review Comment:
Ok, I'll just keep it for https://github.com/apache/datafusion/blob/684c7df6ecf9c7579654b68cb733a943173961e2/.github/workflows/extended.yml#L37 which is required to pass before a bunch of additional jobs are started.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942615130
##########
ci/scripts/rust_clippy.sh:
##########
@@ -18,6 +18,4 @@
# under the License.
set -ex
-cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests -- -D warnings
-cd datafusion-cli
-cargo clippy --all-targets --all-features -- -D warnings
Review Comment:
i think all-features may be problematic for core, not benchmarks. but if the abve works, great!
let's do this in a follow-up though.
let's focus on getting cargo.lock into the repo, and do non-mandatory clippy improvement on its own focused pr
(review time is non-linear of its size)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942744884
##########
.github/workflows/extended.yml:
##########
@@ -87,7 +87,7 @@ jobs:
- name: Run tests
run: |
cd datafusion
- cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests
+ cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked
Review Comment:
[152f1cf](https://github.com/apache/datafusion/pull/14483/commits/152f1cf8829c110248e27187ed5535c455eee80f)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942577243
##########
.github/workflows/extended.yml:
##########
@@ -87,7 +87,7 @@ jobs:
- name: Run tests
run: |
cd datafusion
- cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests
+ cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked
Review Comment:
cargo test shouldn't need the `--locked` flag (here & other places)
it would suffice to verify cargo lock updatodateness once in single job
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "mbrobbel (via GitHub)" <gi...@apache.org>.
mbrobbel commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942614160
##########
.github/workflows/extended.yml:
##########
@@ -87,7 +87,7 @@ jobs:
- name: Run tests
run: |
cd datafusion
- cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests
+ cargo test --profile ci --exclude datafusion-examples --exclude datafusion-benchmarks --exclude datafusion-sqllogictest --workspace --lib --tests --features=force_hash_collisions,avro,extended_tests --locked
Review Comment:
Yes, I added it everywhere to let all these jobs fail early when the lock file needs to be updated (to save CI resources), because in that case all those jobs are going to have to run again when the lock file is updated. But I'm also fine with only keeping it for one check.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "findepi (via GitHub)" <gi...@apache.org>.
findepi commented on code in PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#discussion_r1942615130
##########
ci/scripts/rust_clippy.sh:
##########
@@ -18,6 +18,4 @@
# under the License.
set -ex
-cargo clippy --all-targets --workspace --features avro,pyarrow,integration-tests -- -D warnings
-cd datafusion-cli
-cargo clippy --all-targets --all-features -- -D warnings
Review Comment:
i think all-features may be problematic for core, not benchmarks. but if the abve works, great!
let's do this in a follow-up though.
let's focus on getting cargo.lock into the repo, and do non-mandatory clippy improvement on its own focused pr
(PR review time is non-linear of its size)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14483:
URL: https://github.com/apache/datafusion/pull/14483#issuecomment-2645809803
🚀 let's give it a try
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] Add `Cargo.lock` [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #14483:
URL: https://github.com/apache/datafusion/pull/14483
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org