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

fix cmake installation #84

Merged
merged 5 commits into from
Aug 8, 2022
Merged

Conversation

mwittgen
Copy link

@mwittgen mwittgen commented Aug 4, 2022

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

@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 (recipe) and found some lint.

Here's what I've got...

For recipe:

  • pin_subpackage should be used instead of pin_compatible for fftw because it is one of the known outputs of this recipe: ['fftw'].

@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 (recipe) and found it was in an excellent condition.

@mwittgen
Copy link
Author

mwittgen commented Aug 5, 2022

cmake find_package does not work with autotool installation
FFTW/fftw3#130
This is a workaround to add the missing cmake file copied from a cmake build.

@egpbos
Copy link
Contributor

egpbos commented Aug 5, 2022

Thanks for this contribution @mwittgen!

How did you generate this file? Since it looks a bit different from the example in the thread you linked to, have you checked that it works like this? Is it also possible to dynamically generate the file in the recipe? That way we don't have to maintain this file.

It would be nice to have a simple test if it works; just a build against the installed FFTW3, using CMake for configuration. That would also catch situations when it needs to be updated.

If you don't have time for that, I'm okay with merging anyway, since I don't think it will hurt either way to add this file.

Copy link
Contributor

@egpbos egpbos left a comment

Choose a reason for hiding this comment

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

See my comments above.

@mwittgen
Copy link
Author

mwittgen commented Aug 5, 2022

The file was generated by obtaining the tarball and doing a full cmake/make install.
Just running cmake configuration does not generate the correct paths.
In principle this could be done in the recipe at the cost of adding a cmake build just to
generate one file. I can add an actual cmake build test to validate this, so far I only tested locally
by installing the patched fftw in my conda environment and building another project using the library.

@egpbos
Copy link
Contributor

egpbos commented Aug 5, 2022

That would be great! If you have time, of course. If not, then I would vote to merge anyway so that people can try it out. Do other reviewers have another opinion on this?

@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 (recipe) and found some lint.

Here's what I've got...

For recipe:

  • Recipe maintainer "eg:wq!" does not exist

@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 (recipe) and found it was in an excellent condition.

@mwittgen
Copy link
Author

mwittgen commented Aug 6, 2022

I implemented generating the cmake file from source and added a small cmake test to build and link.

@egpbos
Copy link
Contributor

egpbos commented Aug 8, 2022

This is perfect, thanks a lot for this excellent contribution!

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

Successfully merging this pull request may close these issues.

3 participants