Added warning on CHANGELOG.md & rust-toolchain.toml
#413
Conversation
| name: check rust version consistency | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@main | ||
| with: | ||
| profile: minimal | ||
| override: true | ||
| - name: check rust versions | ||
| run: ./scripts/check-rust-version.sh |
There was a problem hiding this comment.
While we are at it, let's also change rust-toolchain to rust-toolchain.toml and update the check-rust-version.sh script accordingly.
There was a problem hiding this comment.
I don't understand why we're restricting the current toolchain to the MSRV of the workspace. I've read through #267, #272 and the related miden-vm PRs and issues, and discord.
From what I can tell its issues due to using the nightly compiler -- which afaik we're not using? Having the lock file committed should be more than enough when using stable compiler.
There was a problem hiding this comment.
I think the primary motivation is that everyone is using the same toolchain as we've had issues when things suddenly break if a different version is used. As an example, in facebook/winterfell#277 code that worked fine prior to 1.78 started failing with 1.78 onwards (this was due to the bug in the code, but still).
Another example is something like 0xPolygonMiden/miden-vm#917, where nothing broke but there was a significant performance degradation when moving from 1.69 to 1.70 (which fixed itself after 1.74).
.github/workflows/lint.yml
Outdated
| else | ||
| echo $'::warning file=CHANGELOG.md::CHANGELOG.md has not been modified.\n This warning can be ignored if is has been explicitely decided not to log changes.\n Except in this situation, make sure to add log changes.' | ||
| exit 1 |
There was a problem hiding this comment.
A downside to this is that this marks the overall CI run as a failure; somewhat destroying the glance value. If we think it will be rare to have a no-op changelog PR then this is probably fine.
Did you look into existing github actions we could use? e.g. this one allows you to skip the check by adding a no changelog label to the PR. I haven't actually audited the action itself though.
There was a problem hiding this comment.
I'm a bit hesitant to use actions that are not very popular - but skipping changelog check by adding no changelog label seems cool. Is this something that can be fairly easily emulated locally?
There was a problem hiding this comment.
Fair enough; I believe we should be able to simply copy the actions code. Its very short, label is added as an env variable and then simply used in an if then within the script.
env:
NO_CHANGELOG_LABEL: ${{ contains(github.event.pull_request.labels.*.name, 'no changelog') }}
There was a problem hiding this comment.
In this PR I inspired myself with the action code and merged it with our existing one, enabling us to get the no changelog label feature: 0xPolygonMiden/miden-vm#1406
And not having to depend on not widely used actions as @bobbinth mentioned
There was a problem hiding this comment.
Some questions, but the addition itself seems fine (modulo @bobbinth's comments).
I would also consider using dtolnay's toolchain action but that can be a separate issue.
^^ already been discussed :)
We've had this before, I believe, but decided to switch to the methodology used by rust-analyzer. |
There was a problem hiding this comment.
Looks good! Thank you! Let's do the following and then merge:
- Confirm that Add
changelog.ymlfile miden-vm#1406 worked and apply the same changes here. - Update
rust-toolchainto be a TOML file.
phklive
changed the title
CHANGELOG.md modificationCHANGELOG.md & rust-toolchain.toml
|
@bobbinth what is the toolchain that we want to use for our repos? |
There was a problem hiding this comment.
Looks good! Thank you!
@bobbinth what is the toolchain that we want to use for our repos?
Let's use the one from the miden-vm repo.
In this PR I propose to add a warning in the CI if the
CHANGELOG.mdfile has not been modified.