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

Add perl module Text::Fuzzy #25932

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Add perl module Text::Fuzzy #25932

wants to merge 16 commits into from

Conversation

lskatz
Copy link

@lskatz lskatz commented Apr 2, 2024

Checklist

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

@conda-forge-webservices
Copy link

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-text-fuzzy/0.29) and found some lint.

Here's what I've got...

For recipes/perl-text-fuzzy/0.29:

  • The recipe could do with some maintainers listed in the extra/recipe-maintainers section.
  • Please do not delete the example recipe found in recipes/example/meta.yaml.

For recipes/perl-text-fuzzy/0.29:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@lskatz
Copy link
Author

lskatz commented Apr 2, 2024

@conda-forge/help-perl I wanted to contribute this package but when I build it locally with conda-build ., it complains crypt.h. Could I have any advice or help? Thank you!

@lskatz
Copy link
Author

lskatz commented Apr 2, 2024

Note: this was originally submitted to bioconda but is probably more appropriate for conda-forge bioconda/bioconda-recipes#46913

@conda-forge-webservices
Copy link

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-text-fuzzy) and found some lint.

Here's what I've got...

For recipes/perl-text-fuzzy:

  • The recipe could do with some maintainers listed in the extra/recipe-maintainers section.

For recipes/perl-text-fuzzy:

  • License is not an SPDX identifier (or a custom LicenseRef) nor an SPDX license expression.

Documentation on acceptable licenses can be found here.

@conda-forge-webservices
Copy link

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-text-fuzzy) and found it was in an excellent condition.

@conda-forge-webservices
Copy link

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-text-fuzzy) and found some lint.

Here's what I've got...

For recipes/perl-text-fuzzy:

  • The top level meta key noarch_platforms is unexpected

@conda-forge-webservices
Copy link

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-text-fuzzy) and found some lint.

Here's what I've got...

For recipes/perl-text-fuzzy:

  • The top level meta key noarch_platforms is unexpected
  • The top level meta key target_platform is unexpected

@conda-forge-webservices
Copy link

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-text-fuzzy) and found some lint.

Here's what I've got...

For recipes/perl-text-fuzzy:

  • The top level meta key build_platform is unexpected
  • The top level meta key target_platform is unexpected

@conda-forge-webservices
Copy link

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-text-fuzzy) and found some lint.

Here's what I've got...

For recipes/perl-text-fuzzy:

  • noarch packages can't have selectors. If the selectors are necessary, please remove noarch: generic.

@conda-forge-webservices
Copy link

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-text-fuzzy) and found it was in an excellent condition.

@lskatz
Copy link
Author

lskatz commented Apr 2, 2024

I am not clear on how to limit the build to just Linux

@lskatz
Copy link
Author

lskatz commented Apr 2, 2024

I am not clear on how to limit the build to just Linux

@conda-forge/help-perl I am almost ready for review but I have no idea how to just limit the test to Linux. Thank you for any help. Additionally, when I build this locally with conda-build ., it fails with

lib/5.26.2/x86_64-linux-thread-multi/CORE/reentr.h:104:17: fatal error: crypt.h: No such file or directory
  104 | #       include <crypt.h>
      |                 ^~~~~~~~~
compilation terminated.
make: *** [Makefile:338: Fuzzy.o] Error 1

I am not sure if this is an actual issue since it seems to build in the CI environment.

@lskatz
Copy link
Author

lskatz commented Apr 2, 2024

I see some guidance in #25110 but I don't think it completely addresses my issues.

@jvolkening
Copy link
Contributor

Hi Lee,

To limit the build to just Linux, you need to add skip: true # [not linux] under build: in your meta.yaml. My personal opinion is that it is preferred to try to get the OSX and Windows builds to work, but admittedly this can be a challenge with the given state of Perl in conda or without access to those platforms for testing. A few other comments on your recipe:

  • The module is compiled so definitely not noarch -- you can remove that commented line completely.

  • The current policy in conda-forge is to use INSTALLDIRS=vendor in your build script rather than site. The full recommended argument string to Makefile.PL is INSTALLDIRS=vendor NO_PERLLOCAL=1 NO_PACKLIST=1.

  • All of the host and run requirements except for perl are unnecessary and should be removed. I believe many of the build requirements are also unnecessary -- see below.

  • The SPDX license string for most Perl packages that indicate "licensed under the same terms as Perl" or LICENSE => 'perl' (including Text::Fuzzy) should be Artistic-1.0-Perl OR GPL-1.0-or-later.

  • Conda-forge wants license_file specified under about:, so this file either needs to be packaged with your recipe or included in some other way. In my migrated recipes I've been packaging a LICENSE file when missing, but I got some push-back on this in one review, although I don't agree with the alternative suggested. For now I would suggest downloading the text of these licenses and including them in your recipe.

I've developed an alternative Perl recipe generator as part of the bioconda->conda-forge migration. In case it helps for comparison of requirements, etc, here is the recipe it generated for Text::Fuzzy which builds successfully under Linux and Windows (without license_file as it's still being worked out how best to handle that):

{% set version = "0.29" %}
{% set sha256 = "3df5cfd2ca1a4c5ca7ff7bab3cc8d53ad2064e134cbf11004f3cf8c4b9055bff" %}

# regex to use for Windows build hack
{% set win_patch = "s|C:\\\\strawberry\\\\c|$ENV{LIBRARY_PREFIX}\\\\mingw-w64|g" %}

package:
  name: perl-text-fuzzy
  version: {{ version }}

source:
  url: https://cpan.metacpan.org/authors/id/B/BK/BKB/Text-Fuzzy-{{ version }}.tar.gz
  sha256: {{ sha256 }}

build:
  number: 0
  # needed for linker to find some m2w64 libs
  merge_build_host: True  # [win]
  script:
    - >-
      perl Makefile.PL INSTALLDIRS=vendor NO_PERLLOCAL=1 NO_PACKLIST=1 MAKE=make
      && (for /r . %%f in (*Makefile) do perl -i -pe "{{ win_patch }}" %%f)  # [win]
      && make
      && make test
      && make install VERBINST=1

requirements:
  build:
    - {{ compiler('c') }}  # [unix]
    - m2w64-gcc # [win]
    - make
  host:
    - perl >= 5.32
  run:
    - perl >= 5.32

test:
  imports:
    - Text::Fuzzy
  source_files:
   - t
  commands:
   - prove

about:
  home: https://metacpan.org/pod/Text::Fuzzy
  summary: Partial string matching using edit distances
  license: Artistic-1.0-Perl OR GPL-1.0-or-later

extra:
  recipe-maintainers:
    - jvolkening
    - conda-forge/perl-packagers

@lskatz
Copy link
Author

lskatz commented Apr 9, 2024

Oh wow there is so much here that I didn't know. Thank you so much for your help.

Follow up questions:

  • Do I remove build.sh because now it is running Makefile.PL inside of the yaml?
  • Do I remove conda_build_config.yaml because now it works in multiple OSs?
  • Do you think I can reduce the perl version to something smaller like 5.16 to help with compatibility? Or I guess I can at least test it in the CI.
  • Should I put my github username in recipe-maintainers?
  • Do I literally copy and paste a license into license.txt and then point to it in license_file? Do I need a copy step or anything?

It seems to have built locally on my Linux computer and so I'm going to try it on CI!

@conda-forge-webservices
Copy link

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-text-fuzzy) and found some lint.

Here's what I've got...

For recipes/perl-text-fuzzy:

  • Selectors are suggested to take a <two spaces>#<one space>[<expression>] form. See lines [29]
  • There are 1 too many lines. There should be one empty line at the end of the file.
  • license_file entry is missing, but is required.
  • The following maintainers have not yet confirmed that they are willing to be listed here: conda-forge/perl-packagers. Please ask them to comment on this PR if they are.

@conda-forge-webservices
Copy link

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-text-fuzzy) and found some lint.

Here's what I've got...

For recipes/perl-text-fuzzy:

  • The following maintainers have not yet confirmed that they are willing to be listed here: conda-forge/perl-packagers. Please ask them to comment on this PR if they are.

@lskatz
Copy link
Author

lskatz commented Apr 9, 2024

@jvolkening it seems to pass CI! How do I follow through with your change to make yourself a maintainer? It says you have to comment? And then how do I trigger a merge?

@jvolkening
Copy link
Contributor

  • Do I remove build.sh because now it is running Makefile.PL inside of the yaml?

Yes, looks like you took care of this.

  • Do I remove conda_build_config.yaml because now it works in multiple OSs?

I've never made use of that file -- you shouldn't need it here I don't think.

  • Do you think I can reduce the perl version to something smaller like 5.16 to help with compatibility? Or I guess I can at least test it in the CI.

You can leave the version unspecified -- that's normally what I do but here for some reason the build test was pulling in an earlier Perl today which didn't work. In the 5.32 conda release the Windows build switched to using Strawberry Perl, and so the recipes are different to accommodate this -- the recipe I shared wouldn't build on earlier Windows conda releases. Actually, I have a PR in to the Perl feedstock to switch to a native Windows build, so if that ever gets merged things should be simpler.

  • Should I put my github username in recipe-maintainers?

Yes, I would swap mine with yours. You can leave mine in if you want, but I hope to be added to perl-packagers and then it won't matter.

  • Do I literally copy and paste a license into license.txt and then point to it in license_file? Do I need a copy step or anything?

How you did it looks good.

recipes/perl-text-fuzzy/meta.yaml Outdated Show resolved Hide resolved
recipes/perl-text-fuzzy/meta.yaml Show resolved Hide resolved
recipes/perl-text-fuzzy/LICENSE Show resolved Hide resolved
@jvolkening
Copy link
Contributor

it seems to pass CI! How do I follow through with your change to make yourself a maintainer? It says you have to comment? And then how do I trigger a merge?

I am okay with being included as a maintainer on this recipe. You will need to get someone from perl-packagers to comment as well. You will need someone with merge permissions for the final review and merge. Be prepared for a slow process here -- I have some outstanding PRs from back in January and February that are still waiting for final reviews. There just aren't that many people who are conda-forge admins and are willing to spend time on the Perl-related stuff.

I just did a review of the current commit, but I don't have the ability to merge.

"mfansler" and "xileF1337" have been helpful on my recipes recently. "mbargull" has done a fair amount of work on Perl recipes in the past few years. I would suggesting pinging them along with "perl-packagers" to ask for a review.

@conda-forge-webservices
Copy link

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-text-fuzzy) and found it was in an excellent condition.

@lskatz
Copy link
Author

lskatz commented Apr 10, 2024

it seems to pass CI! How do I follow through with your change to make yourself a maintainer? It says you have to comment? And then how do I trigger a merge?

I am okay with being included as a maintainer on this recipe. You will need to get someone from perl-packagers to comment as well. You will need someone with merge permissions for the final review and merge. Be prepared for a slow process here -- I have some outstanding PRs from back in January and February that are still waiting for final reviews. There just aren't that many people who are conda-forge admins and are willing to spend time on the Perl-related stuff.

I just did a review of the current commit, but I don't have the ability to merge.

"mfansler" and "xileF1337" have been helpful on my recipes recently. "mbargull" has done a fair amount of work on Perl recipes in the past few years. I would suggesting pinging them along with "perl-packagers" to ask for a review.

Thank you for all your help! It looks like it is passing linting. I didn't make any other major changes, and so I expect the rest of the tests to continue passing. I'll see what I can do about putting this recipe on my private channel and also how I might even make that private channel while I wait.

@lskatz
Copy link
Author

lskatz commented Apr 10, 2024

"mfansler" and "xileF1337" have been helpful on my recipes recently. "mbargull" has done a fair amount of work on Perl recipes in the past few years. I would suggesting pinging them along with "perl-packagers" to ask for a review.

@mfansler @xileF1337 @mbargull @conda-forge/help-perl I would like to merge this pull request. All checks have passed. Thank you for any assistance!

@lskatz
Copy link
Author

lskatz commented Apr 10, 2024

I have learned how to make my own channel and so I uploaded to -c lskatz, e.g., conda install -c lskatz perl-text-fuzzy in the meantime.

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

Successfully merging this pull request may close these issues.

None yet

2 participants