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

perlPackages.AppMusicChordPro: init at 0.977 #100295

Merged
merged 5 commits into from Oct 14, 2020
Merged

perlPackages.AppMusicChordPro: init at 0.977 #100295

merged 5 commits into from Oct 14, 2020

Conversation

tdroxler
Copy link
Contributor

Motivation for this change

ChordPro was missing

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.

@tdroxler tdroxler changed the title Add missing ChordPro perlPackages.ChordPro: init at 0.974 Oct 12, 2020
stigtsp
stigtsp requested changes Oct 12, 2020
View reviewed changes
Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Hi @tdroxler and welcome to the NixOS project!

0.977 is available on CPAN, so please update to the latest version. I've added a few comments to this review.

pkgs/top-level/perl-packages.nix Outdated
@@ -2487,6 +2500,21 @@ let
};
};

ChordPro = buildPerlPackage {
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
ChordPro = buildPerlPackage {
AppMusicChordPro = buildPerlPackage {

The attribute name should be AppMusicChordPro

pkgs/top-level/perl-packages.nix Outdated
@@ -2487,6 +2500,21 @@ let
};
};

ChordPro = buildPerlPackage {
pname = "App-Music-ChordPro";
version = "0.974";
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
version = "0.974";
version = "0.977";

0.977 is latest on CPAN

pkgs/top-level/perl-packages.nix Outdated
Comment on lines 2507 to 2508
url = mirror://cpan/authors/id/J/JV/JV/App-Music-ChordPro-0.974.tar.gz;
sha256 = "7fae50535dcb7f68171a8c1f8ad49b13563f406fb1d44eba78b349e7ef83a8af";
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
url = mirror://cpan/authors/id/J/JV/JV/App-Music-ChordPro-0.974.tar.gz;
sha256 = "7fae50535dcb7f68171a8c1f8ad49b13563f406fb1d44eba78b349e7ef83a8af";
url = "mirror://cpan/authors/id/J/JV/JV/App-Music-ChordPro-0.977.tar.gz";
sha256 = "0ggip43cddi5f6rylb07f56dhkfhbcbm621lvcnjfadnn9lrbwqh";

pkgs/top-level/perl-packages.nix Outdated
};
propagatedBuildInputs = [ AppPackager FileLoadLines FontTTF IOString ImageInfo PDFAPI2 StringInterpolateNamed ];
meta = {
homepage = http://www.chordpro.org;
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
homepage = http://www.chordpro.org;
homepage = "http://www.chordpro.org";

Quote URLs per NixOS/rfcs#45

pkgs/top-level/perl-packages.nix Outdated
url = mirror://cpan/authors/id/J/JV/JV/App-Music-ChordPro-0.974.tar.gz;
sha256 = "7fae50535dcb7f68171a8c1f8ad49b13563f406fb1d44eba78b349e7ef83a8af";
};
propagatedBuildInputs = [ AppPackager FileLoadLines FontTTF IOString ImageInfo PDFAPI2 StringInterpolateNamed ];
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
propagatedBuildInputs = [ AppPackager FileLoadLines FontTTF IOString ImageInfo PDFAPI2 StringInterpolateNamed ];
buildInputs = [ PodParser ];
propagatedBuildInputs = [ AppPackager FileLoadLines IOString ImageInfo PDFAPI2 StringInterpolateNamed TextLayout Wx ];

PodParser is needed for compatibility with perl-5.32.0
Wx can be added for bin/wxchordpro support
FontTTF is already included by PDFAPI2
TextLayout is also needed (by 0.977), here is the derivation:

  TextLayout = buildPerlPackage {
    pname = "Text-Layout";
    version = "0.019";
    src = fetchurl {
      url = "mirror://cpan/authors/id/J/JV/JV/Text-Layout-0.019.tar.gz";
      sha256 = "a043f2a89e113b29c523a9efa71fa8398ed75edd482193901b38d08dd4a4108e";
    };
    buildInputs = [ PDFAPI2 ];
    meta = {
      description = "Pango style markup formatting";
      license = with stdenv.lib.licenses; [ artistic1 gpl1Plus ];
    };
  };

@stigtsp stigtsp requested a review from volth October 12, 2020 14:09
@tdroxler tdroxler force-pushed the feature/perl-packages-chordpro branch 3 times, most recently from 03901de to 564538a Compare October 14, 2020 05:27
@tdroxler
Copy link
Contributor Author

Hi @stigtsp thx for the review and the help.

I fixed the comments and I already squashed/rebased so it's clean.

Cheers

@tdroxler tdroxler changed the title perlPackages.ChordPro: init at 0.974 perlPackages.ChordPro: init at 0.977 Oct 14, 2020
@stigtsp
Copy link
Member

stigtsp commented Oct 14, 2020

Great, thx! Can you also change the commit/PR title from perlPackages.ChordPro: init at 0.977 to perlPackages.AppMusicChordPro: init at 0.977 so it matches the attribute?

@tdroxler tdroxler force-pushed the feature/perl-packages-chordpro branch from 564538a to 0bbcf36 Compare October 14, 2020 05:31
@stigtsp
Copy link
Member

stigtsp commented Oct 14, 2020

Also, can you move AppMusicChordPro so it's in alphabetical order in perl-packages.nix?

@tdroxler tdroxler changed the title perlPackages.ChordPro: init at 0.977 perlPackages.AppMusicChordPro: init at 0.977 Oct 14, 2020
@tdroxler tdroxler force-pushed the feature/perl-packages-chordpro branch from 0bbcf36 to aec71f1 Compare October 14, 2020 07:37
@tdroxler
Copy link
Contributor Author

done

@stigtsp
Copy link
Member

stigtsp commented Oct 14, 2020

@GrahamcOfBorg build perlPackages.AppMusicChordPro

stigtsp
stigtsp requested changes Oct 14, 2020
View reviewed changes
Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Found two more things:

  • url for AppMusicChordPro should be quoted
  • Breaks on Darwin due to: Wx not supporting the platform, and bin/chordpro needs shortenPerlShebang

pkgs/top-level/perl-packages.nix Outdated
pname = "App-Music-ChordPro";
version = "0.977";
src = fetchurl {
url = mirror://cpan/authors/id/J/JV/JV/App-Music-ChordPro-0.977.tar.gz;
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
url = mirror://cpan/authors/id/J/JV/JV/App-Music-ChordPro-0.977.tar.gz;
url = "mirror://cpan/authors/id/J/JV/JV/App-Music-ChordPro-0.977.tar.gz";

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/top-level/perl-packages.nix Outdated
sha256 = "0ggip43cddi5f6rylb07f56dhkfhbcbm621lvcnjfadnn9lrbwqh";
};
buildInputs = [ PodParser ];
propagatedBuildInputs = [ AppPackager FileLoadLines IOString ImageInfo PDFAPI2 StringInterpolateNamed TextLayout Wx ];
Copy link
Member

Choose a reason for hiding this comment

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

The following can be added to support darwin

Suggested change
propagatedBuildInputs = [ AppPackager FileLoadLines IOString ImageInfo PDFAPI2 StringInterpolateNamed TextLayout Wx ];
propagatedBuildInputs = [ AppPackager FileLoadLines IOString ImageInfo PDFAPI2 StringInterpolateNamed TextLayout ]
++ stdenv.lib.optional (!stdenv.isDarwin) [ Wx ];
nativeBuildInputs = stdenv.lib.optional stdenv.isDarwin shortenPerlShebang;
postInstall = stdenv.lib.optionalString stdenv.isDarwin ''
shortenPerlShebang $out/bin/chordpro
rm $out/bin/wxchordpro # Wx not supported on darwin
'';

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, thx again for the help.

@tdroxler tdroxler force-pushed the feature/perl-packages-chordpro branch from aec71f1 to d8ac42b Compare October 14, 2020 09:34
stigtsp
stigtsp reviewed Oct 14, 2020
View reviewed changes
pkgs/top-level/perl-packages.nix Outdated
Comment on lines 610 to 614
nativeBuildInputs = stdenv.lib.optional stdenv.isDarwin shortenPerlShebang;
postInstall = stdenv.lib.optionalString stdenv.isDarwin ''
shortenPerlShebang $out/bin/chordpro
rm $out/bin/wxchordpro # Wx not supported on darwin
'';
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, the indentation on these lines are a bit off. Should align with propagatedBuildInputs above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

np, it's done

@tdroxler tdroxler force-pushed the feature/perl-packages-chordpro branch from d8ac42b to f37f538 Compare October 14, 2020 10:23
@stigtsp
Copy link
Member

stigtsp commented Oct 14, 2020

@GrahamcOfBorg build perlPackages.AppMusicChordPro perldevelPackages.AppMusicChordPro

stigtsp
stigtsp approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@stigtsp stigtsp left a comment

Choose a reason for hiding this comment

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

Looks good to me, thx for the contribution

Build and tested OK on linux x64 and darwin x64

Result of nixpkgs-review pr 100295 1

10 packages built:
  • perl530Packages.AppMusicChordPro
  • perl530Packages.AppPackager
  • perl530Packages.FileLoadLines
  • perl530Packages.StringInterpolateNamed
  • perl530Packages.TextLayout
  • perl532Packages.AppMusicChordPro
  • perl532Packages.AppPackager
  • perl532Packages.FileLoadLines
  • perl532Packages.StringInterpolateNamed
  • perl532Packages.TextLayout

@stigtsp stigtsp merged commit f25cdd4 into NixOS:master Oct 14, 2020
@tdroxler
Copy link
Contributor Author

thx for your help.

@tdroxler tdroxler deleted the feature/perl-packages-chordpro branch October 14, 2020 12:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stigtsp stigtsp stigtsp approved these changes

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@tdroxler @stigtsp @Janik-Haag