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

Jump to bottom

stdenv: prune libtool files by default #51767

Merged
merged 5 commits into from Jan 11, 2019
Merged

stdenv: prune libtool files by default #51767

merged 5 commits into from Jan 11, 2019

Conversation

pbogdan
Copy link
Member

@pbogdan pbogdan commented Dec 9, 2018 •
edited

Motivation for this change

Spotted in the wild:

# Generated by ltmain.sh - GNU libtool 1.5.22-OpenLDAP (1.1220.2.365 2005/12/18 22:14:06)

which pruneLibtoolFiles fails to prune as is.

/cc @orivej

EDIT: now extended to pruning libtool files by default

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

orivej
orivej approved these changes Dec 9, 2018
View reviewed changes
Copy link
Contributor

@orivej orivej left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

Since it's already a mass rebuild, we could as well enable pruneLibtoolFiles by default. I don't expect any immediate effect, but it will make it easier to add new packages and will make it possible to unpropagate some otherwise private dependencies of existing packages.

@hedning
Copy link
Contributor

hedning commented Dec 9, 2018

👍 on enabling by default. This seems like a pretty common issue, just recently upgrading hunspell, cyrus_sasl and iptables caused downstream failures due to new -ls in the .la files. So there's definitely a risk with plain upgrades too.

@hedning hedning mentioned this pull request Dec 9, 2018
Merged
10 tasks
@pbogdan pbogdan changed the title pruneLibtoolFiles: more permissive regex for detecting libtool files stdenv: prune libtool files by default Dec 10, 2018
@pbogdan
Copy link
Member Author

pbogdan commented Dec 10, 2018

Bit out of my depth here but pushed an amended version that enables this by default. Kept the top-level pruneLibtoolFiles attribute on the principle it may have some out of tree uses.
Would appreciate a review here to make sure this is proper way to implement this, please feel free to (force) push to my branch if anything needs to be amended (I'm of course happy to apply any suggestions myself too).

@hedning hedning mentioned this pull request Dec 20, 2018
Closed
@nixos-discourse
Copy link

This pull request has been mentioned on Nix community. There might be relevant details there:

https://discourse.nixos.org/t/prs-ready-for-review-december/1711/4

@Mic92 Mic92 requested review from vcunat and globin December 21, 2018 12:34
@Mic92
Copy link
Member

Mic92 commented Dec 21, 2018

@matthewbauer I wonder if this would break staticPkgs?

@hedning
Copy link
Contributor

hedning commented Dec 21, 2018

It only prunes .la files where old_library='' which is meant to avoid breaking static builds, as I understand it.

@pbogdan
Copy link
Member Author

pbogdan commented Jan 7, 2019

It doesn't appear there's consensus / interest to move this forward at this time. Shall this be closed for now?

@orivej
Copy link
Contributor

orivej commented Jan 11, 2019

I'll do some testing and if there are no issues will merge to staging.

@orivej-nixos orivej-nixos merged commit 9a21967 into NixOS:staging Jan 11, 2019
@orivej
Copy link
Contributor

orivej commented Jan 11, 2019

@pbogdan, thank you! These changes are exactly what I meant to be done.

@pbogdan
Copy link
Member Author

pbogdan commented Jan 11, 2019

Thank you @orivej!

vcunat added a commit that referenced this pull request Feb 10, 2019
@vcunat
treewide: fix missing libs after libtool pruning #51767
64edccb
@vcunat
Copy link
Member

vcunat commented Feb 10, 2019 •
edited

There were some regressions, but not too many and almost all were trivial to fix: 64edccb.

@pbogdan pbogdan deleted the la-prune-regex branch December 3, 2019 17:07
@orivej orivej mentioned this pull request Jan 30, 2020
Merged
10 tasks
@alyssais alyssais mentioned this pull request Jan 30, 2020
Merged
10 tasks
alyssais added a commit to alyssais/nixpkgs that referenced this pull request Jan 30, 2020
@alyssais
dino: remove unnecessary private transitive deps
53f0e2f 
As explained by Orivej (reformatted from Markdown):

> These (except libsignal-protocol-c) are private dependencies of the
> actual dependencies that should neither be propagated nor added
> manually to dino. For example, libpsl and brotli come from
> libsoup-2.4.pc:
>
>     Requires: glib-2.0 >=  2.38, gobject-2.0 >=  2.38, gio-2.0 >=  2.38
>     Requires.private: libxml-2.0, sqlite3, libpsl >=  0.20, libbrotlidec, zlib
>
> (To be precise, glib uses utillinuxMinimal rather than utillinux.)
>
> The warnings we see, such as Package 'mount', required by 'gio-2.0',
> not found, come from CMake running both pkg-config --libs and
> pkg-config --static --libs to populate both <XXX>_LIBRARIES and
> <XXX>_STATIC_LIBRARIES[1], but dino has no use for the latter.
> Currently these warnings can not be disabled:
> https://gitlab.kitware.com/cmake/cmake/issues/18158
>
> (They could be prevented by pruning Requires.private from shared-only
> libraries akin to <NixOS#51767>,
> although it can not be detected if a library is shared-only from the
> .pc file alone, and this is just a warning.)
>
> [1]: docs: https://cmake.org/cmake/help/v3.16/module/FindPkgConfig.html
FRidh pushed a commit that referenced this pull request Feb 10, 2020
@alyssais @FRidh
dino: remove unnecessary private transitive deps
c856d7f 
As explained by Orivej (reformatted from Markdown):

> These (except libsignal-protocol-c) are private dependencies of the
> actual dependencies that should neither be propagated nor added
> manually to dino. For example, libpsl and brotli come from
> libsoup-2.4.pc:
>
>     Requires: glib-2.0 >=  2.38, gobject-2.0 >=  2.38, gio-2.0 >=  2.38
>     Requires.private: libxml-2.0, sqlite3, libpsl >=  0.20, libbrotlidec, zlib
>
> (To be precise, glib uses utillinuxMinimal rather than utillinux.)
>
> The warnings we see, such as Package 'mount', required by 'gio-2.0',
> not found, come from CMake running both pkg-config --libs and
> pkg-config --static --libs to populate both <XXX>_LIBRARIES and
> <XXX>_STATIC_LIBRARIES[1], but dino has no use for the latter.
> Currently these warnings can not be disabled:
> https://gitlab.kitware.com/cmake/cmake/issues/18158
>
> (They could be prevented by pruning Requires.private from shared-only
> libraries akin to <#51767>,
> although it can not be detected if a library is shared-only from the
> .pc file alone, and this is just a warning.)
>
> [1]: docs: https://cmake.org/cmake/help/v3.16/module/FindPkgConfig.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@orivej orivej orivej approved these changes

@Ericson2314 Ericson2314 Awaiting requested review from Ericson2314 Ericson2314 is a code owner

@matthewbauer matthewbauer Awaiting requested review from matthewbauer

@vcunat vcunat Awaiting requested review from vcunat

@globin globin Awaiting requested review from globin

Assignees
No one assigned
Labels
6.topic: stdenv Standard environment 8.has: documentation 10.rebuild-darwin: 501+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@pbogdan @hedning @nixos-discourse @Mic92 @orivej @vcunat @GrahamcOfBorg @orivej-nixos