ci: Cleanup macOS runs #17176
ci: Cleanup macOS runs #17176
Conversation
There was a problem hiding this comment.
Code review ACK fadccb2. But since this change reverts #13728, and I'm not sure what the specific goals of #13728 were, I guess I'd only give an uncertain +0.5 concept ACK.
For the specific scripted diff problem I ran into #13728 (comment), other solutions may make it possible to restore #13728 in the future like using homebrew to install modern versions of sed and bash, or just skipping the scripted-diff lint check on macos but running all the other lint checks.
Can be reviewed with git diff --ignore-all-space --function-context
I assumed it was a low-risk change that would provide us with some experience of macOS on travis (to help go forward with #16597). It also had several ACKs #13728 (comment), #13728 (review) |
|
Also, now that #16597 is merged, we should be more cautious with using too much travis resources. On weekdays a backlog of macOS builds up here: https://www.traviscistatus.com/ |
|
There is a backlog of about 400 jobs on macOS travis right now (see the link in my previous comment). I don't think it is worth it to use those resources for linters, when mac users can simply not run the linters (they are run on travis as the fist step, real quick) or install GNU sed via brew or run them in a bionic docker container. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Instead of the linter, I'd rather have a depends built on macOS. I'll leave that for a follow up pull request. |
|
The linter isn't particularly stable, it seems: #17183 (comment) Any objections to getting this merged. @Sjors ? |
|
Why not revert the merge commit to make it clearer what is reversion and what is clean-up / clarify authorship? |
|
It is a revert of the merge commit (with conflicts solved) and the two commented out lines removed |
|
Just suggesting that what you report would be more explicitly and succinctly communicated by the changelog if you separated those two operations. May be obviated by the conflicts though, I’m not sure about that. |
|
Code review ACK fa677d1 Disabling the linter probably deserves its own commit / a more clear commit name. |
|
ACK fa677d1 |
fa677d1 ci: Remove redundant check for TRAVIS_OS_NAME (MarcoFalke) fadccb2 doc: Document that GNU tools are required for linters (MarcoFalke) 4444704 ci: Cleanup macOS runs (MarcoFalke) Pull request description: * Remove a commented out cleanup task in `before_cache` * Remove the linter run on macOS, and document that GNU tools are required to run the linters ACKs for top commit: Sjors: Code review ACK fa677d1 laanwj: ACK fa677d1 ryanofsky: Code review ACK fa677d1 for new third commit replacing TRAVIS_OS_NAME check with NO_DEPENDS setting Tree-SHA512: 9122a63bbe7887d9e379123152ea4ba44324cb18033b9e6b45bfdb1af665c10ea598564b9fcd57330d208a08e4696e41b4d6175f05f0843a3a76530da114f8c6
before_cache