search

conda-forge / nccl-feedstock

A conda-smithy repository for nccl.
BSD 3-Clause "New" or "Revised" License
4 stars 16 forks source link

Remove copying headers from system hack #1

Closed isuruf closed 5 years ago

isuruf commented 5 years ago

See https://github.com/conda-forge/staged-recipes/pull/9694/files#r337609849

jakirkham commented 5 years ago

Can you please help me understand what your concern is?

The system headers included in the conda-forge compilers don't match the one's in the system. Given this, it seems reasonable to use the system header preferentially.

isuruf commented 5 years ago

The hard coded path (/usr/include) is the issue here. Some systems might not have the header in the same place. Or if you are cross-compiling they won't be there at all.

jakirkham commented 5 years ago

Do you have an example?

isuruf commented 5 years ago

What do you mean by an example?

jakirkham commented 5 years ago

An example of a system that has this header in another location.

isuruf commented 5 years ago

For example the busybox docker image.

isuruf commented 5 years ago

Ubuntu docker image doesn't have it either. Neither does the centos:6 image.

jakirkham commented 5 years ago

For example the busybox docker image.

This I'm not too worried about actually. BusyBox also doesn't have things like bash, GLIBC, or libraries required to load shared objects. So we have issues before getting to missing kernel headers.

Ubuntu docker image doesn't have it either. Neither does the centos:6 image.

It appears the NVIDIA images ship with the kernel headers installed. Though I agree it is worth making this dependency explicit. Have added PR ( https://github.com/conda-forge/nccl-feedstock/pull/2 ), which does that.

jakirkham commented 5 years ago

It appears that upstream added commit ( https://github.com/NVIDIA/nccl/commit/7f2b337e703d73ed369937c9996e1f3d5f664ad0 ) recently, which makes the use of SO_REUSEPORT optional. This should fix the underlying issue making the need for the system header here unnecessary.

jakirkham commented 5 years ago

Have added PR ( https://github.com/conda-forge/nccl-feedstock/pull/5 ) to apply commit ( https://github.com/NVIDIA/nccl/commit/7f2b337e703d73ed369937c9996e1f3d5f664ad0 ) as a patch in our build.

isuruf commented 5 years ago

Thanks