elfutils: 0.180 -> 0.182, fix building under musl #110405
Conversation
pjjw
force-pushed
the
musl-elfutils
branch
3 times, most recently
from
618fa86
to
d2654de
Compare
|
@SuperSandro2000 ah thanks for the early review! left this one a draft b/c there was still some copypasta around but wanted to get CI on it. thanks for the formatting fixes, i'm relatively new here and don't have a firm grip on all convention. |
|
@GrahamcOfBorg build pkgsMusl.elfutils |
| }; | ||
|
|
||
| patches = [ ./debug-info-from-env.patch ]; | ||
| patches = [ ./debug-info-from-env.patch ] | ||
| ++ lib.optional stdenv.hostPlatform.isMusl ./musl-compat.patch; |
There was a problem hiding this comment.
Can we apply this patch all the time?
There was a problem hiding this comment.
We could, though the patch adds extra warnings about null pointer derefs in both versions. This is just cribbed from alpine and nearly unmodified, but didn't want to introduce a warning suppress where it's not needed. I could try and work the patch and see if I can make the compiler happy, but it's an unfamilar codebase and wasn't feeling all the way up to it.
Open to any option, applying and suppressing, keeping them separate with a note to come back, or trying to tweak the patch.
There was a problem hiding this comment.
The problem with applying patches conditional is that they will bit rot because no one will test them on the next update.
There was a problem hiding this comment.
OK, so I switched around the patch stuff to just pull from alpine's git repo, and only conditionally-applying the one making up for musl's lack of error.h, which is the one that causes the null-pointer warning (again, not sure why). also applies a patch that fixes the aarch64 musl build. PTAL.
There was a problem hiding this comment.
ah, right, well, this patch now makes the aarch64 build succeed if pkgsMusl.coreutils succeeds, which it does not right now due to a test failure i can't quite pin down.
pjjw
force-pushed
the
musl-elfutils
branch
2 times, most recently
from
2e8048b
to
d7c4615
Compare
There was a problem hiding this comment.
Can we just fetch the commit alpinelinux/aports@2e3d497 and apply it only to the files we require? fetchpatch should be able to do that and it would be a lot cleaner I think.
|
not sure i understand, that's what i'm doing here, just pulling the minimum set of patches from alpine master- but the commit you refer to is also for an older ver of elfutils and has incompatible patches. the other bits I was just letting nixfmt do its thing, is that not suggested? |
Nothing wrong with that but we can condense that to maybe one or two fetchpatches. fetchpatch supports
It takes 80 line lengths very serious and I don't like the line break style it does with |
|
I looked at that and I still don't understand- include and exclude works per-patch- AFAICT fetchpatch still expects to be fetching a patchfile, not a git directory composed of patches |
Yeah, it is. I thought you are fetching a big patch in small parts instead of the entire patch at once but after taking a closer look at https://github.com/alpinelinux/aports/tree/master/main/elfutils it is clear that the patch are 7 files. Never mind then. |
SuperSandro2000
requested review from
NeQuissimus,
matthewbauer and
lheckemann
|
@SuperSandro2000 you still show as wanting changes. Anything in particular you're looking for from this patch to get it landed? |
GitHub just keeps my last review action and only clears it when I submit a new one. I don't know the musl infrastructure to well so I just did not merge it to not break anything. |
ofborg
bot
added
8.has: clean-up
10.rebuild-darwin: 501+
10.rebuild-darwin: 5001+
10.rebuild-linux: 5001+
10.rebuild-darwin: 1-10
10.rebuild-linux: 1001-2500
and removed
10.rebuild-darwin: 1-10
10.rebuild-linux: 1001-2500
10.rebuild-darwin-stdenv
10.rebuild-linux-stdenv
10.rebuild-darwin: 501+
10.rebuild-darwin: 5001+
10.rebuild-linux: 5001+
labels
|
@SuperSandro2000 rebased to staging- look good? |
|
also, for the record, not sure why the robot rebase failed: |
SuperSandro2000
removed request for
matthewbauer,
stigtsp,
mmahut,
lheckemann,
ttuegel,
NeQuissimus,
RaghavSood,
Ericson2314,
edolstra,
FRidh and
jonringer
Motivation for this change
Trying to restore runctionality to a good amount of pkgsMusl- have copied the approach Alpine uses to get elfutils working in nixpkgs. Had to add two additional packages that are there essentially just for musl packages.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)