Fix generation_number_overflow 32-bit tag creation #1698
Merged
Conversation
This file contains hidden or 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
This temporarily instruments a fixture script to reveal how the
`access::generation_numbers_overflow_is_handled_in_chained_graph`
test fails when `GIX_TEST_IGNORE_ARCHIVES=1` is set: the fixture
script `generation_number_overflow.sh` runs, and it sets the
timestamp `@4147483646` on a commit, which `git` permits. But then
`git tag` rejects this timestamp as being out of range. On stderr:
+ commit future-1 '@4147483646 +0000'
+ local message=future-1
+ local 'date=@4147483646 +0000'
+ local file=future-1.t
+ echo future-1
+ git add -- future-1.t
+ '[' -n '@4147483646 +0000' ']'
+ export 'GIT_COMMITTER_DATE=@4147483646 +0000'
+ GIT_COMMITTER_DATE='@4147483646 +0000'
+ git commit -m future-1
+ git tag future-1
fatal: Timestamp too large for this system: 4147483646
The nature of the problem is *partially* illuminated by the
following experiment on a 32-bit Debian 12 system:
$ git init repo
Initialized empty Git repository in /home/ek/src/repo/.git/
$ cd repo
$ touch a
$ git add a
$ GIT_COMMITTER_DATE='@4147483646 +0000' git commit -m 'Initial commit'
[main (root-commit) 99b1de4] Initial commit
1 file changed, 0 insertions(+), 0 deletions(-)
create mode 100644 a
$ git log --pretty=fuller
commit 99b1de4e0610ff715e784bd4089dc315e8071332 (HEAD -> main)
Author: Eliah Kagan <[email protected]>
AuthorDate: Sat Nov 23 01:05:12 2024 -0500
Commit: Eliah Kagan <[email protected]>
CommitDate: Thu Jan 1 00:00:00 1970 +0000
Initial commit
$ git tag t
fatal: Timestamp too large for this system: 4147483646
$ git tag -a t
fatal: Timestamp too large for this system: 4147483646
The timestamp is accepted when committing, and written as actor
metadata for the committer. But is is not (or at least not always)
interpreted as intended: as `git log` shows it, it has wrapped
around to 1970. Even though the commit was created successfully,
attempting to make a tag that points to it fails. This happens both
for a lightweight tag (as in the fixture script) and an annotated
tag, so making it a tag object does not seem to be a workaround.
This is a workaround for the problem where `git tag` rejects the timestamp `4147483646` from `generation_number_overflow.sh` as out of range, even though it has already been accepted to create a commit and `git tag` is obtaining it from the commit metadata. The test already passed if run without `GIX_TEST_IGNORE_ARCHIVES`, even though the commit and tag metadata in the committed archive repo has this timestamp interpreted as from 1970 rather than in the far future. The change here just allows the fixture script to create the repository on a 32-bit system where `git` would refuse to make the tag, by manually creating a loose tag. (Creating a packed tag would also work, but would not improve the ambiguity.)
Byron
approved these changes
Nov 23, 2024
There was a problem hiding this comment.
Thanks again for your help!
It's good to see that the fixture scripts are this portable, and to have a test run now that prevents that from changing.
I wonder if there's a Git feature that just lets us tell it to go ahead and create the tag, but I looked for such a feature and did not find it. I also wonder (as detailed below) if this general approach--of trying to create the tag on 32-bit systems at all--is good...
I think this should be related to size_t used for time_t so Git probably isn't capable of holding this number. I wonder if it has to be a problem for gitoxide thought, we could just declare the respective types as u64 and be good - for gix-date I am pretty sure that's done as well. This is probably no more than a note.
That test in Git is
t5328-commit-graph-64bit-time.sh. The test description is "commit graph with 64-bit timestamps" and it only runs when prerequisites are satisfied related to the availability of 64-bit timestamps.
I see, that probably explains why 32bit systems are not able to read 64 bit timestamps: by design. I still wonder if a 32bit built could be made to handle this fixture, but maybe there isn't any problem and it's really just about Git not being able to produce the fixture on 32 bit systems.
I think that's a convoluted way of wondering if a 32bit build can read commit-graph caches that are created on 64 bit systems. Probably that's not really an issue even if the answer is negative as one can always recreate these
Some platforms have
If gitoxide is using a type like But I acknowledge that "if at all possible" is an important qualification. Timestamps that are stored in indexes or caches, if It occurs to me that the failures on s390x mentioned in #1687 might be relevant to this and might be illuminating. I have not yet investigated them. Their significance there was merely that none of them involved any data structure size assertions--that is, I mentioned s390x there merely as evidence that our asserted 64-bit data structure sizes don't seem to vary across platforms. But, while not related to anything being done in #1687, on s390x there are a few tests that fail, and the failures do seem related to some data structures having unexpected representations. s390x is 64-bit, but bigendian. The failures even when reusing generated archives are: When ignoring generated archives with At this time, I haven't opened an issue about this or looked into it in detail. But full test output on s390x, showing those failures, for both runs, can be seen in this gist.
Even if
It is because of (4) that the change in this PR does not (as far as I know) add any new incorrect behavior. If not for (4), we would not want to allow a non- To elaborate, here's an experiment that verifies this, involving two machines: I then pushed to a remote and cloned the repository on the 64-bit system, on which: There, the second commit has the originally intended 2101 date, even though it was shown as having the 1970 epoch date on the machine where it was created originally. Furthermore, the first commit has the intended 1970 epoch date on both, so the 32-bit system on which the repository is created really is creating the commits with different timestamps, even though that 32-bit system has 32-bit (As noted above, some 32-bit systems have 64-bit That the commit timestamp has not really overflowed is, I want to say, no surprise--
Since 32-bit Although |
|
Thanks a lot for sharing!
This is very interesting. What stuck out to the difference in compressed size, which was 49 but should be 46. I have no idea how that comes to be though, but also didn't look into it.
I assume this was done because
Indeed! Thanks again for sharing these details, they are illuminating. |
Ordinarily, I would guess that this is due to different data structure layouts across architectures, which due to alignment restrictions can result in data structures being of different sizes. The reason I had tested with s390x when researching #1687 was the idea that maybe some 64-bit size assertions with That's a very weak guess. If I figure out more about that, I'll let you know (probably in an issue, unless I also figure out how to fix it).
I don't think tag creation via The successful invocations of |
EliahKagan
commented
Nov 25, 2024
| tick | ||
| function commit() { | ||
| local message=${1:?first argument is the commit message} | ||
| function force_tag() { |
There was a problem hiding this comment.
Maybe this should be named something else. Calling it force_tag makes it sound like it does something similar to git tag -f, when actually:
force_tagtries (though not very hard) to avoid resetting an existing tag.git tag -fwill still refuse to create a tag in the scenarios where this is intended to be called (or I would've just usedgit tag -f).
(I had meant to bring this up before the PR was merged, but forgot.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 9, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 10, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 10, 2025
This avoids installing `software-properties-common` in the container, instead manually setting up the PPA again. Accordingly, it is not necessary to remove 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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 10, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 11, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 12, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 12, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 12, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 17, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 17, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 17, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 18, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 18, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 18, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 18, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 18, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 18, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 18, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 18, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 19, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 19, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 19, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 20, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 20, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 20, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 20, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 20, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 20, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 23, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 23, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Mar 23, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 6, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 7, 2025
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.
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 7, 2025
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`.)
EliahKagan
added a commit
to EliahKagan/gitoxide
that referenced
this pull request
Apr 7, 2025
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.
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.
Overview
In the "Coupling to gitoxide's own repo" section of #1687, I said:
But the situation in containers is actually better than that. Setting
GIX_TEST_IGNORE_ARCHIVES=1on a 32-bit Debian 12 system--both in the container on CI or locally outside the container--only causes one failure. This failure is not related to coupling to gitoxide's repo, and it is a 32-bit specific failure.This pull request describes and fixes that failure, but I don't know if this fix is optimal. Even if this fix is good, I suspect further changes are warranted.
The current test failure output
The failing test is
gix-commitgraph::commitgraph access::generation_numbers_overflow_is_handled_in_chained_graph:Analysis of the current failure
The test calls
graph_and_expected:gitoxide/gix-commitgraph/tests/access/mod.rs
Line 58 in 438ee47
graph_and_expectedcallsgraph_and_expected_named, which callsscripted_fixture_read_onlyto run thegeneration_number_overflow.shfixture. It wasn't immediately obvious, from the output shown above, what the failure is. But temporarily addingset +xrevealed it:git commitaccepts the timestamp 4147483646 as the committer date. But then, whengit tagis run to tag that commit, it rejects the timestamp!Deeper analysis
The situation becomes even more interesting when we examine how
gitinterprets the timestamp on the commit. Here's a simplified experiment:The timestamp was accepted, but the reported commit date in
git logis:This is, as one would imagine, specific to 32-bit systems. (I also do not know for certain that it happens on all 32-bit systems, but my guess is that it happens with all 32-bit builds of Git on at least some prominent 32-bit platforms, including 32-bit x86 where I have observed it.) On 64-bit systems, we get a future date and there are no errors.
Why it passes without
GIX_TEST_IGNORE_ARCHIVES: 32-bitgituses the tag if presentA repository with such a tag can be cloned on a 32-bit system. The commit has the overflowed date in 1970. But the tag is present. It is a packed tag, in that scenario. But
giton a 32-bit system has the ability to recognize it as a loose tag just as well, which happens if we bring the repository over from another system, as happens with a generated archive.It also works if we create the tag by manually creating a file in
.git/refs/tags. This allows a repository that, as far as I can tell, is equivalent to what we get when we unpack a generated archive, to be created on a 32-bit system wheregit tagwould produce an error. I wonder if there's a Git feature that just lets us tell it to go ahead and create the tag, but I looked for such a feature and did not find it. I also wonder (as detailed below) if this general approach--of trying to create the tag on 32-bit systems at all--is good...Changes in this pull request
...but that's what I've done here. This PR:
GIX_TEST_IGNORE_ARCHIVES=1in thetest-32bitCI job. Ideally we would test both with and without it, either in separate jobs or with a partial clean, covering just generated archives, in between. I'm not sure the added work on CI of running the tests another time on the same 32-bit platform is justified, though. For now it only runs with that set. But I've verified locally that, after all changes here, all tests pass both with and withoutGIX_TEST_IGNORE_ARCHIVESon a 32-bit Debian 12 system.git tag, then it checks ifgitis a 32-bit build and, if so, writes a loose tag file explicitly. The logic for checking if Git is 32-bit and attempting the fallback tactic of creating the loose tag file explicitly is in its own function.commitfunction that performs multiple operations including committing and creating a tag. This renames it totagged_commitand makes a few other changes.Further analysis / alternatives and future directions
It may be that more substantial changes should be made. Some of the operations in the fixture script had been inspired or derived from one of the tests in the Git test suite:
gitoxide/gix-commitgraph/tests/fixtures/generation_number_overflow.sh
Line 32 in 438ee47
That test in Git is
t5328-commit-graph-64bit-time.sh. The test description is "commit graph with 64-bit timestamps" and it only runs when prerequisites are satisfied related to the availability of 64-bit timestamps.I don't think this necessarily means that gitoxide's test suite should not run tests on a repository with such timestamps. The test does pass on 32-bit Debian 12 when it uses the repository from the committed archive, as well as when it generates the repository using the modified fixture script introduced in this PR.
The other issue--I'm not sure if it's related--is that it looks like the test that uses this fixture is believed already, and presumably not specifically on 32-bit systems, not to be fully effective. The assertion in the test is:
gitoxide/gix-commitgraph/tests/access/mod.rs
Lines 64 to 68 in 438ee47
So maybe there is another change that should be made to the fixture script.