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 cfitsio/3.470 #1121

Merged
merged 4 commits into from
Apr 14, 2020
Merged

add cfitsio/3.470 #1121

merged 4 commits into from
Apr 14, 2020

Conversation

SpaceIm
Copy link
Contributor

@SpaceIm SpaceIm commented Mar 16, 2020

Specify library name and version: cfitsio/3.470

  • I've read the guidelines for contributing.
  • I've followed the PEP8 style guides for Python code in the recipes.
  • I've used the latest Conan client version.
  • I've tried at least one configuration locally with the
    conan-center hook activated.

@SpaceIm SpaceIm mentioned this pull request Mar 16, 2020
@conan-center-bot
Copy link
Collaborator

All green in build 1 (367546acf7c3efd06481576ae73c29ef2b56f23f)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 2 (c07f7980b843caedcd5f51dc255963de264293e3)! 😊

@conan-center-bot
Copy link
Collaborator

All green in build 3 (f52816410df27fe287f9e828f3c41591db31a86c)! 😊

@madebr
Copy link
Contributor

madebr commented Mar 17, 2020

Why did you create a new cmake build script?
The tarball contains a autotools and cmake build script.

Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Use the CMakeLists.txt provided by the author. Even other pkg managers are using it.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Mar 17, 2020

I'm not confortable enough with autoconf and its helper class in conan to provide a recipe, therefore I prefer to let someone else create a recipe for cfitsio ;)
There will be several additional patches if you only rely on CMakeLists: cfitsio CMakeLists lacks several features compared to autoconf files (shared memory, simd, few definitions to inject, install only public headers, bzip2 support, proper support of remote file driver).

@madebr
Copy link
Contributor

madebr commented Mar 18, 2020

@SpaceIm What I have done in the past is fork the project's repo (or use the release as a base),
fix some problems and send my patches upstream.
It seems they have an e-mail address listed on the homepage.
You can then re-use that patch here in CCI.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Mar 18, 2020

@madebr @uilianries is it acceptable with this patch on CMakeLists?

And yes, I could send an email to library authors.

@SpaceIm SpaceIm requested a review from uilianries March 18, 2020 14:13
@madebr
Copy link
Contributor

madebr commented Mar 18, 2020

@madebr @uilianries is it acceptable with this patch on CMakeLists?

And yes, I could send an email to library authors.

I ❤️ it! 😄

@conan-center-bot
Copy link
Collaborator

All green in build 4 (2dba85a33fc7ebd30ab3647e0a24b57095ecb2f6)! 😊

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Mar 25, 2020

Need to check compiler.threads for MinGW in order to require pthreads4w if win32 threads (also update CMakeLists to link to link to CONAN_PKG::pthreads4w if CMAKE_USE_WIN32_THREADS_INIT True).

uilianries
uilianries previously approved these changes Mar 25, 2020
Copy link
Member

@uilianries uilianries left a comment

Choose a reason for hiding this comment

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

Only few improvements, but looks good. Please, send your patch to author.

recipes/cfitsio/all/conanfile.py Outdated Show resolved Hide resolved
recipes/cfitsio/all/conanfile.py Show resolved Hide resolved
SpaceIm and others added 2 commits March 26, 2020 09:41
@conan-center-bot
Copy link
Collaborator

All green in build 5 (da3c01f9e1832143bed2e78e8e48b3140960d333)! 😊

uilianries
uilianries previously approved these changes Mar 26, 2020
@danimtb danimtb requested a review from SSE4 March 26, 2020 15:29
@conan-center-bot
Copy link
Collaborator

All green in build 6 (d10182fa5a178e6d285fe7957d74aa71476d2e9c)! 😊

Copy link
Contributor

@jgsogo jgsogo left a comment

Choose a reason for hiding this comment

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

Awesome work on the build system. Did you finally send the patch to the library authors? Did they answer anything? I'm sure they can take advantage of the improvements here... and also their investigators would love to run just a conan install ... within their project to have the dependencies they want working in their project.

Great!

@jgsogo jgsogo merged commit 2ed59c2 into conan-io:master Apr 14, 2020
@SpaceIm SpaceIm deleted the cfitsio/3.470 branch April 14, 2020 11:39
@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 14, 2020

Not yet, the patch requires more love to be submitted to library authors. They can't rely on CONAN_PKG in their own CMakeLists and two options should be added in order to conditionnaly build test and executable.

@jgsogo
Copy link
Contributor

jgsogo commented Apr 14, 2020

When I add a recipe to CCI, I always use the cmake_find_package generator, it provides proper target names to avoid using the CONAN_PKG:: one.

I really hope that all these efforts will eventually arrive upstream and we are contributing to making the original projects better too.

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Apr 14, 2020

I don't trust cmake_find_package with current conan state, since components are not implemented (maybe I'm wrong).

@jgsogo
Copy link
Contributor

jgsogo commented Apr 14, 2020

Components are not implemented from the consumer perspective. I would really like to know what you don't trust from the current state of that generator. Components will add only granularity, but the functionality within each component/target will be essentially the same.

Feel free to contact me slack, open a Github issue or whatever works for you.

Thanks!

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.

6 participants