Set up cross for limited but substantial local testing of s390x and Android
#1895
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
EliahKagan
force-pushed
the
run-ci/s390x
branch
5 times, most recently
from
c36c4af to
79beacf
Compare
|
This isn't ready yet. At minimum, even if nothing else is to be accomplished here, the CI jobs should be removed (or made much more minimal?). But it might be valuable even if it doesn't give any more insight into #1890 or #1894 than it already has. It is already usable locally (though with #1894 affecting local runs too, of course). For me to decide what more to do on this and when, I'm curious if it would be considered of value to merge, with the CI parts removed. (The non-obvious CI things, like enabling If so, I could do that--or move this toward that end--while keeping a branch with CI to reproduce any demonstrate further locally obtained insights on #1890 or #1894, in the event that any arise. If not, then I would want to consider something far more limited than this with CI removed: there are minor bugfixes to the test suite included here--discovered when running |
Byron
approved these changes
Mar 20, 2025
There was a problem hiding this comment.
Thanks for reeling me in, and thanks for your incredible help in making this codebase more portable!
This PR represents considerable research and I'd want it all preserved, so let's keep the commits as is and put alterations, like the removal of the CI job on top.
From there it can be merge when deemed ready.
EliahKagan
force-pushed
the
run-ci/s390x
branch
from
79beacf to
4d26f5a
Compare
EliahKagan
force-pushed
the
run-ci/s390x
branch
5 times, most recently
from
e2fe9f0 to
b1a696e
Compare
So installation_config tests can pass. This also adjusts the RUN script in the dockerfile to show each command before it runs and to make sure it is not overwriting files unintentionally, and adds a convenience script outside for building the image.
Installing `jq` in the container allows the `ask::askpass_only` and `ask::username_password` to pass (they use `jq`).
This changes the style of how we name the custom Docker image for `cross` testing, so it is not looked up on Docker Hub (it is not there, and the text before the `/` in the old name was not intended to have a meaning related to resolving the image in any registry). This also allows through the environment varibles that currently have special meaning to gitoxide or its test suite -- including `GIX_TEST_IGNORE_ARCHIVES` to test the fixture scripts in the container, if it is set for the `cross` process -- or that the test suite may reasonably use to detect CI in general or GitHub Actions in particular. (Most GitHub Actions environment variables continue not to be passed through, because some of them would hold paths that couldn't necessarily be resolved properly in the container, while the presence of others would wrongly create the appearance that all GitHub Actions environment variables would be usable.)
Because `gix_testtools::umask()` is only suitable for use in tests, where signaling an error with a panic is typically acceptable, it panics rather than returning an `Error` to indicate errors. To avoid leading to the use of a potentially inaccurate umask value, it treats as an error any departure from the typical output of the `umask` command: in addition to treating a nonzero exit status as an error, it also treats anything it cannot strictly parse on stdout as an error, as well as anything at all written to stderr as an error. The latter is to avoid a situation where a warning is printed that is could be significant to some umask use cases. Warnings from `umask` are rare, as well as from the shell that is used as an intermediary for running the command (since no external `umask` command need exist and, often, does not) when it is used just to run `umask`. When they do occur, they are sometimes from the dynamic linker, such as a warning about a shared library listed in the `LD_PRELOAD` environment variable that cannot be used by the shell program. To understand and distinguish such errors, it is useful to show the text that was sent to stderr, since tests are sometimes run in environments that are nontrivial to reproduce otherwise. For example, running tests with `cross` produces an environment that is not in all respects the same as what one gets with `docker exec -it <container>`, even if `<container>` is the same still-running container being used to run the tests. This modifies `gix_testtools::umask()` so that when it panics due anything being written to stderr, it shows what was written.
(This is only temporarily in `gix-testtools` and shall either be moved to `internal-tools` or removed altogether.) It is useful to be able to observe environment variables that are set when running code with tools such as `cargo` or `cross`. This is therefore not intended for general-purpose use. A system command like `printenv` or `env` or a shell facility should be preferred where applicable, since it is considered a feature rather than a bug that commands like `cargo run -p gix-testtools env` include changes to the environment from `cargo` itself. Since one use for checking environment variables is to investigate the effects of environments that contain variable names or values that are not valid Unicode, this avoids requiring that environment variables all be Unicode. Any name or value that is not Unicode is shown in its Rust debug representation. This is always quoted, and to decrease ambiguity any name or (more likely) value that contains literal double quotes is likewise shown in its debug representation so that it is always clear if a quotation mark is just for display. Each name and value is otherwise shown literally. In either case the name or its representation is separated by a `=` from the value or its representation.
This further customizes the `cross` Dockerfile so it patches the
`/android-runner` script in the image so, when the script runs, it
checks if something like `NO_PRELOAD_CXX=1` is set (specifically,
if `NO_PRELOAD_CXX` exists with a value other than the empty string
or `0`) and, if so, refrains from setting `LD_PRELOAD` to the path
of an Android C++ standard library implementation. (This also adds
`NO_PRELOAD_CXX` to the list of variables `cross` allows to pass
from host to container environment, so it can be set easily.)
The problem this solves is warnings, which appear to be errors
based on their wording but which do not inherently cause failure,
that a library cannot be loaded into executables that are for a
different architecture. `ld.so` issues these when running x86-64
executables in the `cross` container for `armv7-linux-androideabi`.
Currently that includes shells (`sh`, `bash`) and `git`.
ERROR: ld.so: object '/android-ndk/sysroot/usr/lib/arm-linux-androideabi/libc++_shared.so' from LD_PRELOAD cannot be preloaded (wrong ELF class: ELFCLASS32): ignored.
While the warnings do not inherently cause failure, removing them
is not just to decrease noise in the output of test runs. Because
the messages are sent to stderr, they are picked up in code that
expects stderr to be of a particular form, including test cases
that include assertions about what is written to standard error.
In addition to not representing actual bugs, the resulting test
failures are confusing because they consist of a message that a
library cannot be preloaded for a platform-related reason ("wrong
ELF class"), followed by the failure of a test that exercises code
that runs an external command (or possibly, in some cases, tests
that capture stderr in a fixture script to set up a test).
That creates the appearance that the external command could not be
run, when actually the problem is that the external command ran,
often with no problems at all, but its captured stderr contained
one or (if it ran its own subprocesses) more extra lines describing
themselves as errors.
In e94526e and 094023a (both in GitoxideLabs#1687), I had claimed in comments
that the reason to test `gix-hashtable` rather than `gix` in the
`armv7-linux-androideabi` job (since removed) was that, in
`armv7-linux-androideabi`, some `gix` tests needed to be able to
run `git`, and needed ARMv7 `git`. This is technically true, but
misleading (and while the logs have expired so I cannot check what
I saw at that time, I suspect I was myself misled).
They have relied on ARMv7 `git` on such a platform in the sense
that the x86-64 `git` is completely usable but causes "wrong ELF
class" warnings, which appear to be errors, to be written to
stderr, breaking tests or test fixtures that examine stderr (as
well as any test that attempts to read a `once_cell::sync::Lazy`
that a previous test run in the same process tried to initialize by
running code that panicked due to such a failure -- when control
exits a `Lazy` initializer due to unwinding *from a panic*, the
cell enters a "poisoned" state and subsequent uses of it fail).
If an ARMv7 `git` were to be used, then the shared library -- even
if `git` didn't use the library -- would presumably be preloaded,
which would avoid the warning. But the problem was not inherent to
the architecture or build of binares like `git`, instead arising
from the incompatible `LD_PRELOAD` value. (Also, as far as I know,
we have so far never used an ARMv7 build of `git` in any `cross`
test... though it is used in the current `test-32bit` arm32v7 job.)
Avoiding setting `LD_PRELOAD`, as done here, fixes that. However,
this is not an ideal solution, because the library may be needed if
an external command is built for the Android target architecture
and is written in C++.
It might be ideal to suppress the warnings while still attempting
to load the library, but it's not clear if a reasonably simple way
to do so is available:
- `LD_PRELOAD` paths support `$LIB`, but I am not sure it can be
used here, because the library name, and not just the directory
prefix, differs. (Also, when not attempting to link to the C++
stdlib for Android, nothing would *ordinarily* be added to
`LD_PRELOAD` at all.)
- glibc issue https://sourceware.org/bugzilla/show_bug.cgi?id=20172
proposes a new environment variable `LD_PRELOAD_IGNORE_ERRORS`,
but no patch with that feature has been merged into any version
of glibc at this time (so certainly not the fairly old glibc in
the `cross` image).
The other reason this is not an ideal solution is that, at least if
testing with `cross` is to become fully useful, some external
commands in the `cross` container *should* be built for the target
architecture, because in practice those are the builds (or in some
cases, ports) that gitoxide would have to be compatible with. That
includes `git` and, if feasible, `sh`, and maybe some others such
as `gpg` and (if sufficiently tested to matter) `ssh`.
So while the existing workaround makes it so ARMv7 `git` is not
necessary to keep the tests from failing when they should pass, it
remains potentially necessary to keep the tests from passing when
they should fail. (This also applies to other architectures tested
with `cross`, particularly s390x, where failure occurring in native
runs, mentioned in GitoxideLabs#1687 and discused in GitoxideLabs#1698 comments, have not
yet been reproducible when testing with `cross`. The reason is
*possibly* some difference between the behavior of x86-64 and
s390x `git`.)
However, even if that can be achieved, it will likely also be an
incomplete solution on its own, because as long as we may ever
capture stderr from *some* external command where the warning is
issued, we have to suppress or tolerate the warning in some way.
So the `cross` Dockerfile can continue to be used for both Android and non-Android targets.
`interpolate_user()` gives `UserInterpolationUnsupported` on both Windows and Android, which is intentional. But the tests treated Windows as the only platform under which this was the case, thereby failing on Android tests. This may have been obscured by prior difficulties running tests on Android builds of `gix-config-value`.
Rather than only on non-Android Linux-based systems. Like most Linux-based systems, Android has `/proc`, so the test can get the umask expected value using its `/proc`-based approach that is independent of the approach used in the code under test.
For `armv7-linux-androideabi`, in
GIX_TEST_IGNORE_ARCHIVES=1 cross test ...
as tested by running
GIX_TEST_IGNORE_ARCHIVES=1 just cross-test-android
the `id::ancestors::pre_epoch` test failed with:
/project/gix/tests/fixtures/make_pre_epoch_repo.sh: line 12: patch: command not found
This fixes it by making sure `patch` is installed in the container.
Since we also use `cross` to build some of the releases.
This removes "(limited)" from the description of the `justfile` recipe `cross-test-android`. It *is* very limited, but that label is misleading, since currently `cross-test-s390x` (as well as other `cross` testing targets that could be run through the parameterized `cross-test` recipe) are limited in all the same ways. In particular, we are not currently testing them with any external components built for the target architecture. For example, `git` built for the host has been upgraded in the container and the environment has been configured so `gix-path`, `gix-testtools`, any test cases throughout the workspace can use it without problems (either directly or through a shell). In the future, some `cross` targets may gain a richer target environment to interact with when testing, by installing target builds of `git`, by installing target builds of libraries that facilitate more features than `max-pure`, or in other ways. At that point, convenience recipes for specific targets in the `justfile` that are more limited than other targets could be marked "(limited)".
From `gix-testtools`, where it was originally placed. This also adapts and improves the original description from the commit message that had introduced the subcommand, to make clear when it makes sense to use this (given that operating systems and shells already supply facilities for listing environment variables).
In addition to any invalid Unicode and double quotes, as before. + Revise its documentation.
This *should* be able to run as written, but it can be improved.
Rather than having the last step build the image through the `justfile` recipe dependency, and rather than having it install the `cross` target implicitly due to `cross` finding that it is not yet installed, this performs those actions in preceding steps.
Currently this fails with:
Some packages could not be installed. This may mean that you have
requested an impossible situation or if you are using the unstable
distribution that some required packages have not yet been created
or been moved out of Incoming.
The following information may help to resolve the situation:
The following packages have unmet dependencies:
git:s390x : Depends: perl:s390x but it is not going to be installed
Depends: liberror-perl:s390x but it is not installable
Or, in some variations, the (same) error is expressed as:
E: Package 'liberror-perl:s390x' has no installation candidate
This does not work as written, at least on s390x, because the ports list is already written there from the base image. The existing file is actually better than what this writes, though what this writes is simpler and so might potentially be easier to debug or reason about. Unfortunately, if we clobber the existing file with this one, the error installing `git:s390x` is unchanged. Uninstalling the old `amd64` `git` package first (not shown here) and packages installed as its dependencies doesn't help either.
Thus, the lightweight manual approach used before isn't the cause.
The following packages have unmet dependencies:
perl : Conflicts: perl:s390x but 5.22.1-9ubuntu0.9 is to be installed
perl:s390x : Depends: perl-base:s390x (= 5.22.1-9ubuntu0.9) but it is not going to be installed
Depends: libperl5.22:s390x (= 5.22.1-9ubuntu0.9) but it is not going to be installed
Conflicts: perl but 5.22.1-9ubuntu0.9 is to be installed
This does cause `git:s390x` to install in the s390x cross container but the changes are not finished -- container creation still fails due to a subsequent attempt to fix missing packages. Assuming `git` can use `perl` from a different architecture, which seems likely since it is invoking it as an interpreter (rather than linking to it), this should be fine, but to limit container bloat some other changes are needed to make it so we don't need the `autoremove` operation to get rid of files only used in setting up the PPA.
This avoids installing `software-properties-common` in the container, instead manually setting up the PPA again. Accordingly, it is not necessary to uninstall any packages to avoid high bloat in the image, so that is removed. Other cleanup is still done. This also removes the old `git` and packages that were marked as automatically installed that were only needed as its dependencies, and shows details about the `git` that is installed. For s390x, this works to cause the test suite to use a s390x build of `git`, at a reasonably new version as provided by the PPA. That should happen for some other targets, but not all. This therefore shows the `git` version as well as examining the executable to report its architecture while the container is being built. Note that this does not yet appear sufficient to produce the s390x test failures mentioned in GitoxideLabs#1687 and discused in GitoxideLabs#1698 comments. (That might relate to how this is still just testing `max-pure`.)
It is provided by `dpkg-dev`, which is installed already in some but not all `cross` images. This commit also: - Allows more configuration to occur by installing `apt-utils`. Without this, `gnupg` warns about skipping some debconf configuration. - Sets up the environment in which commands like `apt-get` and `dpkg` run so configuring packages never prompts. Passing `-y` is not sufficient to achieve this. (The problem currently does not occur, but it would happen in at least the Android image if a new dependency brings in `tzdata` indirectly, as will occur if attempting to install dependencies to test more than `max-pure`.)
+ Adjust whitespace style by letting short closely related commands appear on the same line. Broadly speaking, this style is often not clearer, but in this case, especially with the new commands here, it seems to improve clarity. (Though maybe not as much as breaking the `RUN` contents out into a script file, where they could instead be easily grouped with whitespace and comments.)
This modifies the `cross` dockerfile and associated `justfile` recipes to install dependencies needed for testing beyond max-pure. This works when installing packages for the cross target is already working, which is the same scenario where the upgraded `git` (from the git-core PPA) is installed for the `cross` target. But unlike `git`, when testing max-pure it is not necessary to install these libraries. Yet the current approach does install them... but native builds of them. This is inefficient in the Android test where there may be no immediately forthcoming way to install appropriate `cross`-target builds of the libraries. This extra work should probably be eliminated. But the best way to do that will depend on whether the two cases of cross targets (those we can install packages for and those we cannot) remain only two cases, and whether a single `RUN` script continues to be used, versus splitting it (or creating multiple `cross` dockerfiles). This also gives the `cross-test` recipe a `*cargo-test-args` parameter, and moves `--no-default-features --features max-pure` out of `cross-test` and into the arguments `cross-test-android` passes to `cross-test` (causing `*cargo-test-args` to receive it). The reason for doing this rather than having a parameter that is put in place of `max-pure`, for which `cross-test-s390x` could pass `max`, is that it is not obvious that `--no-default-features` is free of undesired effects when `--workspace` is used: it seems like it may suppress some features of some tested `gix-*` crate projects that do not individually recognize `max`. (This differs from the situation with the journey tests, which test the `ein` and `gix` executables from by the `gitoxide` crate, which recognizes `max-pure`.) The intuitive effect of "testing max" is currently to run the entire test suite with all default features enabled, so this strives to do that. This still does not appear sufficient to produce the s390x test failures mentioned in GitoxideLabs#1687 and discused in GitoxideLabs#1698 comments, even though this is now running s390x tests with the same full default features as were used when those failures were observed.
- Clarify and expand comments. - Adjust style for greater consistency within the script. - Have the script tolerate leading whitespace when it patches the Android runner script, since it looks like future releases of `cross` may have it undented under an `if` check for the Android version (not all versions have a C++ library in the NDK). - Move the script into a subdirectory that will be used as context. - Replace the old `RUN` step with a command to run the script. - Modify the build command in the `justfile` to use the context.
We install `git` in the `cross` container, intending that it come
from the git-core PPA and thus be of a recent version. If we can
find a Debian-style architecture name that is correct for the
`cross` target triple, then we install it for that architecture. In
principle we might find such a name without the PPA having a build
for that architecture, but that is in practice unlikely to occur
and, if it does, then either the `git` we do manage to install is
new enough, or we will get test failures due to absent features the
test suite needs (such as `GIT_CONFIG_{COUNT,KEY,VALUE}` support).
When we don't find such a name, we use the host architecture.
When we install `git` for the target architecture, its `perl` and
`liberror-perl` dependencies, which as declared must be of that
architecture as well, may not be installable. We run into this
problem in the container for `s390x-unknown-linux-gnu`. To solve
it, we install the dependencies except for those, as well as
ensuring those are installed for the host architecture, and then
install `git` while forcing it install even if those two
dependencies are considered unmet. This works because `git` doesn't
use `perl` as a library, but rather as an interpreter for some
scripts; and `libperl-error` is a library, but it is a Perl module
used from Perl scripts, not a shared library object any Git-related
binaries have to load. (Currently, direct dependencies of `git` are
hard-coded, which, as commented, may be worth changing to decrease
the likelihood of breakage in future versions.)
In order to test that the procedure really has a chance of working
on a variety of architectures (in case `cross` is to be used to
test others at some point), this complex procedure for installing
`git` is used whether it is being installed for the target or the
host architecture. But this doesn't provide much assurance; that
the procedure works when it is effectively just installing exactly
the same packages as would be installed anyway does not imply that
it works with other cross-target installations where it would be
installing different packages. More importantly, using this even
when it is known not to be needed obscures the cases where it is
not needed, and may create the impression that test failures are
due to the strange way `git` was installed, even when they are not.
Therefore, this modifies the `cross` container customization script
so we only take the strange semi-manual dependency installation
approach when there is a chance it might actually be needed, i.e.,
when we are installing `git` for the cross target rather than the
host target.
So option arguments like `--test-threads=1` that must come after `--` can be passed through `just` to `cross`, while still being able to customize options like `--features` that go before it.
Also, since `--test-threads 1` didn't reveal anything different locally, this uses the nicknamed convenience recipes.
One or both of these matrix jobs, or related ones, be brought back some day, probably to run only specific tests that have failed in the past and started passing. The files for local testing with `cross` (and associated `justfile` recipes) are kept.
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.
This allows local testing of some targets that are interesting but may not be justified to test--or at least not very extensively--on CI, given the tests we already have. The targets are
s390x-unknown-linux-gnuandarmv7-linux-androideabi, though it's hoped that the samecrossconfiguration andDockerfilewill work with various other targets too.Why this does extend CI, for now
Right now, it does attempt some CI checks, but this is mostly just as part of the process of working on the PR. I am not sure if new CI checks are part of what I will ultimately propose here once this is ready to review. Even if so, they might be pared down, so as to build and test less, and therefore take less time.
Relationship to #1890
I had originally hoped that this would allow #1890 to be reproduced without s390x hardware. Currently this does not manage that. I have another feature branch on this feature branch, which I shall eventually get to the point of merging into this feature branch and making part of this PR (whereupon this PR might be ready, I am not sure). But even those forthcoming commits, which include s390x-related improvements such as using the s390x build of
gitand most of its dependencies, has not produced the failures.It seems to me that this will eventually be worth merging even if it never manages to reproduce that, but it might not be worth having any CI for in that case.
Relationship to #1894
This did find a regression, which may be a bug in gitoxide, brought about by dependency updates in cf7f34d (#1882), in the
armv7-linux-androideabi. I've reported this with extensive details in #1894.The hope that this could help showcase that with CI even if the CI jobs are ultimately removed before merging is really what led me to add CI jobs here. But another reason is for them to look for ways I might be depending on my local configuration inadvertently.
Ways QEMU is invoked
One way I am depending on my local configuration inadvertently is that
binfmt_miscneeds to be configured in the local kernel to run QEMU automatically. I have this set up, but it seems not to be set up on CI.This is not needed for running test binaries, because
crossuses a runner script in the container that conditionally runs an appropriate QEMU emulator explicitly. But it is needed to run target-architecture binaries directly inside the container, which is something the tests also need to be able to do:arrowprocess filter. For s390x, in the commits I plan to merge into here, they also need to be able to do it so they can run an s390x build ofgit.gitis used in both the Androidcrosscontainer, where no nativegitis readily available, and in the s390xcrosscontainer, where an s390xgitis readily available but involves some subtleties to set up.So that's something I intend to improve. Then even if the new CI jobs are removed, I can document the issue precisely. (We can set up
binfmt_miscon CI, but we likely should not attempt to do it locally through thejustfile, since it is a privileged operation.)Improvements to the actual test suite
In addition to attempting to provide further ways of testing locally, this also does include some actual bug fixes, albeit only in the test suite, not in the code under test. For Android, one test has the wrong
#[cfg(...)], which had to be fixed to let it pass. Another had an analogous problem, which was useful to fix since it let it run rather than wrongly be skipped. Some of the error reporting I had added for a test helper in a previous PR was also limited in ways that made it hard to understand test output in some situations, which is improved.Also, there are considerable subtleties in what environment variables get through
cross, because they are filtered or modified at multiple points, so this adds aninternal-toolssubcommand to view them in a way that can easily be run withcargoorcross, which may occasionally be helpful to see howcargomodifies the environment, but is something I found especially helpful to see howcrossmodifies the environment.