staged-recipes icon indicating copy to clipboard operation
staged-recipes copied to clipboard

Added recipe for Perl module Moo and its deps

Open xileF1337 opened this issue 2 years ago • 10 comments

This PR adds recipes for Perl module Moo and its dependencies. The following recipes are provided:

perl-class-method-modifiers
perl-moo
perl-role-tiny
perl-sub-quote
perl-test-fatal

Checklist

  • [x] Title of this PR is meaningful: e.g. "Adding my_nifty_package", not "updated meta.yaml".
  • [x] License file is packaged (see here for an example).
  • [x] Source is from official source.
  • [x] Package does not vendor other packages. (If a package uses the source of another package, they should be separate packages or the licenses of all packages need to be packaged).
  • [x] If static libraries are linked in, the license of the static library is packaged.
  • [x] Package does not ship static libraries. If static libraries are needed, follow CFEP-18.
  • [x] Build number is 0.
  • [x] A tarball (url) rather than a repo (e.g. git_url) is used in your recipe (see here for more details).
  • [x] GitHub users listed in the maintainer section have posted a comment confirming they are willing to be listed there.
  • [x] When in trouble, please check our knowledge base documentation before pinging a team.

xileF1337 avatar Sep 30 '22 17:09 xileF1337

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipes/perl-class-method-modifiers, recipes/perl-moo, recipes/perl-role-tiny, recipes/perl-sub-quote, recipes/perl-test-fatal, recipes/perl-test-needs) and found some lint.

Here's what I've got...

For recipes/perl-class-method-modifiers:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-moo:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-role-tiny:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-sub-quote:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-test-fatal:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-test-needs:

  • Feedstock with the same name exists in conda-forge.

conda-forge-linter avatar Sep 30 '22 17:09 conda-forge-linter

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipes/perl-class-method-modifiers, recipes/perl-moo, recipes/perl-role-tiny, recipes/perl-sub-quote, recipes/perl-test-fatal) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipes/perl-class-method-modifiers:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-moo:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-role-tiny:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-sub-quote:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

For recipes/perl-test-fatal:

  • Recipe with the same name exists in bioconda: please discuss with @conda-forge/bioconda-recipes.

conda-forge-linter avatar Sep 30 '22 17:09 conda-forge-linter

@conda-forge/bioconda-recipes, I'm porting the recipes mentioned above to CondaForge if there are no objections from your side.

xileF1337 avatar Sep 30 '22 17:09 xileF1337

@conda-forge/help-perl please review!

xileF1337 avatar Sep 30 '22 17:09 xileF1337

@cbrueffer I added you as maintainer to all recipes above, is this okay for you? Also, I would appreciate your review ;-) Thank you so much!

xileF1337 avatar Sep 30 '22 17:09 xileF1337

@cbrueffer If you don't have time to review right now, that's ok, but could you please give your consent (or not!) for being a maintainer? Then someone else could also do the review ... Thank you!

xileF1337 avatar Oct 05 '22 11:10 xileF1337

Yes, I agree to be a maintainer. I'll try to look at this later today or tomorrow.

cbrueffer avatar Oct 06 '22 09:10 cbrueffer

Technically the recipes look OK (see questions on the commented out dependencies), I've added some comments on how to improve them on a style level to improve consistency. I've only made those on the first recipe, but they apply to all recipes in the PR. Here's an example of a (at least in my view) well-formatted recipe: https://github.com/conda-forge/perl-scalar-list-utils-feedstock/blob/main/recipe/meta.yaml

Generally PRs with only one recipe are much more motivating to review than large PRs; something to keep in mind to ensure quick reviews.

cbrueffer avatar Oct 08 '22 16:10 cbrueffer

Technically the recipes look OK (see questions on the commented out dependencies), I've added some comments on how to improve them on a style level to improve consistency. I've only made those on the first recipe, but they apply to all recipes in the PR. Here's an example of a (at least in my view) well-formatted recipe: https://github.com/conda-forge/perl-scalar-list-utils-feedstock/blob/main/recipe/meta.yaml

Thanks for for your feedback, @cbrueffer. Unfortunately I don't see any comments in the files section, am I blind or did you miss to submit something? Since you explicitly mentioned the commented out deps, I comment on this: they were generated by conda skeleton and I assume the background is that it is assumed that all Perl recipes define themselves as weak run-export, i.e. when such packages are used as a build-time dep, they are automatically assumed to be a run-time dep as well. Therefore, they are not explicitly required there. I now removed them completely and everything still works nicely. I hope that we are ready to merge now!

Generally PRs with only one recipe are much more motivating to review than large PRs; something to keep in mind to ensure quick reviews.

Okay, well, it is much more efficient to prepare a bunch of recipes when the actual target recipe has dependencies which are not yet provided by conda-forge. I could factor out individual recipes, but that's quite some effort (testing each of them again, single recipe tests take much longer in total than a bulk of recipes in one build). I have another recipe in preparation that ports Moose to conda-forge and it has some 30 recipes in total. Do you really think that 30 individual PRs would be easier to review? Also it would take very long until all dependencies are finally available for the recipes closer to the root of the dependency tree. What do you think?

xileF1337 avatar Oct 11 '22 15:10 xileF1337

Seems I left the comments pending and forgot to actually submit the review, sorry about that. Yeah, my question was around the commented out dependencies; I think simply removing them is better.

As for the size of PRs, yeah, 30 dependencies is a lot. My point was just that the bigger the PR, the more motivation needed to actually dig through it (at least for me, YMMV). If the recipes are in good shape it removes some of the pain though; how about at least chunking it into 3 x 10? You can also add the build instructions directly into meta.yaml, that removes 2/3 of the files to review directly.

cbrueffer avatar Oct 13 '22 20:10 cbrueffer

@cbrueffer Fixed the issues you mentioned. Good catch with the shebang lines, I removed them when filtering out comments -- stupid me. Concerning the idea of moving the build script into meta.yaml: I will look into that; I have to check the module builder in my scripts (ExtUtils::MakeMaker or Module::Build) to get the code right, but could add that to my scripts. I would do that for future recipes, though. Another thought: what about Windows support? Currently, we don't have a Perl package for Windows, but if that becomes available again at some point in the future, having bothbld.bat and build.sh in place would make supporting it easier, wouldn't it?

Could we merge this now? I need some of the packages as deps for other recipes ;-) Thank you!

xileF1337 avatar Oct 18 '22 22:10 xileF1337

The bld.bat files still have EOF whitespace but I'm merging it anyway now. Regarding Windows, I think the inline build instructions should work regardless. There are cases where something additional needs to be done during the build, but those could always have separate build files. I'm happy to merge Windows builds, but personally I don't care about it; my main concern is moving Perl out of Bioconda.

cbrueffer avatar Oct 21 '22 08:10 cbrueffer