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

Updated cuda-api-wrappers with versions 0.7.1 and 0.8.0 #26256

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

eyalroz
Copy link
Contributor

@eyalroz eyalroz commented Dec 24, 2024

Summary

Changes to recipe: cuda-api-wrappers

Motivation

  • previous PR for one the last version was ignored for months
  • New version of the library have been released
  • Mistype in the CMakeLists.txt for the test package

Details

  • Updated the recipe with versions 0.7.1 and 0.8.0
  • fixed a mistype in the test package CMakeLists.txt.

@eyalroz eyalroz force-pushed the add-caw-0.7.1-and-0.8.0 branch from d5053a9 to 8f8353e Compare December 24, 2024 00:36
@eyalroz
Copy link
Contributor Author

eyalroz commented Dec 24, 2024

@valgur , @franramirez688 : Four months have passed, and I've released a new version. Closed the last PR, opened this one. Please don't let me wait four more months...

edit: Now passing the checks.

@eyalroz eyalroz force-pushed the add-caw-0.7.1-and-0.8.0 branch 6 times, most recently from b266bc0 to b9c01ff Compare December 26, 2024 20:48
@@ -1,10 +1,10 @@
cmake_minimum_required(VERSION 3.15)
cmake_minimum_required(VERSION 3.25)
Copy link
Contributor

Choose a reason for hiding this comment

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

thanks @eyalroz for your PR.

it's a bit unusual for a test package (and a test package alone) to require a higher version of CMake - because the find_package(cuda-api-wrappers REQUIRED CONFIG) call is resolved with files that COnan generates targeting compatible with at least CMake 3.15

if you could provide more details as to why this is needed (e.g. what error this is fixing, with logs), that would be great, thanks!

Copy link
Contributor Author

@eyalroz eyalroz Dec 30, 2024

Choose a reason for hiding this comment

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

Well, let me first note that the (non-test) package does requires v3.25; it's just that the package doesn't get build - it's a header-only library.

The CMake version bump is due to the need is for decent and relatively comprehensive CUDA support, and as you may be aware, that's still very much a work in progress at Kitware. Some changes in locating / identifying the "NVIDIA Toolkit Extensions" library were made in v3.25, which make it actually work on Windows.

See also:

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, thanks for providing additional insights!

The recipe itself is not currently using CMake, and only "copies" header files to the package folder:

copy(
self,
pattern="*",
dst=os.path.join(self.package_folder, "include"),
src=os.path.join(self.source_folder, "src"),
)

So while this project https://github.com/eyalroz/cuda-api-wrappers/blob/master/CMakeLists.txt would require a recent version of CMake, it's not really part of the package process.

As for the test package in this repository: the purpose is to have a very simple check of verifying that find_package(cuda-api-wrappers) works, for a header-only library, that the compiler can locate a header from the package (assuming

) and reference anything defined in that header (assuming
std::cout << "cuda-api-wrappers v" << CUDA_API_WRAPPERS_VERSION << std::endl;
) - so the version of CMake does not need to be altered in this instance either, unless I'm missing something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So while this project https://github.com/eyalroz/cuda-api-wrappers/blob/master/CMakeLists.txt would require a recent version of CMake, it's not really part of the package process.

I'm the opposite of an expert on conan matters, but still... CMake 3.25 is necessary to properly locate and register the relevant dependencies in the exported .cmake files; does that not make it part of the packaging process?

As for the test package in this repository: the purpose is to have a very simple check of verifying that find_package(cuda-api-wrappers) works, for a header-only library, that the compiler can locate a header from the package

But that's not how the library works. find_package(cuda-api-wrappers) locates configuration files; and these files require CMake 3.25 . Are you suggesting I write a test package which does a lower-level file search? I suppose I could do that, but it wouldn't really test much of anything.

Copy link
Contributor

@jcar87 jcar87 Dec 31, 2024

Choose a reason for hiding this comment

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

What I mean is that the the test package does not require CMake 3.25, and it should work with whatever version was there before (just for the purposes of the test package).

in the exported .cmake files; does that not make it part of the packaging process?

Conan Center does not make use of packaged .cmake files - instead, the relevant information is included in package_info() - and then Conan generates the files for the consumer. Those files, as generated by Conan, target CMake 3.15 (and usually, when they are generated by CMake,t hey are also compatible with other versions).

As I mentioned before, the recipe for cuda-api-wrappers, not only is it not generating or packaging the exported CMake files - it is not even invoking CMake at all
https://github.com/conan-io/conan-center-index/blob/b9c01ff294197ca0120d734546d838811324e36a/recipes/cuda-api-wrappers/all/conanfile.py

But that's not how the library works. find_package(cuda-api-wrappers) locates configuration files; and these files require CMake 3.25 .

The configuration files that the test package are locating, are generated by Conan and they are compatible with CMake 3.15 -

Are you suggesting I write a test package which does a lower-level file search?

Just suggesting that for the sake of the test_package inside Conan Center in this PR - these cmake changes are not necessary - I understand that they are necessary for actual users of the library, but that is not the in the scope of the test package.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the test package does not require CMake 3.25, and it should work with whatever version was there before

Ok, removed that requirement and it does indeed work. :-)

Sorry for being a little slow to understand.

@jcar87 jcar87 self-assigned this Dec 30, 2024
@eyalroz eyalroz force-pushed the add-caw-0.7.1-and-0.8.0 branch from b9c01ff to adb2d94 Compare December 31, 2024 19:38
* Removed 0.7 beta versions
* Added versions 0.7.0, 0.7.1 and 0.8.0
* Now specifying the correct exported target name in the main `conanfile.py`
* Now using the correct the target name in the test package's `CMakeLists.txt`
@eyalroz eyalroz force-pushed the add-caw-0.7.1-and-0.8.0 branch from adb2d94 to 11f6099 Compare December 31, 2024 20:55
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.

2 participants