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

doc: add section on running ad-hoc Perl programs to the nixpkgs manual #86589

Merged
merged 1 commit into from Jul 17, 2020
Merged

doc: add section on running ad-hoc Perl programs to the nixpkgs manual #86589

merged 1 commit into from Jul 17, 2020

Conversation

raboof
Copy link
Member

@raboof raboof commented May 2, 2020

Motivation for this change
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.

@raboof raboof changed the title Add section on running ad-hoc Perl programs to the nixpkgs manual doc: add section on running ad-hoc Perl programs to the nixpkgs manual May 2, 2020
@raboof
Copy link
Member Author

raboof commented Jul 7, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2 marvin-mk2 bot added the marvin label Jul 7, 2020
@marvin-mk2
Copy link

marvin-mk2 bot commented Jul 7, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@timokau
Copy link
Member

timokau commented Jul 8, 2020

Looks like I missed one bot error due to the issue yesterday.
/status needs_reviewer

@marvin-mk2 marvin-mk2 bot requested a review from fgaz July 8, 2020 21:20
Mic92
Mic92 reviewed Jul 8, 2020
View reviewed changes
doc/languages-frameworks/perl.xml
<programlisting>
foo = import ../path/to/foo.nix {
inherit stdenv fetchurl ...;
inherit (perlPackages) ClassC3;
Copy link
Member

Choose a reason for hiding this comment

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

We don't have a perlPackages.callPackage scope like we do have for python?

Copy link
Member

Choose a reason for hiding this comment

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

Actually we do...

foo = perlPackages.callPackage ./path/to/foo.nix {};

should fill out:

{ ClassC3 }:
# ...

Copy link
Member

Choose a reason for hiding this comment

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

I am not actually sure which section was actually changed so I read the whole article.

Copy link
Member Author

@raboof raboof Jul 16, 2020 •
edited

Choose a reason for hiding this comment

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

This section was indeed unchanged in this PR, just indented.

If you're using the github web UI, you can drastically simplify the diff by using the 'ignore whitespace changes' feature
Screenshot_2020-07-16_17-33-57

@marvin-mk2 marvin-mk2 bot requested a review from timokau July 14, 2020 02:49
timokau
timokau requested changes Jul 14, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

See @Mic92's feedback.

@raboof raboof requested review from Mic92 and timokau July 16, 2020 15:35
timokau
timokau requested changes Jul 16, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

All in all this section is not really perl specific. But I suppose it can't hurt to have a section in the perl docs, if the pattern is particularly common there.

doc/languages-frameworks/perl.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/perl.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/perl.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/perl.xml Show resolved Hide resolved
@timokau
Copy link
Member

timokau commented Jul 16, 2020

I don't understand where all the unrelated whitespace changes come from.

@raboof
Copy link
Member Author

raboof commented Jul 16, 2020

I don't understand where all the unrelated whitespace changes come from.

The 'Perl' section used to consist of a number of sections on different ways of packaging Perl applications.

In this PR, I put those under a 'Packaging Perl programs' section (added a 'Running Perl programs' section before it). Because of that, all the sections on packaging were indented 1 level.

@raboof raboof force-pushed the document-running-perl-scripts-from-the-shell branch from 3e64e99 to 0c8047a Compare July 16, 2020 16:19
@raboof raboof requested a review from timokau July 16, 2020 16:22
timokau
timokau requested changes Jul 16, 2020
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

Ah, I missed that the "Packaging" part was top-level before. Two more small nitpicks, one of which is on my own suggestion.

doc/languages-frameworks/perl.xml Outdated Show resolved Hide resolved
doc/languages-frameworks/perl.xml Outdated Show resolved Hide resolved
@raboof @timokau
Add section on running ad-hoc Perl programs to the nixpkgs manual
6093372 
Co-authored-by: Timo Kaufmann <timokau@zoho.com>
@raboof raboof force-pushed the document-running-perl-scripts-from-the-shell branch from 0c8047a to 6093372 Compare July 17, 2020 07:55
@raboof raboof requested a review from timokau July 17, 2020 07:57
@Mic92 Mic92 merged commit 3829979 into NixOS:master Jul 17, 2020
@timokau
Copy link
Member

timokau commented Jul 17, 2020

Thanks!

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

@fgaz fgaz Awaiting requested review from fgaz

@Mic92 Mic92 Awaiting requested review from Mic92

@timokau timokau Awaiting requested review from timokau

Assignees
No one assigned
Labels
8.has: documentation 10.rebuild-darwin: 0 10.rebuild-linux: 0 awaiting_reviewer marvin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@raboof @timokau @Mic92