Comply with TOML string requirements #3097
Conversation
mattschlebusch
force-pushed
the
drop-license-template-fmt
branch
5 times, most recently
from
22e2119 to
466f4e2
Compare
mattschlebusch
added
Status: Awaiting review
tools/devctr/Dockerfile.aarch64
Outdated
| @@ -99,6 +99,8 @@ RUN cd "$TMP_POETRY_DIR" \ | |||
| RUN mkdir "$TMP_BUILD_DIR" \ | |||
| && curl https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal --default-toolchain "$RUST_TOOLCHAIN" \ | |||
| && rustup target add aarch64-unknown-linux-musl \ | |||
| && rustup install nightly \ | |||
There was a problem hiding this comment.
hmm this adds a third Rust runtime to the Docker image
There was a problem hiding this comment.
That's correct. For whatever reason, rust-lang puts a number of the formatter options in the nightly toolchain. Using stable yields errors such as the following:
Warning: can't set `wrap_comments = true`, unstable features are only available in nightly channel.
Warning: can't set `format_code_in_doc_comments = true`, unstable features are only available in nightly channel.
Warning: can't set `comment_width = 100`, unstable features are only available in nightly channel.
Warning: can't set `normalize_comments = true`, unstable features are only available in nightly channel.
Warning: can't set `normalize_doc_attributes = true`, unstable features are only available in nightly channel.
Warning: can't set `format_strings = true`, unstable features are only available in nightly channel.
Warning: can't set `imports_granularity = Module`, unstable features are only available in nightly channel.
Warning: can't set `group_imports = StdExternalCrate`, unstable features are only available in nightly channel.
Drops string handling from pre-commit entirely. rustfmt --config option can't be used with the parameters 'as is' (string values as unquoted). Instead use rustfmt --config-path that takes the whole fmt.toml file. To be able to use --config-path requires the nightly toolchain to be added to the docker dev container as well. This change additionally drops the license-template-path formatting parameter as it is no longer supported by the rust toolchain. See github discussion: rust-lang/rustfmt#3352 Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
mattschlebusch
force-pushed
the
drop-license-template-fmt
branch
2 times, most recently
from
6b4a4fd to
079c7d5
Compare
Signed-off-by: Matthew Schlebusch <schlebus@amazon.com>
mattschlebusch
force-pushed
the
drop-license-template-fmt
branch
from
079c7d5 to
7827f33
Compare
mattschlebusch
added
Status: Author
and removed
Status: Awaiting review
| @@ -108,6 +108,7 @@ RUN mkdir "$TMP_BUILD_DIR" \ | |||
| && curl https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal --default-toolchain "$RUST_TOOLCHAIN" \ | |||
| && rustup target add x86_64-unknown-linux-musl \ | |||
| && rustup component add rustfmt clippy clippy-preview \ | |||
| && rustup install nightly \ | |||
There was a problem hiding this comment.
If we add it, can we do it with --profile minimal?
| # This file can be used as a whole TOML file via rustfmt's --config-path option | ||
| # The individual key-pairs can also used with the rustfmt --config option, | ||
| # but requires removing the quotes to work | ||
|
|
||
| comment_width=100 |
There was a problem hiding this comment.
Maybe add a comment saying all those options are unstable and require nightly channel
| # Check that the output is empty. | ||
| _, stdout, _ = utils.run_cmd(f"cargo fmt --all -- --check --config {config}") | ||
| _, stdout, _ = utils.run_cmd("cargo fmt --all -- --check --config-path fmt.toml") |
There was a problem hiding this comment.
Shouldn't this also need to be run with +nightly?
There was a problem hiding this comment.
This would have, but I have dropped nightly from the toolchain here. This generates warnings, but the test doesn't vet or fail for warnings.
| # Read rustfmt config, replace '\n' with ',' | ||
| rustfmt_config="$(sed -z "s/\n/,/g;s/,$/\n/" ./tests/fmt.toml)" | ||
| # We first do a check run, this will fail when it finds a non-matching license. | ||
| rustfmt $i --check --config $rustfmt_config | ||
| # Run `cargo fmt` for this file | ||
| rustfmt $i --config $rustfmt_config | ||
| rustfmt $i --check --config-path ./tests/fmt.toml | ||
|
|
||
| # Run `cargo fmt` for this source file using the fmt.toml config | ||
| rustfmt $i --config-path ./tests/fmt.toml |
There was a problem hiding this comment.
The primary reason I used this approach was essentially to circumvent needing to include nightly. If you are including nightly it might be best to simply use rustfmt.toml file (although this may require multiple files)(see https://rust-lang.github.io/rustfmt/).
|
There is legitimate reasons around this change but a revision around how we consume the nightly features in the formatter are in order. This isn't urgent and doesn't warrant the time for it at this stage. |
Reason for this Change
The pre-commit script used to manually parse the key-value pairs in the file, which required strings as unquoted when passed in manually to the
rustmt --config ...command. This meant string manipulation when parsing parameters from thetests/fmt.tomlfile. With the--config-pathparameter forrustfmt, it is possible to leave all the string values in the TOML file as quoted.Description of Changes
Remove string handling from code that make use of
fmt.toml. This requires the use of rustfmt's --config-path option to be able to use the string values as quoted as per the TOML standard.Drops the license-template-path
formatting parameter as it is no longer supported by the
rust toolchain. See github discussion:
[unstable option] license_template_path rust-lang/rustfmt#3352
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license.