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

Update to 2.2.0 (and switch to monorepo) #6

Merged
merged 15 commits into from
Feb 16, 2024

Conversation

conda-forge-admin
Copy link
Contributor

@conda-forge-admin conda-forge-admin commented Feb 13, 2024

Fixes #2

  • Update to CCCL 2.2.0
  • Switch to using CCCL monorepo (instead of combining the sources ourselves)
  • Use CMake & Ninja to install (instead of manually copying files)
  • Disable tests as those are irrelevant here (CCCL is header-only and tests add CPM dependencies)

@conda-forge-webservices
Copy link
Contributor

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.

Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to update the version for you, but it looks like there was nothing to do.

I am closing this PR!

This message was generated by GitHub actions workflow run https://github.com/conda-forge/cccl-feedstock/actions/runs/7893076871.

@github-actions github-actions bot closed this Feb 13, 2024
@jakirkham jakirkham reopened this Feb 13, 2024
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

conda-forge-webservices[bot] and others added 2 commits February 13, 2024 21:43
Using different extensions unnecessarily complicates source retrieval.
So just pick an extension and use it in all cases.
@jakirkham jakirkham marked this pull request as draft February 13, 2024 22:03
@jakirkham jakirkham changed the title ENH: update package version [WIP] ENH: update package version Feb 13, 2024
@jakirkham
Copy link
Member

jakirkham commented Feb 13, 2024

Noticing we haven't done the arch migration here. Going to pause and start that first: conda-forge/conda-forge-pinning-feedstock#5501

Will come back to this after that is done

Edit: Was done in PR: #7

@jakirkham jakirkham changed the title [WIP] ENH: update package version [WIP] Update to 2.2.0 (and switch to monorepo) Feb 13, 2024
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

Copy link
Contributor

Hi! This is the friendly automated conda-forge-webservice.

I tried to rerender for you, but it looks like there was nothing to do.

This message was generated by GitHub actions workflow run https://github.com/conda-forge/cccl-feedstock/actions/runs/7894807589.

recipe/build.sh Outdated
@@ -3,6 +3,10 @@
mkdir -p ${PREFIX}/lib/cmake
mkdir -p ${PREFIX}/include

pushd cccl-${PKG_VERSION}
Copy link
Member

Choose a reason for hiding this comment

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

Weird, for some reason when running python build-locally.py, the source is put in this directory. However on CI (or if the Docker image is spun up manually and the build is done in there), the source directory is not named this way (and is not nested)

@jakirkham jakirkham force-pushed the conda_forge_admin_2 branch 2 times, most recently from 910552f to 0c9580a Compare February 14, 2024 02:37
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

@jakirkham jakirkham changed the title [WIP] Update to 2.2.0 (and switch to monorepo) Update to 2.2.0 (and switch to monorepo) Feb 14, 2024
@jakirkham jakirkham marked this pull request as ready for review February 14, 2024 02:57
@jakirkham
Copy link
Member

@jrhemstad , could you please look over this and let me know if there is anything that we need to change here?

Want to make sure we are picking up all the files we need to. Looks just like a couple of CMake files, but please let us know if we need other components as well

Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Highlighted the relevant CCCL changes below

Otherwise this just copies over the same Thrust, CUB & libcudacxx files as before

Please let us know if there is anything else we should have here

recipe/build.sh Outdated Show resolved Hide resolved
recipe/bld.bat Outdated Show resolved Hide resolved
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

@jakirkham jakirkham force-pushed the conda_forge_admin_2 branch 2 times, most recently from 40e1533 to b9b2dab Compare February 14, 2024 23:33
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

Should fix space limitations affecting cross-compilation.
@jakirkham
Copy link
Member

@conda-forge-admin , please re-render

conda-forge-webservices[bot] and others added 2 commits February 14, 2024 23:52
recipe/build.sh Outdated Show resolved Hide resolved
Copy link
Member

@jakirkham jakirkham left a comment

Choose a reason for hiding this comment

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

Following up on Rob's suggestion to use a relative path for CMAKE_INSTALL_LIBDIR

recipe/build.sh Outdated Show resolved Hide resolved
recipe/bld.bat Outdated Show resolved Hide resolved
Instead of using an absolute path with `CMAKE_INSTALL_LIBDIR`, use a relative path based on `CMAKE_INSTALL_PREFIX`.

ref: https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
@jakirkham
Copy link
Member

Rob also suggested offline that we add a test to verify find_package(CCCL) works correctly. Will look into that next

Copy link

@jrhemstad jrhemstad left a comment

Choose a reason for hiding this comment

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

Rob's approval means a lot more than mine, but I'll still approve fwiw 🙂

@jakirkham jakirkham merged commit f542448 into conda-forge:main Feb 16, 2024
6 checks passed
@jakirkham
Copy link
Member

Thanks you both! 🙏

If anything else comes up, would be happy to follow up 🙂

@leofang
Copy link
Member

leofang commented Feb 16, 2024

Thanks everyone!

set(CMAKE_CXX_STANDARD_REQUIRED TRUE)

find_package(CUDAToolkit REQUIRED)
find_package(CCCL REQUIRED)

Choose a reason for hiding this comment

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

We want to validate that the CCCL includes are coming from the package we just made and not the version packaged in the CUDAToolkit.
I believe that the following check should be added at the end:

get_target_property(cccl_raw_includes CCCL::libcudacxx INTERFACE_INCLUDE_DIRECTORIES)
foreach(item IN LISTS cccl_raw_includes)
  foreach(toolkit_path IN LISTS CUDAToolkit_INCLUDE_DIRS)
    cmake_path(IS_PREFIX toolkit_path "${item}" NORMALIZE is_prefix)
    if(is_prefix)
      message(FATAL_ERROR "Failed to find conda version of the CCCL. Instead found version within the CUDAToolkit")
    endif()
  endforeach()
endforeach()

Copy link
Member

Choose a reason for hiding this comment

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

Right now this is run on CUDA 11.8 where the NVIDIA CUDA-based Docker images are used. Here are the results we see with that

-- The CUDA compiler identification is NVIDIA 11.8.89
-- Detecting CUDA compiler ABI info
-- Detecting CUDA compiler ABI info - done
-- Check for working CUDA compiler: $PREFIX/bin/nvcc - skipped
-- Detecting CUDA compile features
-- Detecting CUDA compile features - done
-- Found CUDAToolkit: /usr/local/cuda/targets/x86_64-linux/include (found version "11.8.89") 
-- Found libcudacxx: $PREFIX/lib/cmake/libcudacxx/libcudacxx-config.cmake (found suitable version "2.2.0.0", minimum required is "2.2.0.0") 
-- Found Thrust: $PREFIX/lib/cmake/thrust/thrust-config.cmake (found suitable exact version "2.2.0.0") 
-- Found CUB: $PREFIX/lib/cmake/cub/cub-config.cmake (found suitable version "2.2.0.0", minimum required is "2.2.0.0") 
-- Found CCCL: $PREFIX/lib/cmake/cccl/cccl-config.cmake (found version "2.2.0.0") 
-- CCCL info:
--     config: $PREFIX/lib/cmake/cccl/cccl-config.cmake
--     version: 2.2.0.0
-- Configuring done (3.3s)
-- Generating done (0.0s)
-- Build files have been written to: $SRC_DIR/test_cmake

So that seems to point $PREFIX as expected

On CUDA 12, we ensured that $PREFIX (so this cccl) come before the targets directory in the search order

If we are concerned about cuda-cccl from targets being picked up, should we just override CMAKE_FIND_ROOT_PATH in the test?

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.

Transition to use the CCCL monorepo
5 participants