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

groff: split out perl dependencies #27305

Merged
merged 1 commit into from Nov 17, 2017
Merged

groff: split out perl dependencies #27305

merged 1 commit into from Nov 17, 2017

Conversation

therealpxc
Copy link
Contributor

Motivation for this change

This resolves issue #26892 , and results in a ~50MB closure reduction for some packages including the fish shell!

I tried at least running all of the executables that use perl, although I'm not really sure what inputs they use. I've grepped through the nixpkgs tree a little bit and none of the perl-dependent executables are directly referenced in any packages, although they may be in use there. There seem to be several which only use groff's nroff command, which means this PR will benefit those packages w/r/t closure reduction as well.

I will try to do some more testing this week of the individual perl-dependent groff executables to make sure their output in some simple cases is complete and correct; I'm not sure that I've adequately tested this but would like to put it up at this point for advice nonetheless.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • 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/)
  • Fits CONTRIBUTING.md.

@therealpxc
Copy link
Contributor Author

(I'm now running nox-review pr 27305 to review this PR, but will probably take a long time as many packages depend on groff one way or another and this machine is not the fastest.)

@Mic92
Copy link
Member

Mic92 commented Jul 11, 2017

This is a mass-rebuild please re-target the branch against staging.

@therealpxc therealpxc changed the base branch from master to staging July 11, 2017 18:33
@therealpxc
Copy link
Contributor Author

What do you good folks need from me next, if anything?

@knedlsepp
Copy link
Member

@therealpxc: The re-targeting lead to some unrelated commits being included in the PR.

@Mic92
Copy link
Member

Mic92 commented Jul 12, 2017

@therealpxc I can remove the unrelated commits in the pull request, if you want.

@therealpxc
Copy link
Contributor Author

therealpxc commented Jul 13, 2017 •
edited

Maybe I shouldn't have just done that in the GitHub web interface, lol. I didn't even notice that at first. Sorry, folks!

I've dropped those commits.

@orivej orivej merged commit f92f07f into NixOS:staging Nov 17, 2017
@vcunat
Copy link
Member

vcunat commented Nov 18, 2017 •
edited

There are just two packages in nixpkgs that use generic ${groff}/bin – man and ronn (they extend $PATH in wrappers). At least basic functionality of man seems still OK to me; for ronn /cc maintainer @zimbatm.

@orivej
Copy link
Contributor

orivej commented Nov 18, 2017

Tracing the executables on the current master, man uses preconv nroff grotty troff groff tbl and ronn uses grotty troff groff from the groff package, all of which are in the default output.

@Profpatsch Profpatsch mentioned this pull request Dec 30, 2018
Closed
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

Assignees
No one assigned
Labels
1.severity: mass-rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@therealpxc @Mic92 @knedlsepp @vcunat @orivej @GrahamcOfBorg