Fixes to test framework and GitHub workflow #2
Conversation
After many tries, I think I finally manage to find the right combination of tweaks to make pytest, tox, and GitHub actions to work. This also adds caching of the librocksdb compilation, which is a very slow step, and restricts building of wheels and other artifacts to run only when pushing to main.
| with: | ||
| python-version: ${{ matrix.py_ver }} | ||
| key: ${{ matrix.os }}-librocksdb-${{ matrix.rocksdb_ver }} |
There was a problem hiding this comment.
You would want librocksdb-${{ matrix.os }}-${{ matrix.rocksdb_ver } if the partial restore-keys are to have any effect. But...
.github/workflows/build.yml
Outdated
| git clone https://github.com/facebook/rocksdb && | ||
| cd rocksdb && | ||
| git reset --hard ${{ matrix.rocksdb_ver }} && | ||
| CXXFLAGS='-flto -Os -s' PORTABLE=1 make shared_lib -j 4 && |
There was a problem hiding this comment.
I haven't been able to test yet (I tried #3 for that) but, if there's a partial key match (say, for a different RocksDB version in the same operating system)… I fear the git clone might fail, because the directory eists and is not empty?
Plus, what is the benefit of a partial-key match here? We could reuse the clone, but it'd also have the artifacts of a previous build, and no (visible) clean is being done. So after the reset, make would have to get right to redo all targets, correctly. (Of course that its job, but some dependency might have been missed.)
I think I'd remove partial-key matches completely and, perhaps, use the standard checkout@v2 action to checkout the repository (not sure if that takes a destination_path: /opt). Or, at least, perhaps use --depth 0 --branch ${{ rocksdb_ver }}?
There was a problem hiding this comment.
You are right, I completely missed this. I checked, and the checkout action does not let you specify a path outside of the workdir, so I kept the manual clone, but changed it to avoid reusing a cache from a different version.
NightTsarina
force-pushed
the
fix-github-actions
branch
3 times, most recently
from
8cd12a3 to
e1c5134
Compare
Following suggestions from code review, this commit avoid re-using the working directory from different versions of the library.
NightTsarina
force-pushed
the
fix-github-actions
branch
from
e1c5134 to
2f942de
Compare
|
Comments addressed, enabled building and testing for multiple rocksdb and python versions, and all still works! PTAL? |
| on: ['push', 'pull_request'] | ||
| on: | ||
| push: |
There was a problem hiding this comment.
@NightTsarina I missed the implications of this change. I think we might want to include pull_request here (I'll submit a PR).
Rationale: we have in the past set it to only push in repos in which we only create PRs out of branches in the same repo (not forks). This way CI for push was shared for PR, and there were no duplicate checks. But here, with forks as in #4, the checks don't run in this repo / don't show on the PR itself.
Unless you prefer some other configuration, I'll prepare a configuration with these semantics:
on:
- pull_request
- push
- branches: main
There was a problem hiding this comment.
Ah, I haven't thought of that, thanks!
After many tries, I think I finally manage to find the right combination of
tweaks to make pytest, tox, and GitHub actions to work.
This also adds caching of the librocksdb compilation, which is a very slow
step, and splits the building of wheels and other artifacts to run them only when
pushing to main.
CC: @dato who has more experience with this than I.
CC2: Debugging this is HELL, it is turtles all the way down!