You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Recognize NO_PRELOAD_CXX to not preload Android C++ library
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.
0 commit comments