New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
freeimage: Unbundle dependencies, fix aarch64 support, and other fixes #92708
Conversation
The |
For the tiff patch, naming it Maybe shoehorning CVE-2019-12212 and CVE-2019-12214 into the name for the unbundling patch is worth it to get those struck off too. I don't think we build the docs, so tbh I don't think we should bother with the doxygen patch. If you do a rebase I'll do a proper review :) |
@risicle Done. |
@ofborg build arrayfire (local machine hanging on cuda) |
One problem with this is it breaks the darwin build, because it adds the Edit: spoke too soon 👇 |
Hmm looks like that was only the first darwin problem. The main problem is that the unbundling patch fixes |
Ok I've got it working for macos in my branch https://github.com/risicle/nixpkgs/tree/ris-freeimage-unbundled-darwin-fix Looking at it all now, having this stack of patches, a I also realised that there is a slight possibility of breakage of downstream applications that are using the static library. Any such applications will be expecting the static lib to have all necessary code bundled, and won't expect to have to link with e.g. |
I doubt that breakage will be a problem since it seems that most major distros do the unbundling. I'll take a look at doing the unbundling myself and see how easy it is. |
It could just be a matter of squashing our modifications down into the patch. |
or squashing our changes down into a patch which sits on top of the fedora patch. |
This works for me, I've been using it with |
Apparently there is a newer version of freeimage (r1859 / 3.19.0) which fedora uses since 2 months ago. It makes a lot of the patches redundant, which is nice. |
@risicle so freeimage requires private headers from libtiff (tiffiop.h) and private headers that should be public from libjpeg-turbo (transupp.h). Fedora just disables the plugin, but I assume that that's due to their build system not being powerful enough and the fact that it's not a monorepo, so coordinating changes across is probably hard. Do you think it's acceptable to make these 2 derivations export these headers? It is a simple change that just adds a line to CMakeLists.txt as so (for libjpeg-turbo): {
# ...
prePatch = ''
echo 'install(FILES ''${CMAKE_CURRENT_SOURCE_DIR}/transupp.h DESTINATION ''${CMAKE_INSTALL_INCLUDEDIR})' >> CMakeLists.txt
'';
# ...
} |
It's an interesting one - libjpeg is a pretty central package, and people like to limit weirdness. Seeing as this is a header change, might it be possible to dig into Also, with |
@risicle The headers are only required when building |
I pushed my WIP changes so that you can tentatively review them if you so desire. |
The transupp.h header is exported in the dev_private output.
…aarch64 support The unreleased version of freeimage contains many important fixes, amongst others CVEs, and is taken from the svn repository (r1859). We also unbundle all the dependencies to make it fit into the Nixpkgs ecosystem. All the changes needed to unbundle and make it compile with Nix is contained in unbundle.diff. Fixes NixOS#77653 Replaces NixOS#77655
@GrahamcOfBorg eval |
1 similar comment
@GrahamcOfBorg eval |
Thanks again @L-as for all your hard work with this! |
This might have broken freeimage on macOS: https://hydra.nixos.org/job/nixpkgs/trunk/freeimage.x86_64-darwin/all |
The error is:
|
Damn. I'd also try some of the earlier revisions in this PR as we definitely had it working on darwin eariler in this thread. |
With the fix the error is the same as the one on hydra, trying some older commits from this PR now. |
Unfortunately the branch was force pushed so I can't access the older revisions of the PR. 2cc1925 builds correctly but that's before the freeimage version bump. Will open a new issue for this in the meantime. |
But wait, the force-pushes are listed in the PR history:
and the links to the old heads are still valid |
You're right, I tried checking them out but I didn't have those commits since there's no heads pointing to them, but I can fetch them directly. Trying them now. |
Ok, tested all commits in the PR:
|
Explanations of errors: jxrlib incompatible
big sur issue
probably big sur issue
404
chmod
install
|
The big sur issues are because I'm building on big sur, and the range of commits that had that issue but not others were all based on master before the fix for it was merged: #107013. So it's possible one of those was working on darwin if you tested them on catalina. |
Fixed in #120124. |
I probably tested these on macos 10.14 back then
👏 |
Spoke too soon, it's still broken, now at runtime: #121550 |
This is missing some fixup, the darwin dynamic library referring to dependencies in the temporary build directories rather than their final store paths:
|
This is described in https://nixos.org/manual/nixpkgs/stable/#sec-darwin:
|
Fix: #121646. |
This pretty much just adapts https://src.fedoraproject.org/rpms/freeimage into nixpkgs.
All of the patches except the substream one is included, since it's not clear if it's
needed.
It seems to me that freeimage is unfortunately partly abandonware, considering
that there is a patch for a CVE included and that the last release was in 2018.
Fixes #77653
Replaces #77655
Motivation for this change
It was broken on AArch64, and there are apparently also other bugs in the codebase.
Things done
NB: I've only tried compiling it on aarch64.
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after) (results: ~30% reduced)