Skip to content

Create action to install linter#380

Merged
BD103 merged 14 commits intomainfrom
linter-action
Apr 25, 2025
Merged

Create action to install linter#380
BD103 merged 14 commits intomainfrom
linter-action

Conversation

@BD103
Copy link
Copy Markdown
Member

@BD103 BD103 commented Apr 22, 2025

This PR adds support for installing bevy_lint in CI. (Motivated by TheBevyFlock/bevy_new_2d#365, caused dtolnay/rust-toolchain#144.)

- name: Install `bevy_lint`
  uses: TheBevyFlock/bevy_cli/bevy_lint@main

- name: Run `bevy_lint`
  run: bevy_lint --workspace

In the process, I also wrote a workflow to test the action. I also plan on updating the docs and contributing guide for this feature.

@BD103 BD103 added A-Build-System Related to CI and GitHub Actions A-Linter Related to the linter and custom lints C-Feature Make something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 22, 2025
@BD103 BD103 marked this pull request as ready for review April 22, 2025 14:54
@BD103 BD103 added S-Needs-Review The PR needs to be reviewed before it can be merged and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 22, 2025
@BD103 BD103 requested a review from TimJentzsch April 22, 2025 14:55
@BD103 BD103 added this to the `bevy_lint` v0.3.0 milestone Apr 23, 2025
using: composite
steps:
- name: Install Rust toolchain and components
uses: dtolnay/rust-toolchain@master
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if the user's workflow already includes this step beforehand? Nothing or does it break?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It installs this toolchain alongside the one they previously installed. It also runs rustup default, which is unfortunately not something that can be disabled, so the new nightly-2025-XX-XX toolchain will be made the default.

will be available through the `PATH` by default.

Note that this currently will override the default Rustup toolchain to nightly. You can fix this
by calling `rustup default` after this action is run.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we integrate rustup default directly in this script? Or are there scenarios where you don't want to use it?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need rustup default, since we always manually specify which toolchain should be used. Although we could use it to revert this behavior...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to make the action restore the original default toolchain, but my first attempt didn't work. I'm going to leave this to a follow-up issue.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered not using the action, instead requiring that rustup and cargo are already installed and then manually installing the required toolchain via the rustup command?

But that can indeed be done in a follow-up :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! That does sound a lot nicer, although it would mean ditching dtolnay/rust-toolchain since it's just not configurable enough to install Rustup without a specific toolchain. I think it's worth considering, but definitely later

@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review The PR needs to be reviewed before it can be merged labels Apr 24, 2025
@BD103 BD103 added S-Needs-Review The PR needs to be reviewed before it can be merged and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Apr 24, 2025
@BD103 BD103 mentioned this pull request Apr 24, 2025
3 tasks
Copy link
Copy Markdown
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already good for the first iteration, we can refine it further as we go :)

will be available through the `PATH` by default.

Note that this currently will override the default Rustup toolchain to nightly. You can fix this
by calling `rustup default` after this action is run.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you considered not using the action, instead requiring that rustup and cargo are already installed and then manually installing the required toolchain via the rustup command?

But that can indeed be done in a follow-up :)

@BD103 BD103 enabled auto-merge (squash) April 25, 2025 01:14
@BD103 BD103 merged commit 77a75a8 into main Apr 25, 2025
13 checks passed
@BD103 BD103 deleted the linter-action branch April 25, 2025 01:27
BD103 added a commit that referenced this pull request Apr 30, 2025
This PR prepares `bevy_lint` for v0.3.0, following the [release
guide](https://github.com/TheBevyFlock/bevy_cli/blob/e85154b9bd5851a0dcac3815853ec8194d362fb3/bevy_lint/docs/how-to/release.md).
Once this is marked ready for review, this will need at least one
approval from a Bevy maintainer and no outstanding concerns. Once this
is merged, a feature freeze will be enacted until the [post-release PR
is
merged](https://github.com/TheBevyFlock/bevy_cli/blob/e85154b9bd5851a0dcac3815853ec8194d362fb3/bevy_lint/docs/how-to/release.md#post-release).

## Blockers

- [x] #380
- [x] #389
- [x] #391

## For Reviewers

Now is your chance to look through the
[docs](https://thebevyflock.github.io/bevy_cli/bevy_lint/index.html),
the changelog, and the migration guide to ensure everything is in
working order. If you wish to test the linter, you may install it from
this branch:

```sh
rustup toolchain install nightly-2025-04-03 \
    --component rustc-dev \
    --component llvm-tools-preview

rustup run nightly-2025-04-03 cargo install \
    --git https://github.com/TheBevyFlock/bevy_cli.git \
    --branch linter-v0.3.0 \
    --locked \
    bevy_lint
```

You're encouraged to run the linter on several projects to ensure it
works correctly. Some good places to start include:

- [The Engine](https://github.com/bevyengine/bevy)
- [`bevy_new_2d`](https://github.com/TheBevyFlock/bevy_new_2d)
- [The Official
Template](https://github.com/bevyengine/bevy_github_ci_template)

Thank you for all of your help!

## Next Steps

- Release on Github
- Announce the release on Discord, Mastodon, and other social medias
- Potentially publish an announcement [blog
post](https://bd103.github.io/)
- TheBevyFlock/bevy_new_2d#365

---------

Co-authored-by: DAA <42379074+DaAlbrecht@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Build-System Related to CI and GitHub Actions A-Linter Related to the linter and custom lints C-Feature Make something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review The PR needs to be reviewed before it can be merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants