Remove monorepo #1030
Remove monorepo #1030
Conversation
There was a problem hiding this comment.
I thought mono_repo is how we manage CI workflows in the ecosystem packages, but I just realized that https://github.com/dart-lang/core doesn't use mono_repo. Are we migrating away from mono_repo? Or do I misremember and mono_repo wasn't the standard way of doing this?
I think we should follow the rest of the ecosystem packages. If they're migrating away from mono_repo and/or won't be using mono_repo in the future we should also drop it.
cc @devoncarew who may have opinions on this.
I also just filed #1031, do we want integrate that change into this PR, or do we want to do it separately?
I integrated it.
In the end, it's personal preference - some package maintainers prefer it. It does not have the benefits which led to it's development anymore, features such as running only when certain files are touched is now possible with standard Github features. I personally think it is quite complicated and has little to no benefits, and most dart-lang monorepos (core, tools, labs, native, i18n, ...) we don't use it anymore. |
|
It seems like |
Sorry I also realized this too late -- we need to skip golden tests in the browser as they read files (which we can't do in a browser). I'll figure out how to skip them. In the meantime feel free to revert the commit to test on the browsers, we can cherry-pick it later. |
|
As a drive-by comment, here's where I switched dart-lang/web from mono_repo to straight github workflow files; it also tests across vm and browsers / dart2wasm. dart-lang/web#428 |
|
@mosuem since your branch is on this repo (instead of a fork) I can take this from here if you prefer that or don't have the time. |
| - 'benchmarks/**' | ||
| - 'tool/setup.sh' | ||
| schedule: | ||
| - cron: '0 0 * * 0' # weekly |
There was a problem hiding this comment.
Why do we want to run this periodically?
There was a problem hiding this comment.
Just to check - in case some dependency update brings in a bug or similar. Also it's very low effort.
.github/workflows/protobuf.yaml
Outdated
| - run: dart analyze --fatal-infos | ||
|
|
||
| - run: dart format --output=none --set-exit-if-changed . | ||
| if: ${{matrix.run-tests && (success() || failure()) }} |
There was a problem hiding this comment.
Is matrix.run-tests ever false? It looks hard-coded above.
There was a problem hiding this comment.
run-tests is true only for the stable sdk, so we don't test on dev. Feel free to change that if needed.
There was a problem hiding this comment.
Thanks. I'm not sure why we would want to analyze but not test. I think we should either run all of the steps or none of the steps (i.e. don't test with dev). I'll update.
I don't like that the build can go red on its own (with no changes to the repo) with periodic jobs and trying with new versions of deps, but it's not a new issue in this repo. Probably not a big deal.
There was a problem hiding this comment.
I don't like that the build can go red on its own (with no changes to the repo) with periodic jobs and trying with new versions of deps, but it's not a new issue in this repo. Probably not a big deal.
That's just a problem of packages without pinned dependencies, and it's better to test for it than to have it happen silently.
That has the problem that it sets the check to success, so gives a green checkmark in the UI, regardless of whether any step failed or not.
Yes it does, it runs through all the checks, not stopping when of them fails. |
No description provided.