Test on CI that update-meson is properly ran #39527
Conversation
user202729
force-pushed
the
check-update-meson
branch
2 times, most recently
from
74afe78 to
627262f
Compare
|
Documentation preview for this PR (built with commit 6bf8c80; changes) is ready! 🎉 |
user202729
force-pushed
the
check-update-meson
branch
from
d74473a to
82a439a
Compare
@tobiasdiez tried this once and it launched a months-long battle. It affects the sagemath-{objects,categories,etc} namespace packages. Though at this point they are unmaintained, nobody should be using them, and we could be more pragmatic about fixing whatever goes wrong. |
.github/workflows/ci-meson.yml
Outdated
| if: ${{ steps.check_update_meson.outcome == 'failure' }} | ||
| shell: bash -l {0} | ||
| run: | | ||
| git status |
There was a problem hiding this comment.
I think this can be combined with the previous step.
There was a problem hiding this comment.
I can't find some other good way to ensure simultaneously
- test step is not run when build step (or some previous step) fails
- test step is still run when update-meson step fails
- update-meson is ran reasonably early so that in principle someone want to check the failure of this early can do so (though if it's after build step it is kind of moot anyway)
also the purpose of git diff is to print out the diff, there's no need to print out the diff when there's no change.
There was a problem hiding this comment.
Alternatively we could just remove the whole continue-on-error and let the test not run when update-meson is not ran (which might not be that bad of an idea? If pull request author is quickly notified then they can quickly fix the thing and repush)
There was a problem hiding this comment.
Alternatively we could just remove the whole continue-on-error and let the test not run when update-meson is not ran (which might not be that bad of an idea? If pull request author is quickly notified then they can quickly fix the thing and repush)
I would prefer this yes. It's the simplest method to maintain and also the one with the least surprising outcomes (if you forget to add a python file in the meson build, then you almost surely get 'confusing' test failures). Alternatively, you could argue that it's a kind of "linter"/checker and thus should be moved to the Lint workflow.
.github/workflows/ci-meson.yml
Outdated
| run: | | ||
| # Please see step 'Show files changed by update-meson' above and apply the changes, | ||
| # or run tools/update-meson.py locally | ||
| false |
There was a problem hiding this comment.
I don't quite understand the purpose of this check. Why would we want to have two steps that fail due to one reason?
Fixes sagemath#39172 Side note, can we just include the `__init__.py` to be tracked by git? ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> - [x] The title is concise and informative. - [x] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [x] I have created tests covering the changes. (not applicable) - [x] I have updated the documentation and checked the documentation preview. (no documentation change? Do we need to mention this in developer guide somewhere?) ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on. For example, --> <!-- - sagemath#12345: short description why this is a dependency --> <!-- - sagemath#34567: ... --> URL: sagemath#39527 Reported by: user202729 Reviewer(s): Tobias Diez, user202729
Fixes #39172
Side note, can we just include the
__init__.pyto be tracked by git?📝 Checklist
⌛ Dependencies