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

stdenv: prune libtool files by default #51767

Merged
merged 5 commits into from Jan 11, 2019

Conversation

@pbogdan
Copy link
Contributor

pbogdan commented Dec 9, 2018

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

Copy link
Contributor

orivej left a comment

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

This comment has been minimized.

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 referenced this pull request Dec 9, 2018

Merged

iptables: prune libtool files after build #51787

3 of 10 tasks complete

@pbogdan pbogdan requested a review from matthewbauer as a code owner Dec 10, 2018

@pbogdan pbogdan changed the title pruneLibtoolFiles: more permissive regex for detecting libtool files stdenv: prune libtool files by default Dec 10, 2018

@pbogdan

This comment has been minimized.

Copy link
Contributor

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

@nixos-discourse

This comment has been minimized.

Copy link

nixos-discourse commented Dec 20, 2018

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 Dec 21, 2018

@Mic92

This comment has been minimized.

Copy link
Contributor

Mic92 commented Dec 21, 2018

@matthewbauer I wonder if this would break staticPkgs?

@hedning

This comment has been minimized.

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

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

9 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
@orivej

This comment has been minimized.

Copy link
Contributor

orivej commented Jan 11, 2019

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

@pbogdan

This comment has been minimized.

Copy link
Contributor

pbogdan commented Jan 11, 2019

Thank you @orivej!

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