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

perlPackages.AppMusicChordPro: init at 0.977 #100295

Merged
merged 5 commits into from Oct 14, 2020

Conversation

@tdroxler
Copy link
Contributor

@tdroxler tdroxler commented Oct 12, 2020

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 requested a review from stigtsp as a code owner Oct 12, 2020
@tdroxler tdroxler changed the title Add missing ChordPro perlPackages.ChordPro: init at 0.974 Oct 12, 2020
Copy link
Member

@stigtsp stigtsp left a comment

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.

@@ -2487,6 +2500,21 @@ let
};
};

ChordPro = buildPerlPackage {
Copy link
Member

@stigtsp stigtsp Oct 12, 2020

Suggested change
ChordPro = buildPerlPackage {
AppMusicChordPro = buildPerlPackage {

The attribute name should be AppMusicChordPro

@@ -2487,6 +2500,21 @@ let
};
};

ChordPro = buildPerlPackage {
pname = "App-Music-ChordPro";
version = "0.974";
Copy link
Member

@stigtsp stigtsp Oct 12, 2020

Suggested change
version = "0.974";
version = "0.977";

0.977 is latest on CPAN

url = mirror://cpan/authors/id/J/JV/JV/App-Music-ChordPro-0.974.tar.gz;
sha256 = "7fae50535dcb7f68171a8c1f8ad49b13563f406fb1d44eba78b349e7ef83a8af";
Copy link
Member

@stigtsp stigtsp Oct 12, 2020

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";

};
propagatedBuildInputs = [ AppPackager FileLoadLines FontTTF IOString ImageInfo PDFAPI2 StringInterpolateNamed ];
meta = {
homepage = http://www.chordpro.org;
Copy link
Member

@stigtsp stigtsp Oct 12, 2020

Suggested change
homepage = http://www.chordpro.org;
homepage = "http://www.chordpro.org";

Quote URLs per NixOS/rfcs#45

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

@stigtsp stigtsp Oct 12, 2020

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 Oct 12, 2020
@tdroxler tdroxler force-pushed the feature/perl-packages-chordpro branch 3 times, most recently from 03901de to 564538a Oct 14, 2020
@tdroxler
Copy link
Contributor Author

@tdroxler tdroxler commented Oct 14, 2020

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 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 Oct 14, 2020
@stigtsp
Copy link
Member

@stigtsp 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 Oct 14, 2020
@tdroxler
Copy link
Contributor Author

@tdroxler tdroxler commented Oct 14, 2020

done

@stigtsp
Copy link
Member

@stigtsp stigtsp commented Oct 14, 2020

@GrahamcOfBorg build perlPackages.AppMusicChordPro

Copy link
Member

@stigtsp stigtsp left a comment

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

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

@stigtsp stigtsp Oct 14, 2020

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

@tdroxler tdroxler Oct 14, 2020

done

sha256 = "0ggip43cddi5f6rylb07f56dhkfhbcbm621lvcnjfadnn9lrbwqh";
};
buildInputs = [ PodParser ];
propagatedBuildInputs = [ AppPackager FileLoadLines IOString ImageInfo PDFAPI2 StringInterpolateNamed TextLayout Wx ];
Copy link
Member

@stigtsp stigtsp Oct 14, 2020

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

@tdroxler tdroxler Oct 14, 2020

done, thx again for the help.

@tdroxler tdroxler force-pushed the feature/perl-packages-chordpro branch from aec71f1 to d8ac42b Oct 14, 2020
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

@stigtsp stigtsp Oct 14, 2020

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

Copy link
Contributor Author

@tdroxler tdroxler Oct 14, 2020

np, it's done

@tdroxler tdroxler force-pushed the feature/perl-packages-chordpro branch from d8ac42b to f37f538 Oct 14, 2020
@stigtsp
Copy link
Member

@stigtsp stigtsp commented Oct 14, 2020

@GrahamcOfBorg build perlPackages.AppMusicChordPro perldevelPackages.AppMusicChordPro

Copy link
Member

@stigtsp stigtsp left a comment

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
16 of 18 checks passed
@tdroxler
Copy link
Contributor Author

@tdroxler tdroxler commented Oct 14, 2020

thx for your help.

@tdroxler tdroxler deleted the feature/perl-packages-chordpro branch Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants