[Merged by Bors] - Silence annoying rustfmt config warnings

[CleanCut]

Silence those annoying rustfmt config warnings that happen because we have unstable rustfmt options in rustfmt.toml, but we run it in stable on CI. Thanks to @Ratysz for calling it out. 😄

The final approach we settled on was to comment out the unstable options in rustfmt.toml. Those who are using nightly may uncomment the unstable options locally if they wish. Once the options stabilize, we can uncomment them again.

We also decided that instead of fixing the alias, we would remove the alias entirely so that we do not introduce a custom .cargo/config.toml that would conflict with users' custom version of the same file. This means that instead of using a cargo ci alias you should use cargo run -p ci or cargo run --package ci instead.

Original Approach (abandoned)

We decided not to go this way...

In my quest to find a portable way to filter out the warnings I switched the library used to execute commands from xshell to duct (as advised by the xshell project itself when you want to do less simple things). This still uses the "xtask" pattern of using a cargo command alias and a rust project for what would have usually been done with a bash script (on posix), just a different helper library is being used internally.

NOTE 1: Also, thanks to some sleuthing by @DJMcNab we were able to fix the broken cargo alias. The issue turned out to be that .cargo/config.toml was being ignored because of .gitignore.

NOTE 2: This is a known breaking change for anyone working on bevy who has their own local .cargo/config.toml.

[DJMcNab]

I'd rather if this just didn't check the formatting at all on ci with a non stable toolchain

Then always ran cargo +nightly fmt --all when run locally.

[CleanCut]

I'd rather if this just didn't check the formatting at all on ci with a non stable toolchain

@DJMcNab Your wish has already been granted! In CI this already only runs on stable Rust. Here's the current config on main:

bevy/.github/workflows/ci.yml

Lines 36 to 39 in bc4fe9b

	- name: Check the format
	# See tools/ci/src/main.rs for the commands this runs
	run: cargo run --package ci
	if: runner.os == 'linux' && matrix.toolchain == 'stable'

Then always ran cargo +nightly fmt --all when run locally.

Opinion noted. This is really up to the user, unfortunately. We don't place requirements on whether the user uses stable or nightly. This means that a local user may see different results than CI if they're using a different version of Rust than CI's current stable. Unfortunately, I think that's just the reality of how things need to be at the moment. I don't see a way to avoid that unless we constrain what version of Rust a user selects. The topic of which Rust to use was explored starting around here: #1309 (comment)

We could add some documentation (a la #1507) stating that you should use the latest stable rust iff you want cargo ci to exactly match CI -- but that may be confusing when contrasted with suggestions to use nightly in other places for best compile times.

[CleanCut]

It occurred to me that .cargo/config.toml was in .gitignore so that we don't stomp users' changes. So, I restored that entry in .gitignore. We'll just need to be careful to add -f or --force to git commands when we, ourselves, modify that file. For example, git add -f .cargo/config.toml.

[DJMcNab]

@DJMcNab Your wish has already been granted! In CI this already only runs on stable Rust.

Except 'stable Rust' is exactly the opposite of 'not stable'

I'm saying we shouldn't be having these warnings, because it's significantly easier just to only run it in ci on nightly

Then we can actually check that the code follows the rustfmt config we have set up

And also mean cargo xtask fmt could be an alias for cargo +nightly fmt

[DJMcNab]

Will force pushing allow our changes to be pulled?

I don't have a good enough git intuition to determine what will happen here

[CleanCut]

@DJMcNab Thank you for clarifying your thoughts. Regarding nightly vs stable, the decision to go ahead with stable was settled on recently in #1309 (comment), so I don't feel like it needs to be rehashed here.

Regarding your Git question, forcing is only necessary to add your changes to the index before you commit. No forcing will be necessary during a push, nor will pulling be affected. In other words, special care only needs to be taken by someone who changes the .cargo/config.toml to make sure that change is added to a local commit if they want the changes to be included in their pull request -- after that step (adding a change to the index to be committed) nothing else is affected by .gitignore. This is already how things were (and what I failed to notice when I created the .cargo/config.toml file earlier).

[DJMcNab]

I'd seen that but hadn't read it fully - in that case I'd argue we just shouldn't have a rustfmt.toml checked in at the root, and instead have it somewhere else. Then cargo xtask fmt would be cargo +nightly fmt -- config-path <path_to_rusfmt_toml>

And the change to ignore warnings in this PR would not be needed

[CleanCut]

I'd seen that but hadn't read it fully - in that case I'd argue we just shouldn't have a rustfmt.toml checked in at the root, and instead have it somewhere else. Then cargo xtask fmt would be cargo +nightly fmt -- config-path <path_to_rusfmt_toml>

AFAIK this would break @cart's desired experience. He uses nightly and wants those options to take effect in his environment.

[DJMcNab]

this would break @cart's desired experience.

I would suggest that in that case he should configure his format on save to be pointed to the new location of rustfmt.toml. When he (anyone) runs the catchup format they would just run cargo xtask fmt to apply it to the whole workspace.

[CleanCut]

this would break @cart's desired experience.

I would suggest that in that case he should configure his format on save to be pointed to the new location of rustfmt.toml. When he (anyone) runs the catchup format they would just run cargo xtask fmt to apply it to the whole workspace.

I'm totally fine doing it whichever way @cart wants. Just let me know!

[cart]

My vote is that we move away from aliases entirely in favor of cargo run --package ci. Its slightly more typing, but it allows us to not stomp on user config and is more self-documenting.

[cart]

And (in a separate pr) I think we should add cargo run --package fmt, which runs stable cargo fmt --all.

Users (like me) that want to use nightly features can just set nightly as default, uncomment the relevant nightly config items, and run cargo run --package fmt

[cart]

Or (alternatively) users can just run cargo fmt --all. Which is much simpler / not custom :)

[CleanCut]

@cart Other than doing it in a separate PR, I'm not sure what you want done with fmt. Currently we have cargo fmt --all -- --check as one of the things that runs inside of the ci package. Do you want that to remain there? Do you want it changed / removed?

[cart]

Yeah sorry I wasn't super clear. I think we should leave ci as it is (check only). I don't think we need any more packages. Users can use the ci package to validate and cargo fmt --all to format.

[CleanCut]

@cart Ready for another review! I updated the issue to reflect the approach we ended up taking. I'll repeat it here for convenience:

The final approach we settled on was to comment out the unstable options in rustfmt.toml. Those who are using nightly may uncomment the unstable options locally if they wish. Once the options stabilize, we can uncomment them again.

We also decided that instead of fixing the alias, we would remove the alias entirely so that we do not introduce a custom .cargo/config.toml that would conflict with users' custom version of the same file. This means that instead of using a cargo ci alias you should use cargo run -p ci or cargo run --package ci instead.

I also updated the documentation on the website in bevyengine/bevy-website#106

[cart]

Ooh i didn't know cargo run -p ci was an option. Lets just recommend that everywhere instead of including both. I'm happy making the "terseness" decision on behalf of users.

Great work here. I think we found an ideal path forward :)

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-android
• build (nightly, ubuntu-latest)
• build (stable, macos-latest)
• build (stable, ubuntu-latest)
• build (stable, windows-latest)
• build-wasm (nightly, ubuntu-latest)
• build-wasm (stable, ubuntu-latest)
• markdownlint

[CleanCut]

Great work here. I think we found an ideal path forward :)

I think everyone will be happy with it. 😄