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 soxr recipe #17935

Merged
merged 31 commits into from
Apr 7, 2022
Merged

Add soxr recipe #17935

merged 31 commits into from
Apr 7, 2022

Conversation

HosameldinMohamed
Copy link
Contributor

@HosameldinMohamed HosameldinMohamed commented Feb 3, 2022

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.
  • 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-linter
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/soxr) and found some lint.

Here's what I've got...

For recipes/soxr:

  • The home item is expected in the about section.
  • The recipe must have some tests.
  • There are too few lines. There should be one empty line at the end of the file.
  • license_file entry is missing, but is required.

For recipes/soxr:

  • 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-linter
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/soxr) and found some lint.

Here's what I've got...

For recipes/soxr:

  • The home item is expected in the about section.
  • The recipe must have some tests.
  • license_file entry is missing, but is required.

@conda-forge-linter
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/soxr) and found some lint.

Here's what I've got...

For recipes/soxr:

  • The home item is expected in the about section.
  • The recipe must have some tests.

@conda-forge-linter
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/soxr) and found some lint.

Here's what I've got...

For recipes/soxr:

  • The recipe must have some tests.

@conda-forge-linter
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/soxr) and found it was in an excellent condition.

@conda-forge-linter
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/soxr) and found some lint.

Here's what I've got...

For recipes/soxr:

  • The top level meta key host is unexpected

@conda-forge-linter
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/soxr) and found it was in an excellent condition.

@HosameldinMohamed
Copy link
Contributor Author

Enabled OpenMP on linux and macOS by adding the libgomp and llvm-openmp packages to the build section of the meta.yaml file

https://conda-forge.org/docs/maintainer/knowledge_base.html#openmp

@conda-forge-linter
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/focal-stats) and found some lint.

Here's what I've got...

For recipes/focal-stats:

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

@conda-forge-linter
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/soxr) and found it was in an excellent condition.

@HosameldinMohamed
Copy link
Contributor Author

It fails because it doesn't find the LICENSE file.

https://dev.azure.com/conda-forge/feedstock-builds/_build/results?buildId=479682&view=logs&j=6f142865-96c3-535c-b7ea-873d86b887bd&t=22b0682d-ab9e-55d7-9c79-49f3c3ba4823&l=1537

In meta.yaml I put

  license_file: ${PREFIX}/share/doc/libsoxr/LICENCE

Because I saw the LICENSE file is installed in <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/LICENCE

@HosameldinMohamed
Copy link
Contributor Author

Same problem is related to the test files, here are the files I need to test: (for linux)

    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/README  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/LICENCE  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/NEWS  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/lib/libsoxr.so.0.1.2  # [linux]
    - test -f <CMAKE_INSTALL_PREFIX>/lib/libsoxr.so.0  # [linux]
    - test -f <CMAKE_INSTALL_PREFIX>/lib/libsoxr.so  # [linux]
    - test -f <CMAKE_INSTALL_PREFIX>/include/soxr.h  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/lib/libsoxr-lsr.so.0.1.9  # [linux]
    - test -f <CMAKE_INSTALL_PREFIX>/lib/libsoxr-lsr.so.0  # [linux]
    - test -f <CMAKE_INSTALL_PREFIX>/lib/libsoxr-lsr.so  # [linux]
    - test -f <CMAKE_INSTALL_PREFIX>/include/soxr-lsr.h  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/examples/1-single-block.c  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/examples/1a-lsr.c  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/examples/2-stream.C  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/examples/3-options-input-fn.c  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/examples/4-split-channels.c  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/examples/5-variable-rate.c  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/examples/examples-common.h  # [not win]
    - test -f <CMAKE_INSTALL_PREFIX>/share/doc/libsoxr/examples/README  # [not win]

@conda-forge-linter
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/soxr) and found it was in an excellent condition.

@HosameldinMohamed
Copy link
Contributor Author

Just FYI you can also use Ninja on Windows for uniformity.

At the end I compiled Windows and non-windows with make.

@HosameldinMohamed
Copy link
Contributor Author

@conda-forge/staged-recipes, ready for review, thanks.

@traversaro
Copy link
Contributor

@conda-forge/staged-recipes, ready for review, thanks.

You are a new user so you can't mention, I will give a first round of review and then mention it for you.

recipes/soxr/bld.bat Outdated Show resolved Hide resolved
recipes/soxr/build.sh Outdated Show resolved Hide resolved
@HosameldinMohamed
Copy link
Contributor Author

@traversaro I addressed your comments!

@traversaro
Copy link
Contributor

Thanks!

https://github.com/orgs/conda-forge/teams/staged-recipes @conda-forge/help-c-cpp The recipe is ready for review.

@carterbox carterbox added the c-cpp label Apr 6, 2022
Copy link
Member

@carterbox carterbox left a comment

Choose a reason for hiding this comment

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

Conda thinks that you aren't dynamically linking against tinyxml or libstdcxx-ng/libcxx/vsruntime for each of the three builds. Is that the case? It doesn't seem like tinyxml is a header-only library, so conda should be able to detect the links, and you shouldn't be statically linking to tinyxml. The runtime libraries you can add to the ignore_run_exports section because you may not be using the standard libraries.

recipes/soxr/meta.yaml Outdated Show resolved Hide resolved
HosameldinMohamed and others added 2 commits April 7, 2022 15:32
`not win` -> `unix`

Co-authored-by: Daniel Ching <carterbox@users.noreply.github.com>
@conda-forge-linter
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/soxr) and found some lint.

Here's what I've got...

For recipes/soxr:

  • Failed to even lint the recipe, probably because of a conda-smithy bug 😢. This likely indicates a problem in your meta.yaml, though. To get a traceback to help figure out what's going on, install conda-smithy and run conda smithy recipe-lint . from the recipe directory.

@conda-forge-linter
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/soxr) and found it was in an excellent condition.

@HosameldinMohamed
Copy link
Contributor Author

Conda thinks that you aren't dynamically linking against tinyxml or libstdcxx-ng/libcxx/vsruntime for each of the three builds. Is that the case? It doesn't seem like tinyxml is a header-only library, so conda should be able to detect the links, and you shouldn't be statically linking to tinyxml. The runtime libraries you can add to the ignore_run_exports section because you may not be using the standard libraries.

Thanks @carterbox for your checks.

I added

  ignore_run_exports:
    - libstdc++

But the warning is still persisting, I'll try to ignore the correct package.

I also realized that tinyxml is not required at all! My bad!

@traversaro

@carterbox
Copy link
Member

  ignore_run_exports_from:
    - {{ compiler('cxx') }}

I recently learned about ignore_run_exports_from. You could try that. It may be more robust.

@isuruf
Copy link
Member

isuruf commented Apr 7, 2022

Why not remove the C++ compiler from build instead?

requirements:
build:
- cmake
- {{ compiler('cxx') }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- {{ compiler('cxx') }}

As suggested by @isuruf

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 in e315dc9

@traversaro
Copy link
Contributor

@traversaro

See https://github.com/conda-forge/staged-recipes/pull/17935/files#r845431275 .

@carterbox carterbox merged commit 2fee8f6 into conda-forge:main Apr 7, 2022
@HosameldinMohamed
Copy link
Contributor Author

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

5 participants