Skip to content
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

Merged
merged 4 commits into from Jan 11, 2021

Conversation

L-as
Copy link
Member

@L-as L-as commented Jul 8, 2020

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.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after) (results: ~30% reduced)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@risicle
Copy link
Contributor

risicle commented Aug 16, 2020

The fetchpatch function takes a name = argument - it would be helpful if you used that to label these patches appropriately. Additionally, if you name a patch something like name = "CVE-1234-1234.patch", automated vulnerability scanners like https://broken.sh should be able to pick it up as "fixed".

@risicle
Copy link
Contributor

risicle commented Aug 16, 2020

For the tiff patch, naming it CVE-2019-12211.CVE-2019-12213.patch should allow scanners to pick it up.

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 :)

@L-as
Copy link
Member Author

L-as commented Aug 16, 2020

@risicle Done.
I built it, and tested it with imv (which did not work on aarch64 before!), and it works fine.

@risicle
Copy link
Contributor

risicle commented Aug 16, 2020

@ofborg build arrayfire

(local machine hanging on cuda)

@risicle
Copy link
Contributor

risicle commented Aug 16, 2020

One problem with this is it breaks the darwin build, because it adds the jxrlib dependency and nobody's got jxrlib building on darwin yet.

Edit: spoke too soon 👇

@risicle risicle mentioned this pull request Aug 16, 2020
10 tasks
@risicle
Copy link
Contributor

risicle commented Aug 16, 2020

Hmm looks like that was only the first darwin problem. The main problem is that the unbundling patch fixes Makefile.gnu but doesn't make equivalent changes to Makefile.osx.

@risicle
Copy link
Contributor

risicle commented Aug 23, 2020

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 prePatch phase, a postPatch phase, a preBuild phase, a postBuild phase, it's all starting to look like a complex fragile mess and I'd be really tempted to make the unbundling patch our own because it would allow us to eliminate many of the other things the various phases are trying to do (e.g. the makeFlagsArray trick can be eliminated).

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. libpng. Don't think there's anything we can do about this though.

@L-as
Copy link
Member Author

L-as commented Sep 5, 2020

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.

@risicle
Copy link
Contributor

risicle commented Sep 5, 2020

It could just be a matter of squashing our modifications down into the patch.

@risicle
Copy link
Contributor

risicle commented Sep 5, 2020

or squashing our changes down into a patch which sits on top of the fedora patch.

@colemickens
Copy link
Member

This works for me, I've been using it with imv on the pinebook pro. Thanks for doing this @L-as, I look forward to it being merged.

@L-as
Copy link
Member Author

L-as commented Sep 15, 2020

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.

@L-as
Copy link
Member Author

L-as commented Sep 16, 2020

@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
  '';
  # ...
}

@risicle
Copy link
Contributor

risicle commented Sep 16, 2020

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 libjpeg.src and extract the required files for just this package's use (perhaps by giving it to a runCommand ... "cp include/some_header.h $out/include/"-style derivation)?

Also, with freeimage itself being a library, what do you mean by requires these headers? Does it require these headers just at freeimage build-time, or at build-time of a program using freeimage?

@L-as
Copy link
Member Author

L-as commented Sep 16, 2020

@risicle The headers are only required when building freeimage, not when building derivations that depends on freeimage. As for using the src attribute, that is not possible for libjpeg, since it seems that transupp.c (for transupp.h) isn't actually included in the library either! It works fine if I also include that to be built into libjpeg.so, but because of that obviously just using the header won't work. As for libtiff, one of the headers is built when building libtiff, so that header needs to be somehow exported from the build process. Would using overrideAttrs be preferred?

@L-as L-as changed the title freeimage: Unbundle dependencies, fix AAarch64 support, and other fixes [WIP] freeimage: Unbundle dependencies, fix AAarch64 support, and other fixes Sep 17, 2020
@L-as
Copy link
Member Author

L-as commented Sep 17, 2020

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
@veprbl
Copy link
Member

veprbl commented Jan 11, 2021

@GrahamcOfBorg eval

1 similar comment
@veprbl
Copy link
Member

veprbl commented Jan 11, 2021

@GrahamcOfBorg eval

@Ericson2314 Ericson2314 merged commit da2562a into NixOS:staging Jan 11, 2021
Staging automation moved this from Ready to Done Jan 11, 2021
@Ericson2314
Copy link
Member

Thanks again @L-as for all your hard work with this!

@L-as L-as deleted the freeimage branch January 11, 2021 20:24
@uri-canva
Copy link
Contributor

This might have broken freeimage on macOS: https://hydra.nixos.org/job/nixpkgs/trunk/freeimage.x86_64-darwin/all
Trying to build be94ded locally now.

@uri-canva
Copy link
Contributor

The error is:

make -f Makefile.osx install 
make[1]: Entering directory '/private/tmp/nix-build-freeimage-unstable-2020-07-04.drv-0/svn-r1859/FreeImage/trunk'
install Source/FreeImage.h /usr/local/include
install: cannot create regular file '/usr/local/include': Permission denied
make[1]: *** [Makefile.osx:107: install] Error 1

@risicle
Copy link
Contributor

risicle commented Apr 22, 2021

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.

@uri-canva
Copy link
Contributor

be94ded fails on libtiff tests, now trying with the fix from #110729.

@uri-canva
Copy link
Contributor

With the fix the error is the same as the one on hydra, trying some older commits from this PR now.

@uri-canva
Copy link
Contributor

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.

@risicle
Copy link
Contributor

risicle commented Apr 22, 2021

But wait, the force-pushes are listed in the PR history:

L-as force-pushed the L-as:freeimage branch from 9de653e to be94ded on 10 Jan

and the links to the old heads are still valid

@uri-canva
Copy link
Contributor

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.

@uri-canva
Copy link
Contributor

Ok, tested all commits in the PR:

commit error
90736ab6c29963d5f18894228c6dc5b4fb6db0bc jxrlib incompatible
a3739ba6645bbb73050aac64e600a65f8dd6c4b4 jxrlib incompatible
f6f2092 jxrlib incompatible
50d6e41c3d2c3400518df85f5e7372e6f6e1f2c9 jxrlib incompatible
61f6e58aaba42430f707f1bafb3fc0e1c48eb175 big sur issue
d88ca144d5027134caa0e6302c8cbc03ce018ae3 big sur issue
d822b8dab3e13927affea71c6a2eae7f973e7c45 big sur issue
19ab108e0d03e4d041e70c1e9a5da29d1feac525 big sur issue
3b612aeaa733b9b659152a90609878a2da7406c6 big sur issue
eeb6728b0374e31df45b9728e7600794a5b2f99e big sur issue
b152b39c1f4acae404b4959767af604d1a8336c5 probably big sur issue
cb76d0d4803f606c9092c40e4ace17d29ec1909a 404
33bd7b8e2f43ffee2b903ebd6069d7b15ec485de 404
796083ce711059762d9b01feb7a262b9a233b775 404
72fd3e310bdd1d48cd3ed3e75ca4cb2ec9269744 404
bbd45ed9d1df28588788aadeb9aa15c6355abeea 404
364de14d30ee140e626f03789ffaade579bb9dc7 404
c8af9e83d46d47c8ce1c2daa04e83deec52ac314 chmod
8ee3e74e74f6273199d1ab681fe8f8584bfd9ac5 install
9b2628a15e129cefe235dc3bf363a1bdb5ed18fe install
6f83ffd72302045cfd67a1e3408931396e6bde38 install
27e05af4e9b2f458f0a43bc034aa31ee8d465830 install
9de653e0d9d67031f797e0c1ded1587b129454aa install
be94ded install

@uri-canva
Copy link
Contributor

Explanations of errors:

jxrlib incompatible

error: Package ‘jxrlib-1.1’ in /Users/uri/github/NixOS/nixpkgs/pkgs/development/libraries/jxrlib/default.nix:21 is not supported on ‘x86_64-darwin’, refusing to evaluate.

big sur issue

-- Detecting C compiler ABI info - failed
-- Check for working C compiler: /nix/store/gkcr1p2x0psdc31bavmqbfb2nwq0d6jn-clang-wrapper-7.1.0/bin/clang
-- Check for working C compiler: /nix/store/gkcr1p2x0psdc31bavmqbfb2nwq0d6jn-clang-wrapper-7.1.0/bin/clang - broken
CMake Error at /nix/store/z7qvx6wbvmjvs767g06h03pkzr5aqn6i-cmake-3.18.2/share/cmake-3.18/Modules/CMakeTestCCompiler.cmake:66 (message):
  The C compiler

    "/nix/store/gkcr1p2x0psdc31bavmqbfb2nwq0d6jn-clang-wrapper-7.1.0/bin/clang"

  is not able to compile a simple test program.

  It fails with the following output:

    Change Dir: /tmp/nix-build-libjpeg-turbo-2.0.5.drv-0/source/build/CMakeFiles/CMakeTmp

    Run Build Command(s):/nix/store/c3v8mz19las3k4afgqfjj9h054yv9sbz-gnumake-4.3/bin/make cmTC_a5f98/fast && /nix/store/c3v8mz19las3k4afgqfjj9h054yv9sbz-gnumake-4.3/bin/make  -f CMakeFiles/cmTC_a5f98.dir/build.make CMakeFiles/cmTC_a5f98.dir/build
    make[1]: Entering directory '/private/tmp/nix-build-libjpeg-turbo-2.0.5.drv-0/source/build/CMakeFiles/CMakeTmp'
    Building C object CMakeFiles/cmTC_a5f98.dir/testCCompiler.c.o
    /nix/store/gkcr1p2x0psdc31bavmqbfb2nwq0d6jn-clang-wrapper-7.1.0/bin/clang   -arch x86_64 -o CMakeFiles/cmTC_a5f98.dir/testCCompiler.c.o -c /tmp/nix-build-libjpeg-turbo-2.0.5.drv-0/source/build/CMakeFiles/CMakeTmp/testCCompiler.c
    Linking C executable cmTC_a5f98
    /nix/store/z7qvx6wbvmjvs767g06h03pkzr5aqn6i-cmake-3.18.2/bin/cmake -E cmake_link_script CMakeFiles/cmTC_a5f98.dir/link.txt --verbose=1
    /nix/store/gkcr1p2x0psdc31bavmqbfb2nwq0d6jn-clang-wrapper-7.1.0/bin/clang  -arch x86_64 -Wl,-search_paths_first -Wl,-headerpad_max_install_names CMakeFiles/cmTC_a5f98.dir/testCCompiler.c.o -o cmTC_a5f98
    ld: warning: passed two min versions (10.12.0, 10.12) for platform macOS. Using 10.12.
    ld: file not found: /usr/lib/system/libcache.dylib for architecture x86_64
    clang-7: error: linker command failed with exit code 1 (use -v to see invocation)
    make[1]: *** [CMakeFiles/cmTC_a5f98.dir/build.make:106: cmTC_a5f98] Error 1
    make[1]: Leaving directory '/private/tmp/nix-build-libjpeg-turbo-2.0.5.drv-0/source/build/CMakeFiles/CMakeTmp'
    make: *** [Makefile:140: cmTC_a5f98/fast] Error 2





  CMake will not be able to correctly generate this project.
Call Stack (most recent call first):
  CMakeLists.txt:7 (project)

probably big sur issue

checking for gcc... clang
checking whether the C compiler works... no
configure: error: in `/private/tmp/nix-build-bison-3.7.3.drv-0/bison-3.7.3':
configure: error: C compiler cannot create executables
See `config.log' for more details
builder for '/nix/store/1x46w6dy0al2y4xx8fshc7l4dy9db3v3-bison-3.7.3.drv' failed with exit code 77

404

trying https://sourceforge.net/code-snapshots/svn/f/fr/freeimage/svn/freeimage-svn-r1859.zip
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
curl: (22) The requested URL returned error: 404
error: cannot download source from any mirror

chmod

unpacking sources
unpacking source archive /nix/store/dpsnbzmcbaqnk1nmhxzfll66vqf5jibs-svn-r1859
source root is FreeImage/trunk
chmod: cannot access 'FreeImage/trunk': No such file or directory

install

make -f Makefile.osx install 
make[1]: Entering directory '/private/tmp/nix-build-freeimage-unstable-2020-07-04.drv-0/svn-r1859/FreeImage/trunk'
install Source/FreeImage.h /usr/local/include
install: cannot create regular file '/usr/local/include': Permission denied
make[1]: *** [Makefile.osx:107: install] Error 1

@uri-canva
Copy link
Contributor

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.

@uri-canva
Copy link
Contributor

Fixed in #120124.

@risicle
Copy link
Contributor

risicle commented Apr 23, 2021

So it's possible one of those was working on darwin if you tested them on catalina.

I probably tested these on macos 10.14 back then

Fixed in #120124.

👏

@uri-canva
Copy link
Contributor

Spoke too soon, it's still broken, now at runtime: #121550

@uri-canva
Copy link
Contributor

This is missing some fixup, the darwin dynamic library referring to dependencies in the temporary build directories rather than their final store paths:

Load command 24
          cmd LC_LOAD_DYLIB
      cmdsize 96
         name /private/tmp/nix-build-jxrlib-1.1.drv-0/source/build/libjpegxr.dylib (offset 24)
   time stamp 2 Thu Jan  1 10:00:02 1970
      current version 0.0.0
compatibility version 0.0.0
Load command 25
          cmd LC_LOAD_DYLIB
      cmdsize 96
         name /private/tmp/nix-build-jxrlib-1.1.drv-0/source/build/libjxrglue.dylib (offset 24)
   time stamp 2 Thu Jan  1 10:00:02 1970
      current version 0.0.0
compatibility version 0.0.0

@uri-canva
Copy link
Contributor

This is described in https://nixos.org/manual/nixpkgs/stable/#sec-darwin:

On Darwin, libraries are linked using absolute paths, libraries are resolved by their install_name at link time. Sometimes packages won't set this correctly causing the library lookups to fail at runtime. This can be fixed by adding extra linker flags or by running install_name_tool -id during the fixupPhase.

@uri-canva
Copy link
Contributor

Fix: #121646.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

imv fails to build on aarch64 (due to freeimage vendoring libpng)
9 participants