Added warning on CHANGELOG.md & rust-toolchain.toml
#413
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,17 +9,6 @@ on: | |
| types: [opened, reopened, synchronize] | ||
|
|
||
| jobs: | ||
| rustfmt: | ||
| name: rustfmt check nightly on ubuntu-latest | ||
| runs-on: ubuntu-latest | ||
|
|
@@ -40,4 +29,44 @@ jobs: | |
| run: | | ||
| rustup update --no-self-update nightly | ||
| rustup +nightly component add clippy | ||
| make clippy | ||
|
|
||
| doc: | ||
| name: doc stable on ubuntu-latest | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@main | ||
| - name: Build docs | ||
| run: | | ||
| rustup update --no-self-update | ||
| make doc | ||
|
|
||
| version: | ||
| 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 | ||
|
|
||
| changelog: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout code | ||
| uses: actions/checkout@main | ||
| with: | ||
| fetch-depth: 0 | ||
| - name: Check if CHANGELOG.md is modified | ||
| run: | | ||
| # Get the list of changed files in the PR | ||
| changed_files=$(git diff --name-only origin/${{ github.event.pull_request.base.ref }}...${{ github.sha }}) | ||
| # Check if CHANGELOG.md is in the list of changed files | ||
| if echo "$changed_files" | grep -q '^CHANGELOG.md$'; then | ||
| echo "CHANGELOG.md has been modified." | ||
| 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.' | ||
|
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 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 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 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 And not having to depend on not widely used actions as @bobbinth mentioned |
||
| exit 1 | ||
bobbinth marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| fi | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we are at it, let's also change
rust-toolchaintorust-toolchain.tomland update thecheck-rust-version.shscript accordingly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
lockfile committed should be more than enough when using stable compiler.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).