Various CI fixes and cleanup #8289
Conversation
|
Can one of the admins verify this patch? |
mehrdadn
force-pushed
the
ci-cleanup
branch
29 times, most recently
from
d40a97a to
88eaa1f
Compare
| @@ -0,0 +1,49 @@ | |||
| diff --git deps/jemalloc/configure deps/jemalloc/configure | |||
There was a problem hiding this comment.
what happen when redis update and this patch break?
There was a problem hiding this comment.
the question is more of "how likely will that happen"
There was a problem hiding this comment.
Good question! I think fixing it in the event of a breakage shouldn't need more than 5 minutes—it's just a matter of copy/pasting a few changed lines.
Note that we don't just update Redis randomly. We have much more difficult patches to take care of for Windows compatibility and such (see the existing patches). So I think the overhead is negligible. And it's also a really significant gain in terms of how much noise it cuts down on in the builds (that itself wastes a lot of time). And in any case this patch is trivial enough to comment out if it somehow becomes a problem.
ci/travis/install-dependencies.sh
Outdated
| @@ -60,7 +60,7 @@ install_miniconda() { | |||
| ;; | |||
| esac | |||
| reload_env | |||
| if [ -n "${PYTHON-}" ]; then | |||
| if [ -n "${PYTHON-}" ] && [ "${PYTHON}" != "$(python -s -c "import sys; sys.stdout.write('.'.join(map(str, sys.version_info[:2])))")" ]; then | |||
There was a problem hiding this comment.
Oh this line already changed in a subsequent commit—I think it's a bit clearer there, let me know if it still needs changing?
|
Test PASSed. |
| GITHUB_BASE_SHA: ${{ github.event.pull_request.base.sha }} | ||
| GITHUB_HEAD_SHA: ${{ github.event.pull_request.head.sha }} | ||
| GITHUB_PULL_REQUEST: ${{ github.event.number }} | ||
|
|
There was a problem hiding this comment.
Can you add a comment saying where these are used?
There was a problem hiding this comment.
Yup, they're used for translating Travis environment variables, I'll update them.
.travis.yml
Outdated
| - . ./ci/travis/ci.sh build | ||
| script: | ||
| - . ./ci/travis/ci.sh lint | ||
| - true |
There was a problem hiding this comment.
if we are just skipping, we can just remove the script block right?
There was a problem hiding this comment.
Sadly not :( if you do that then it falls back to the default outermost block. I think I can technically put script: [] but it's not clear to me why that works, it's kind of confusing.
There was a problem hiding this comment.
I see. Add a comment so people maintaining this in the future knows?
ci/travis/ci.sh
Outdated
| if [ -z "${TRAVIS_BRANCH-}" ] && [ -n "${GITHUB_WORKFLOW-}" ]; then | ||
| TRAVIS_BRANCH="${GITHUB_BASE_REF:-${GITHUB_REF}}" | ||
| TRAVIS_BRANCH="${TRAVIS_BRANCH#refs/heads/}" | ||
| TRAVIS_BRANCH="${GITHUB_HEAD_SHA:-${TRAVIS_BRANCH}}" # Need a hash on GitHub Actions when possible (even though on Travis it's always a name) |
There was a problem hiding this comment.
this comment doesn't say much, can we explain more?
also can you explain does this block do in the comment?
| assert ("python" in process or "ray" in process | ||
| or "travis" in process) | ||
| assert ("python" in process or "conda" in process | ||
| or "travis" in process or "runner" in process |
There was a problem hiding this comment.
I think travis was meant to refer to the username on Travis, so I added runner, which is the username on GitHub Actions.
|
Test PASSed. |
Why are these changes needed?
Addresses various CI issues.
Related issue number
#631
Checks
scripts/format.shto lint the changes in this PR.