You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "ongchi (via GitHub)" <gi...@apache.org> on 2023/10/09 08:50:17 UTC
[PR] Make CI fail on any occurrence of rust-tomlfmt failed [arrow-datafusion]
ongchi opened a new pull request, #7774:
URL: https://github.com/apache/arrow-datafusion/pull/7774
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
`ci/scripts/rust_toml_fmt.sh` does not return an error when any occurrence of failed.
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
- Update `cargo-tomlfmt` check script and CI.
- Fix the format of `datafusion/sqllogictest/Cargo.toml`
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CI fail on any occurrence of rust-tomlfmt failed [arrow-datafusion]
Posted by "ongchi (via GitHub)" <gi...@apache.org>.
ongchi commented on code in PR #7774:
URL: https://github.com/apache/arrow-datafusion/pull/7774#discussion_r1358388711
##########
ci/scripts/rust_toml_fmt.sh:
##########
@@ -17,5 +17,11 @@
# specific language governing permissions and limitations
# under the License.
+# 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.
Review Comment:
I'd also add some documentation to the script to guide the user on how to fix the `Cargo.toml` formatting.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CI fail on any occurrence of rust-tomlfmt failed [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7774:
URL: https://github.com/apache/arrow-datafusion/pull/7774
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CI fail on any occurrence of rust-tomlfmt failed [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #7774:
URL: https://github.com/apache/arrow-datafusion/pull/7774#issuecomment-1753894587
I have noticed this error as well -- thank you @ongchi
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CI fail on any occurrence of rust-tomlfmt failed [arrow-datafusion]
Posted by "ongchi (via GitHub)" <gi...@apache.org>.
ongchi commented on code in PR #7774:
URL: https://github.com/apache/arrow-datafusion/pull/7774#discussion_r1358392373
##########
.github/workflows/rust.yml:
##########
@@ -505,12 +505,8 @@ jobs:
- name: Check Cargo.toml formatting
run: |
- # if you encounter error, try rerun the command below, finally run 'git diff' to
- # check which Cargo.toml introduces formatting violation
- #
# ignore ./Cargo.toml because putting workspaces in multi-line lists make it easy to read
Review Comment:
The comments have been restored and updated.
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CI fail on any occurrence of rust-tomlfmt failed [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7774:
URL: https://github.com/apache/arrow-datafusion/pull/7774#discussion_r1358391209
##########
.github/workflows/rust.yml:
##########
@@ -505,12 +505,11 @@ jobs:
- name: Check Cargo.toml formatting
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.
Review Comment:
👍
##########
.github/workflows/rust.yml:
##########
@@ -505,12 +505,11 @@ jobs:
- name: Check Cargo.toml formatting
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.
Review Comment:
👍
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CI fail on any occurrence of rust-tomlfmt failed [arrow-datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7774:
URL: https://github.com/apache/arrow-datafusion/pull/7774#discussion_r1350801847
##########
.github/workflows/rust.yml:
##########
@@ -505,12 +505,8 @@ jobs:
- name: Check Cargo.toml formatting
run: |
- # if you encounter error, try rerun the command below, finally run 'git diff' to
- # check which Cargo.toml introduces formatting violation
- #
# ignore ./Cargo.toml because putting workspaces in multi-line lists make it easy to read
Review Comment:
Can you please leave comments here about how to fix the issue if the CI fails when someone didn't run rust_fmt_toml correctly?
When I purposely added a problem to `Cargo.toml` I got an error that I would find hard to interpret
```shell
alamb@MacBook-Pro-8:~/Software/arrow-datafusion2$ if `./ci/scripts/rust_toml_fmt.sh`; then echo "pass" ; else echo "failed"; fi
....
+ cargo tomlfmt -d -p ./datafusion/sqllogictest/Cargo.toml
WARN 2023-10-09T21:27:06Z: cargo_tomlfmt: dryrun founds problems in Cargo.toml
WARN 2023-10-09T21:27:06Z: cargo_tomlfmt: exit with -1
failed
```
##########
.github/workflows/rust.yml:
##########
@@ -505,12 +505,8 @@ jobs:
- name: Check Cargo.toml formatting
run: |
- # if you encounter error, try rerun the command below, finally run 'git diff' to
- # check which Cargo.toml introduces formatting violation
- #
# ignore ./Cargo.toml because putting workspaces in multi-line lists make it easy to read
Review Comment:
I think the reason CI originally ran`rust_fmt_toml.sh` + `git diff --exit-code` was so that people could run `rust_fmt_toml.sh` locally and check in the result to fix CI. Perhaps we can restore that behavior
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make CI fail on any occurrence of rust-tomlfmt failed [arrow-datafusion]
Posted by "ongchi (via GitHub)" <gi...@apache.org>.
ongchi commented on PR #7774:
URL: https://github.com/apache/arrow-datafusion/pull/7774#issuecomment-1752591806
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! :)
```
--
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@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org