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.

Sign up for GitHub

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

Reduce closure gnome-shell closure #77835

Merged
merged 7 commits into from
Jan 16, 2020
Merged

Reduce closure gnome-shell closure #77835

merged 7 commits into from
Jan 16, 2020

Conversation

hedning
Copy link
Contributor

@hedning hedning commented Jan 16, 2020

Motivation for this change
Things done

Adds a dev output to a few gnome-shell dependencies (and gtkd), preventing their propagated build inputs becoming part of eg. gnome-shell's closure. Specifically reduces gnome-shell's closure size from 1.6GB to 1.1GB.

It seems like it's very common for derivations to specify propagated build inputs without having an extra output, so there's probably some really low hanging fruits here. (Might honestly be good for the propagated-build-inputs files to get a dedicated output which is automatically added?).

  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

worldofpeace
worldofpeace reviewed Jan 16, 2020
View reviewed changes
pkgs/development/libraries/gtkd/default.nix
Comment on lines +124 to +129
for pc in $dev/lib/pkgconfig/*; do
substituteInPlace $pc \
--replace "$out/include" "$dev/include"
done
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't the multiple outputs setup hook do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like that only works on lines starting with includedir=, but the gtkd pc files doesn't have any variables unfortunately.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah right, so it's cflags and libs.
Can you comment this? I believe in the past gtkd's .pc files have gone through some problematic issues.

@worldofpeace
Copy link
Contributor

Aside from the other comments, this should be harmless.

@ofborg ofborg bot requested review from worldofpeace and jtojnar January 16, 2020 17:41
jtojnar
jtojnar approved these changes Jan 16, 2020
View reviewed changes
Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

LGTM

@ofborg ofborg bot requested a review from jtojnar January 16, 2020 18:36
@FRidh
Copy link
Member

FRidh commented Jan 16, 2020

It seems like it's very common for derivations to specify propagated build inputs without having an extra output, so there's probably some really low hanging fruits here. (Might honestly be good for the propagated-build-inputs files to get a dedicated output which is automatically added?).

As soon as we have structured outputs, we can disallow references per output NixOS/nix@3cd15c5 and #49417

worldofpeace
worldofpeace approved these changes Jan 16, 2020
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I've built the relevant packages on staging (including gnome-shell)
NP, merging 👍

@worldofpeace worldofpeace merged commit c8c87f6 into NixOS:staging Jan 16, 2020
@worldofpeace
Copy link
Contributor

Also verified the closure size is reduced as reported.

@alyssais alyssais added the 6.topic: closure size The final size of a derivation, including its dependencies label Jan 17, 2020
@hedning hedning deleted the reduce-closure branch January 17, 2020 12:06
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Jan 21, 2020
@worldofpeace @dtzWill
Merge pull request NixOS#77835 from hedning/reduce-closure
8f842e6 
Reduce closure gnome-shell closure

(cherry picked from commit c8c87f6)
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/january-2020-in-nixos/5771/1

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

@worldofpeace worldofpeace worldofpeace approved these changes

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
6.topic: closure size The final size of a derivation, including its dependencies 10.rebuild-darwin: 11-100 10.rebuild-linux: 501-1000 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hedning @worldofpeace @FRidh @nixos-discourse @jtojnar @alyssais