Make CI fail on any occurrence of rust-tomlfmt failed #7774

ongchi · 2023-10-09T08:50:15Z

Which issue does this PR close?

Closes #.

Rationale for this change

ci/scripts/rust_toml_fmt.sh does not return an error when any occurrence of failed.

What changes are included in this PR?

Update cargo-tomlfmt check script and CI.
Fix the format of datafusion/sqllogictest/Cargo.toml

Are these changes tested?

Are there any user-facing changes?

ongchi · 2023-10-09T08:55:49Z

There is an error when running ci/scripts/rust_toml_fmt.sh

+ find . -mindepth 2 -name Cargo.toml -exec cargo tomlfmt -p '{}' ';'
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
Error: TomlError { message: "TOML parse error at line 19, column 8\n   |\n19 | authors.workspace = true\n   |        ^\nUnexpected `.`\nExpected `=`\n" }
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T08:52:55Z: cargo_tomlfmt: no problem found. good job! :)

alamb · 2023-10-09T21:19:49Z

I have noticed this error as well -- thank you @ongchi

alamb

Thank you @ongchi -- this is super helpful 🙏

The only change I think this PR needs is a comment about how to fix the CI if it fails (i left more detailed comments on this).

I also tested locally and it seems to have worked great

On main:

alamb@MacBook-Pro-8:~/Software/arrow-datafusion2$ if `./ci/scripts/rust_toml_fmt.sh`; then echo "pass" ; else  echo "failed"; fi
+ find . -mindepth 2 -name Cargo.toml -exec cargo tomlfmt -p '{}' ';'
 INFO 2023-10-09T21:23:33Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:33Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:34Z: cargo_tomlfmt: no problem found. good job! :)
Error: TomlError { message: "TOML parse error at line 19, column 8\n   |\n19 | authors.workspace = true\n   |        ^\nUnexpected `.`\nExpected `=`\n" }
 INFO 2023-10-09T21:23:35Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:35Z: cargo_tomlfmt: no problem found. good job! :)
 INFO 2023-10-09T21:23:35Z: cargo_tomlfmt: no problem found. good job! :)
pass

on this branch (without the toml fix) it fails (which is good)

alamb@MacBook-Pro-8:~/Software/arrow-datafusion2$ if `./ci/scripts/rust_toml_fmt.sh`; then echo "pass" ; else  echo "failed"; fi
++ find . -mindepth 2 -name Cargo.toml
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./test-utils/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion-cli/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/physical-plan/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/core/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/physical-expr/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/proto/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/proto/gen/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/optimizer/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/expr/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/substrait/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/common/Cargo.toml
 INFO 2023-10-09T21:25:19Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/wasmtest/Cargo.toml
 INFO 2023-10-09T21:25:20Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/execution/Cargo.toml
 INFO 2023-10-09T21:25:20Z: cargo_tomlfmt: no problem found. good job! :)
+ for toml in $(find . -mindepth 2 -name 'Cargo.toml')
+ cargo tomlfmt -d -p ./datafusion/sqllogictest/Cargo.toml
Error: TomlError { message: "TOML parse error at line 19, column 8\n   |\n19 | authors.workspace = true\n   |        ^\nUnexpected `.`\nExpected `=`\n" }
failed

.github/workflows/rust.yml

ongchi · 2023-10-13T15:06:31Z

ci/scripts/rust_toml_fmt.sh

+# Run cargo-tomlfmt with flag `-d` in dry run to check formatting
+# without overwritng the file. If any error occur, you may want to
+# rerun 'cargo tomlfmt -p path/to/Cargo.toml' without '-d' to fix
+# the formatting automatically.


I'd also add some documentation to the script to guide the user on how to fix the Cargo.toml formatting.

alamb

Thank you @ongchi

alamb · 2023-10-13T15:08:46Z

.github/workflows/rust.yml

        run: |
-          # if you encounter error, try rerun the command below, finally run 'git diff' to
-          # check which Cargo.toml introduces formatting violation
+          # if you encounter an error, try running 'cargo tomlfmt -p path/to/Cargo.toml' to fix the formatting automatically.


github-actions bot added the sqllogictest SQL Logic Tests (.slt) label Oct 9, 2023

alamb reviewed Oct 9, 2023

View reviewed changes

.github/workflows/rust.yml Show resolved Hide resolved

ongchi added 2 commits October 13, 2023 23:02

Make CI fail on any occurrence of rust-tomlfmt failed

6f1feed

Fix the format of datafusion/sqllogictest/Cargo.toml

5b0ee40

ongchi force-pushed the fail-on-cargo-toml-failed branch from a5e5c93 to 5b0ee40 Compare October 13, 2023 15:03

ongchi commented Oct 13, 2023

View reviewed changes

alamb approved these changes Oct 13, 2023

View reviewed changes

alamb merged commit e0fa75f into apache:main Oct 13, 2023

ongchi deleted the fail-on-cargo-toml-failed branch October 16, 2023 12:30

matthewgapp mentioned this pull request Jan 11, 2024

matt/feat/recursive ctes/config flag matthewgapp/arrow-datafusion#3

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Make CI fail on any occurrence of rust-tomlfmt failed #7774

Make CI fail on any occurrence of rust-tomlfmt failed #7774

Uh oh!

ongchi commented Oct 9, 2023

Uh oh!

ongchi commented Oct 9, 2023

Uh oh!

alamb commented Oct 9, 2023

Uh oh!

alamb left a comment

Uh oh!

Uh oh!

ongchi Oct 13, 2023

Uh oh!

alamb left a comment

Uh oh!

alamb Oct 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Make CI fail on any occurrence of rust-tomlfmt failed #7774

Make CI fail on any occurrence of rust-tomlfmt failed #7774

Uh oh!

Conversation

ongchi commented Oct 9, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

ongchi commented Oct 9, 2023

Uh oh!

alamb commented Oct 9, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ongchi Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Oct 13, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants