Test in Docker with Alpine Linux on CI#1826
Merged
Byron merged 10 commits intogitpython-developers:mainfrom Feb 16, 2024
Merged
Test in Docker with Alpine Linux on CI#1826Byron merged 10 commits intogitpython-developers:mainfrom
Byron merged 10 commits intogitpython-developers:mainfrom
Conversation
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
As suggested in #1745 (comment), this adds a test job that runs the tests inside a Docker container.
This can be done with Ubuntu in the container, but instead I used Alpine Linux in the container. This provides a greater variety of platforms GitPython tests on, potentially uncovering some bugs that would otherwise go undetected. In particular, Alpine Linux does not satisfy some of the assumptions that are sometimes made about platforms, as it does not, by default, have
bash; itspsand related tools are not procps; it does not have GNU coreutils or GNU libc; and so forth.No tests had to be marked
xfailfor this. In particular, as noted in #1745 (comment), we have no tests for #1756, so thatpsis not procps in Alpine Linux does not currently result in any failures (and if there were such a test then the existing macOS jobs would still be sufficient for that).This adds only one test job, testing only one version of Python. Currently that version is Python 3.11, since the Alpine Linux
python3apk package provides 3.11 at this time. The specified Docker image isalpine:latest, and the version will presumably advance to Python 3.12 eventually. I added only one job for two reasons:I was able to use the
containerkey to do this, so that the workflow is still expressed in separate steps in the same style as the other test workflows, such that the operations that are analogous are represented by steps written in the same (or very similar) way across all workflows. I think the benefit of readability and comparability justifies this at this time, but depending what is done in the future, it might end up being changed to pack most of the work into a smaller number of steps (maybe one) that useuses: docker://..., orrun: docker run ...ordocker-run-action, or something else.Specific reasons for existing design and implementation decisions, including for the strange and somewhat hacky
sudousage, are detailed in commit messages.I've proposed this job as its own pull request, rather than waiting and attempting to introduce it with others, because I think this makes sense to review by itself (and I can incorporate any feedback into future changes), and because if I understand #1745 (comment) correctly it seems to be valued as a feature by itself (which I do agree with). I think that even if there are to be more Docker test jobs, they are better added in a subsequent PR.
However, I'm even more motivated by a goal that this PR does not itself achieve: testing on a bigendian architecture. Because GitPython uses
gitfor most of its work, it shouldn't have a problem with that, but maybe some parts ofgitdb/smmapthat GitPython uses (for which GitPython's use is under test), or more importantly some seemingly innocuous aspects of encoding and decoding, might inadvertently assume little-endianness.The good news is that no such bugs appear to be present. I have not yet gotten this to work on CI, but on my local amd64 system I am able to enable emulation and run s390x images. This is with no hardware acceleration, and some of the performance tests are too slow for me even to have waited for them to finish (which does not mean they would have a problem on an actual s390x machine). But when I tell
pytestto deselect the performance tests, the remaining tests run in a reasonable time and all pass.I'd like to do that on CI, which is not hard to do. If possible I'd like to use a workflow of this form, with the
containerkey, and have it be a matrix job with both architectures, with special logic for enabling emulation and for skipping the performance tests for non-amd64 architectures (which would, initially and also probably forever, be just the one other architecture, s390x). I haven't managed to get it working that way, though. Usingcontainer:, the container is supposed to be able to start before any steps run. Even if I can manage to get emulation enabled without being able to easily run commands directly on the runner, I suspect I can't get that to happen early enough that usingcontainer:is both effective and reasonable.If Docker test jobs for both amd64 and s390x architectures are to exist, I would want both to be written the same way, even if it makes the amd64 one less nice than in this PR. That way, it would be easier to compare their output to troubleshoot any future failures (as well as to understand how they are set up). If you have thoughts on whether or not this could be valuable, and also how much of a problem (if any) it would be to end up later converting the workflow to look more like that with fewer steps and not using
container:, then that can guide me. However, whether or not you have guidance about that, I think this PR itself can probably stand on its own.(I considered posting this section of the PR description as an issue instead, but I figured I'd put it here in case requested changes to this PR would substantially affect it or make it moot. and also because it provides a possible reason, beyond that discussed in #1745 (comment), to have Docker jobs like this one in the first place.)