Skip to content

mupdf: actually build and install the shared libraries version#237199

Merged
delroth merged 1 commit intoNixOS:stagingfrom
delroth:mupdf-shared
Jun 12, 2023
Merged

mupdf: actually build and install the shared libraries version#237199
delroth merged 1 commit intoNixOS:stagingfrom
delroth:mupdf-shared

Conversation

@delroth
Copy link
Contributor

@delroth delroth commented Jun 11, 2023

Description of changes

The current version of the derivation builds in shared libraries mode, but then the "shared" flag is not passed to the "make install" invocation. This causes "make install" to perform a whole second build from scratch, in static mode, and install this to $out instead.

Instead pass shared=yes as part of the makeFlags -- this is basically the only thing that the "shared" build target does anyway, and I don't think there is a similar target for "make install".

This issue was detected because the .pc shipped with mupdf doesn't work with the static libraries currently shipped (it doesn't include recursive dependencies like zlib).

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

The current version of the derivation builds in shared libraries mode,
but then the "shared" flag is not passed to the "make install"
invocation. This causes "make install" to perform a whole second build
from scratch, in static mode, and install this to $out instead.

Instead pass shared=yes as part of the makeFlags -- this is basically
the only thing that the "shared" build target does anyway, and I don't
think there is a similar target for "make install".

This issue was detected because the .pc shipped with mupdf doesn't work
with the static libraries currently shipped (it doesn't include
recursive dependencies like zlib).
@delroth
Copy link
Contributor Author

delroth commented Jun 11, 2023

Incidental closure size savings:

$ du -sh /nix/store/81gp3akl0h55r9fs62jvnnbaim1rz8r7-mupdf-1.22.1-bin
175M    /nix/store/81gp3akl0h55r9fs62jvnnbaim1rz8r7-mupdf-1.22.1-bin

$ du -sh /nix/store/r45wsh1i07cwqha28nhd70vdd1m1bind-mupdf-1.22.1-bin
848K    /nix/store/r45wsh1i07cwqha28nhd70vdd1m1bind-mupdf-1.22.1-bin

@ofborg ofborg bot requested review from fpletz and vrthra June 11, 2023 15:12
@ofborg ofborg bot added 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches. labels Jun 11, 2023
@delroth delroth changed the base branch from master to staging June 11, 2023 15:14
@Mindavi
Copy link
Contributor

Mindavi commented Jun 11, 2023

I wonder how all those dependents consume mupdf if it was broken...

@delroth
Copy link
Contributor Author

delroth commented Jun 11, 2023

I wonder how all those dependents consume mupdf if it was broken...

The answer to that seems to be "via a dependency on cups-filters, which depends on the binaries only": --with-mutool-path=${mupdf}/bin/mutool.

The good news is that this means this PR should translate to a pretty nice closure size decrease on cups itself.

@delroth delroth added the 6.topic: closure size The final size of a derivation, including its dependencies label Jun 11, 2023
@delroth
Copy link
Contributor Author

delroth commented Jun 11, 2023

Before: /nix/store/1f0r6gh1jncp9iv49qmiybsx8n0rjfxa-cups-filters-1.28.17 464.8M
After: /nix/store/mxkg4yhg7fwar14vz6q56w9fvbb3hbhw-cups-filters-1.28.17 337.2M

@delroth
Copy link
Contributor Author

delroth commented Jun 11, 2023

And for completion's sake, I built the commit from Oct 2020 which removed the previously used custom patch for shared libraries building (e13120b):

$ nix build 'github:nixos/nixpkgs?rev=e13120bb098f25ac552570c2f9ea5908ef102a50#mupdf'
$ ls result-bin/bin -lah
-r-xr-xr-x 2 root root  35M Jan  1  1970 mupdf-gl
-r-xr-xr-x 2 root root  34M Jan  1  1970 mupdf-x11
-r-xr-xr-x 2 root root  34M Jan  1  1970 mupdf-x11-curl
-r-xr-xr-x 2 root root  34M Jan  1  1970 muraster
-r-xr-xr-x 2 root root  35M Jan  1  1970 mutool

So when dropping the custom patch and trying to use upstream's mechanism to use shared libraries, a regression was introduced and never noticed until now... :(

@delroth
Copy link
Contributor Author

delroth commented Jun 11, 2023

Actually nevermind, it was noticed (see the last few comments on the PR), but fixed by adding the transitive dependencies to downstream packages, which was the wrong fix. Ugh.

#103727 & #103723

@delroth delroth mentioned this pull request Jun 11, 2023
10 tasks
@github-actions
Copy link
Contributor

Successfully created backport PR for staging-23.05:

@vcunat
Copy link
Member

vcunat commented Jun 26, 2023

@delroth
Copy link
Contributor Author

delroth commented Jun 26, 2023

Feel free to revert this commit, I didn't realize that so many people had written workarounds around what is an obvious bug in the derivation (it was literally unusable unless you went and duplicated all its build inputs into your own build inputs). Frankly I don't have that much motivation to clean up other people's mess. Thanks for tracking down these Hydra failures.

If it had taken less than 2 weeks for a breakage to get noticed perhaps I'd still have some interest in fixing this, but I'm not about to dive back into this now.

vcunat added a commit that referenced this pull request Jun 26, 2023
This reverts commit 3d4769a.
This broke many packages; let's revert until that's resolved.
#237199 (comment)
vcunat added a commit that referenced this pull request Jun 26, 2023
This reverts commit 3d4769a.
This broke many packages; let's revert until that's resolved.
#237199 (comment)

(cherry picked from commit 597d5f8)
mccurdyc pushed a commit to mccurdyc/nixpkgs that referenced this pull request Jul 3, 2023
This reverts commit 3d4769a.
This broke many packages; let's revert until that's resolved.
NixOS#237199 (comment)
@lilyinstarlight
Copy link
Member

Second attempt at this PR is in #261113 (with all reverse regressions that I could (on aarch64-linux) find fixed)

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

Labels

6.topic: closure size The final size of a derivation, including its dependencies 10.rebuild-darwin: 101-500 This PR causes between 101 and 500 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 1001-2500 This PR causes many rebuilds on Linux and should target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants