Change includes to be system instead of user

conda-forge / nvcc-feedstock

A conda-smithy repository for nvcc.

BSD 3-Clause "New" or "Revised" License

12 stars 23 forks source link

Change includes to be system instead of user #80

Closed kkraus14 closed 2 years ago

kkraus14 commented 2 years ago

Checklist

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

Fixes #79

conda-forge-linter commented 2 years ago

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.

kkraus14 commented 2 years ago

@conda-forge-admin, please rerender

kkraus14 commented 2 years ago

2022-04-25T15:23:58.2451571Z /home/conda/feedstock_root/build_artifacts/nvcc_1650900156699/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_p/bin/x86_64-conda-linux-gnu-c++: /lib64/libc.so.6: version `GLIBC_2.14' not found (required by /home/conda/feedstock_root/build_artifacts/nvcc_1650900156699/_test_env_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_placehold_p/bin/x86_64-conda-linux-gnu-c++)

I'm guessing this is something busted with our custom conda_build_config.yaml but I'm unclear unless we're using outdated docker images?

kkraus14 commented 2 years ago

It looks like the latest gcc / gxx 7.5 packages were built on cos7, where cos6 is no longer really supported. In order to get passing builds on 9.2 - 10.2 needed to move to the cos7 images.

kkraus14 commented 2 years ago

@conda-forge-admin, please rerender

jakirkham commented 2 years ago

@isuruf, do you what is going on here ( https://github.com/conda-forge/nvcc-feedstock/pull/80#issuecomment-1108730100 )? No worries if not

kkraus14 commented 2 years ago

@jakirkham @leofang any issues with moving forward with the cos7 changes? Given this is just a shell script anyway? I have the sense that more and more cos6 issues are going to trickle down where I'm not sure it's worth the pain of us dealing with them for this package.

leofang commented 2 years ago

I don't see any concern here, though I am not sure what issue using -I would cause either. But I'm fine merging it.

kkraus14 commented 2 years ago

Thanks @leofang!

jakirkham commented 2 years ago

Yeah sorry ok with me too. Just wanted to flag that issue since it is a bit odd

kkraus14 commented 2 years ago

I don't see any concern here, though I am not sure what issue using -I would cause either. But I'm fine merging it.

It changes how tools like clang-tidy interact for one thing, and then libraries like thrust and cub are included in the folder where a user might want to specify a different version than what's included in the toolkit.

robertmaynard commented 2 years ago

The primary issue with this approach is that by using CXXFLAGS etc with -I we are setting the first includes therefore overriding any the user has specified. We could use -idirafter to set an include that will be searched after the -I and -isystem.

I don't know if clang-tidy understands the idirafter option

Edit: Using idirafter also makes sure we don't override any -isystem includes that the project has. The primary occurence I can think of is when CMake adds includes from a imported target ( e.g. find_package call) those are all system includes.