Add host prefix to NVCC_PREPEND_FLAGS when using conda-build #19

carterbox · 2023-06-02T16:32:08Z

Adds the host requirements prefix include-paths and linking-search-paths to the NVCC_PREPEND_FLAGS for conda-build (because those two prefixes are separate) by using an activation script only when conda-build is active.

Outside of the conda-build context, these paths are not needed because nvcc would share a prefix with all the cuda libs.

I didn't do anything for Windows because it's skipped.

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.

Closes #18
Closes conda-forge/libcublas-feedstock#11
~~Requires conda-forge/cuda-nvcc-impl-feedstock#2~~

conda-forge-webservices · 2023-06-02T16:32:13Z

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.

carterbox · 2023-06-02T16:34:22Z

@conda-forge-admin, please rerender

…nda-forge-pinning 2023.06.02.16.26.35

jakirkham · 2023-06-02T17:37:25Z

recipe/activate.sh

@@ -27,6 +27,7 @@ export CFLAGS="${CFLAGS} ${CUDA_CFLAGS} ${CUDA_LDFLAGS}"
 export CPPFLAGS="${CPPFLAGS} ${CUDA_CFLAGS} ${CUDA_LDFLAGS}"
 export CXXFLAGS="${CXXFLAGS} ${CUDA_CFLAGS} ${CUDA_LDFLAGS}"
 export LDFLAGS="${LDFLAGS} ${CUDA_LDFLAGS}"
+export CUDAFLAGS="${CUDAFLAGS} ${CUDA_CFLAGS}"


Do we want the LDFLAGS too or just CFLAGS?

Suggested change

export CUDAFLAGS="${CUDAFLAGS} ${CUDA_CFLAGS}"

export CUDAFLAGS="${CUDAFLAGS} ${CUDA_CFLAGS} ${CUDA_LDFLAGS}"

I don't think the library paths are needed because NVCC isn't being used as a linker? For example, the libmagma feedstock (which has separable compilation off for linux) uses GCC to link all of the generated object files as the last step, not NVCC. The library paths are only needed at link-time, the headers are only needed at compile time?

I'm not sure if people ever use the NVCC CLI for linking or whether NVCC accepts / passes these args down to the host compiler.

CUDA_LDFLAGS should only be in LDFLAGS. I don't know why CUDA_LDFLAGS was added to CXXFLAGS and friends. That's clearly wrong.

Maybe this should go in a separate issue or PR? Doesn't seem related to Daniel's initial request

Restored this part of the PR to match upstream. Some else can fix the LDFLAGS issue if they feel strongly about it.

jakirkham · 2023-06-02T17:37:51Z

Thanks Daniel! 🙏

Had a question above 🙂

jakirkham

Think it would be reasonable to handle the LDFLAGS changes in a separate PR as that was not part of the original changes here

That said, if the OP wants to include them here, the CUDA_* variables likely need other updates (to include library search paths)

recipe/meta.yaml

jakirkham · 2023-06-04T21:25:54Z

recipe/activate.sh

@@ -27,6 +27,7 @@ export CFLAGS="${CFLAGS} ${CUDA_CFLAGS} ${CUDA_LDFLAGS}"
 export CPPFLAGS="${CPPFLAGS} ${CUDA_CFLAGS} ${CUDA_LDFLAGS}"
 export CXXFLAGS="${CXXFLAGS} ${CUDA_CFLAGS} ${CUDA_LDFLAGS}"
 export LDFLAGS="${LDFLAGS} ${CUDA_LDFLAGS}"
+export CUDAFLAGS="${CUDAFLAGS} ${CUDA_CFLAGS}"


Maybe this should go in a separate issue or PR? Doesn't seem related to Daniel's initial request

recipe/activate.sh

The complementary part of this PR is found in conda-forge/cuda-nvcc-impl-feedstock#2

…nda-forge-pinning 2023.06.06.09.57.57

The complementary part exists in conda-forge/cuda-nvcc-feedstock#19

isuruf · 2023-06-06T17:42:22Z

I don't think we should introduce new env variables into the mix. Better to use NVCC_PREPEND_FLAGS.

…nda-forge-pinning 2023.06.06.09.57.57

carterbox · 2023-06-06T19:10:17Z

I tested this on a local build of the magma-feedstock. It seems to work, but silently. i.e. The args don't appear in the build logs.

jakirkham

Thanks Daniel! 🙏

This seems reasonable. Had a couple follow up questions

recipe/activate.sh

jakirkham · 2023-06-06T19:46:00Z

The args don't appear in the build logs.

Maybe we should considering adding set -x at the top and set +x at the end (since it is sourced)?

bdice

I tested these changes locally and can confirm it fixes the issues I saw in the conda build process for nvcc in the build environment to search the host environment.

carterbox · 2023-06-07T02:18:43Z

This is the one, people. Let's get this patch out before the CUDA 12 migration is too far along. 😆

jakirkham

Thanks Daniel! 🙏

If it's ok, would set -ccbin first (included a suggestion)

Otherwise agree. Let's merge after that 🙂

recipe/activate.sh

jakirkham · 2023-06-07T04:32:57Z

Thanks Daniel for the PR and everyone for reviewing! 🙏

Let's get this change out. We can follow up on anything else separately

robertmaynard · 2023-06-07T14:48:28Z

I don't think we should introduce new env variables into the mix. Better to use NVCC_PREPEND_FLAGS.

I wanted to use a different env variable and inject into nvcc.profile so that CMake would be able to use these include and link directories as they would be in the INCLUDES and LIBRARIES output of nvcc ( https://gitlab.kitware.com/cmake/cmake/-/issues/24915 ). By using NVCC_PREPEND_FLAGS those directories will not be considered by CMake as that is a user flag and not part of the compiler implicit information.

Add CUDAFLAGS to activate script

a3bd9c6

carterbox requested a review from adibbley as a code owner June 2, 2023 16:32

Bump the build number

16312c7

MNT: Re-rendered with conda-build 3.25.0, conda-smithy 3.23.1, and co…

24c9ea7

…nda-forge-pinning 2023.06.02.16.26.35

jakirkham reviewed Jun 2, 2023

View reviewed changes

carterbox added 2 commits June 2, 2023 23:39

Only add CUDA_LDFLAGS to LDFLAGS not CFLAGS,CXXFLAGS

07413ec

Add linker flags to explicit compiler test

74cacea

jakirkham reviewed Jun 4, 2023

View reviewed changes

robertmaynard requested changes Jun 5, 2023

View reviewed changes

recipe/activate.sh Outdated Show resolved Hide resolved

carterbox mentioned this pull request Jun 5, 2023

Add missing include and library paths conda-forge/cuda-nvcc-impl-feedstock#2

Closed

5 tasks

carterbox added 2 commits June 6, 2023 11:53

REF: No activation scripts allowed in cuda-nvcc-impl-feedstock

3cbf810

The complementary part of this PR is found in conda-forge/cuda-nvcc-impl-feedstock#2

MNT: Re-rendered with conda-build 3.24.0, conda-smithy 3.23.1, and co…

d5eccf1

…nda-forge-pinning 2023.06.06.09.57.57

carterbox added a commit to carterbox/cuda-nvcc-impl-feedstock that referenced this pull request Jun 6, 2023

REF: No activation scripts allowed in this feedstock

3a04c02

The complementary part exists in conda-forge/cuda-nvcc-feedstock#19

carterbox requested review from isuruf, jakirkham and robertmaynard June 6, 2023 17:23

REF: Use exisiting environment variable NVCC_PREPEND_FLAGS

6b4dd71

carterbox changed the title ~~Add CUDAFLAGS to activate script~~ Add host prefix to NVCC_PREPEND_ARGS when using conda-build Jun 6, 2023

carterbox changed the title ~~Add host prefix to NVCC_PREPEND_ARGS when using conda-build~~ Add host prefix to NVCC_PREPEND_FLAGS when using conda-build Jun 6, 2023

MNT: Re-rendered with conda-build 3.24.0, conda-smithy 3.23.1, and co…

168e8f9

…nda-forge-pinning 2023.06.06.09.57.57

jakirkham reviewed Jun 6, 2023

View reviewed changes

recipe/activate.sh Outdated Show resolved Hide resolved

recipe/activate.sh Outdated Show resolved Hide resolved

bdice reviewed Jun 6, 2023

View reviewed changes

REF: Clump all of NVCC_PREPEND_FLAGS augmentation together

e310698

jakirkham reviewed Jun 7, 2023

View reviewed changes

recipe/activate.sh Outdated Show resolved Hide resolved

recipe/activate.sh Outdated Show resolved Hide resolved

REF: Swap order of prepend flags

5476812

jakirkham approved these changes Jun 7, 2023

View reviewed changes

jakirkham merged commit f602ed9 into conda-forge:main Jun 7, 2023

jakirkham mentioned this pull request Jun 7, 2023

Rebuild for CUDA 12 conda-forge/colmap-feedstock#19

Merged

jakirkham mentioned this pull request Jun 8, 2023

Rebuild for CUDA 12 conda-forge/nyxus-feedstock#18

Merged

carterbox deleted the cuda-flags branch October 28, 2024 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add host prefix to NVCC_PREPEND_FLAGS when using conda-build #19

Add host prefix to NVCC_PREPEND_FLAGS when using conda-build #19

carterbox commented Jun 2, 2023 •

edited

Loading

conda-forge-webservices bot commented Jun 2, 2023

carterbox commented Jun 2, 2023

jakirkham Jun 2, 2023

carterbox Jun 2, 2023

carterbox Jun 2, 2023 •

edited

Loading

isuruf Jun 3, 2023

jakirkham Jun 4, 2023

carterbox Jun 6, 2023

jakirkham commented Jun 2, 2023

jakirkham left a comment •

edited

Loading

jakirkham Jun 4, 2023

isuruf commented Jun 6, 2023

carterbox commented Jun 6, 2023

jakirkham left a comment

jakirkham commented Jun 6, 2023

bdice left a comment

carterbox commented Jun 7, 2023

jakirkham left a comment

jakirkham commented Jun 7, 2023

robertmaynard commented Jun 7, 2023

	export CUDAFLAGS="${CUDAFLAGS} ${CUDA_CFLAGS}"
	export CUDAFLAGS="${CUDAFLAGS} ${CUDA_CFLAGS} ${CUDA_LDFLAGS}"

Add host prefix to NVCC_PREPEND_FLAGS when using conda-build #19

Add host prefix to NVCC_PREPEND_FLAGS when using conda-build #19

Conversation

carterbox commented Jun 2, 2023 • edited Loading

conda-forge-webservices bot commented Jun 2, 2023

carterbox commented Jun 2, 2023

jakirkham Jun 2, 2023

Choose a reason for hiding this comment

carterbox Jun 2, 2023

Choose a reason for hiding this comment

carterbox Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

isuruf Jun 3, 2023

Choose a reason for hiding this comment

jakirkham Jun 4, 2023

Choose a reason for hiding this comment

carterbox Jun 6, 2023

Choose a reason for hiding this comment

jakirkham commented Jun 2, 2023

jakirkham left a comment • edited Loading

Choose a reason for hiding this comment

jakirkham Jun 4, 2023

Choose a reason for hiding this comment

isuruf commented Jun 6, 2023

carterbox commented Jun 6, 2023

jakirkham left a comment

Choose a reason for hiding this comment

jakirkham commented Jun 6, 2023

bdice left a comment

Choose a reason for hiding this comment

carterbox commented Jun 7, 2023

jakirkham left a comment

Choose a reason for hiding this comment

jakirkham commented Jun 7, 2023

robertmaynard commented Jun 7, 2023

carterbox commented Jun 2, 2023 •

edited

Loading

carterbox Jun 2, 2023 •

edited

Loading

jakirkham left a comment •

edited

Loading