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

iwd: don't include testing tools by default #105559

Closed
wants to merge 1 commit into from
Closed

iwd: don't include testing tools by default #105559

wants to merge 1 commit into from

Conversation

xaverdh
Copy link
Contributor

@xaverdh xaverdh commented Dec 1, 2020

Hide the testing tools behind a flag. They blow up the closure
size by a huge factor.

Motivation for this change

Reduce closure size from ~250M to 40M

Things done
  • 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.

This was referenced Dec 1, 2020
Closed
Closed
@ofborg ofborg bot requested review from fpletz and dtzWill December 1, 2020 09:21
SuperSandro2000
SuperSandro2000 reviewed Dec 1, 2020
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

otherwise looks good to me.

pkgs/os-specific/linux/iwd/default.nix Outdated
@@ -60,8 +63,9 @@ stdenv.mkDerivation rec {

doCheck = true;

postInstall = ''
postInstall = lib.optionalString withTestingTools ''
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
postInstall = lib.optionalString withTestingTools ''
postInstall = stdenv.lib.optionalString withTestingTools ''

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pkgs/os-specific/linux/iwd/default.nix Outdated
@@ -1,4 +1,5 @@
{ stdenv
, lib
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
, lib

@SuperSandro2000
Copy link
Member

Result of nixpkgs-review pr 105559 run on x86_64-linux 1

1 package built:
  • iwd

@SuperSandro2000
Copy link
Member

Should this be enabled for https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/networking/iwd.nix?

@xaverdh
iwd: don't include testing tools by default
711ca90 
Hide the testing tools behind a flag. They blow up the closure
size by a huge factor.
@xaverdh
Copy link
Contributor Author

xaverdh commented Dec 1, 2020

Should this be enabled for https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/services/networking/iwd.nix?

Yes I don't see any reason, why the module (or any ordinary user) would need the testing tools.
I am currently running my system from unstable-small with this pr on top.

@jtojnar
Copy link
Contributor

jtojnar commented Dec 2, 2020

Would it make sense to install them to a separate output instead?

@xaverdh
Copy link
Contributor Author

xaverdh commented Dec 2, 2020 •
edited

Would it make sense to install them to a separate output instead?

Well the build takes just a few seconds on my mediocre system, but I can do that if its preferred.
What would be a good name for that output, just "test"?

edit: pr is here: #105666

@xaverdh xaverdh mentioned this pull request Dec 2, 2020
Merged
10 tasks
@xaverdh
Copy link
Contributor Author

xaverdh commented Dec 2, 2020

closing in favor of #105666

@xaverdh xaverdh closed this Dec 2, 2020
@xaverdh xaverdh deleted the minimal-iwd branch December 4, 2020 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@fpletz fpletz Awaiting requested review from fpletz

@dtzWill dtzWill Awaiting requested review from dtzWill

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@xaverdh @SuperSandro2000 @jtojnar