search

conda-forge / r-rcppparallel-feedstock

A conda-smithy repository for r-rcppparallel.
BSD 3-Clause "New" or "Revised" License
0 stars 3 forks source link

ENH: Ensure compile-time probe availability on linux #6

Closed thermokarst closed 5 years ago

thermokarst commented 5 years ago

Checklist

While working on a sneaky segfault bug with DADA2, we learned that this package vendors the TBB package. The TBB Makefiles perform a handful of compile-time probes to learn things about the compiler, in particular, one probe handles the zero-initiation problem that we tracked as the root of our segfault woes. However, in conda-build -land, these probes will never be successful, because the probe is hardcoded to search for an executable called gcc in the PATH. Okay, so this changeset attempts to remedy this by supplementing the PATH, but, we are totally open to other (more idiomatic?) ways of solving the issue, just let us know.

Thanks so much!

cc @epruesse cc @benjjneb cc @ebolyen

conda-forge-linter commented 5 years ago

Hi! This is the friendly automated conda-forge-linting service.

I wanted to let you know that I linted all conda-recipes in your PR (recipe) and found some lint.

Here's what I've got...

For recipe:

ebolyen commented 5 years ago

Here's a little more context on the situation after exploring how tbb-feedstock works.

The version used there uses a more recent version of TBB which probes the compiler using shell $(CONLY). This of course works fine with conda build compiler toolchains since they know to set variables such that Make does the right thing.

However because RcppParallel statically links and vendors TBB, it's not using the latest version. As seen in @thermokarst's link above, shell gcc was used in TBB version 2017.7.

We could write a patch to use the more recent "make include" files which would probe correctly, however we're not maintainers of RcppParallel and we don't know the ramifications of that. In a perfect world RcppParallel would be using the latest TBB, but that would of course require actual code changes.

In the meanwhile, updating $PATH was the most obvious way to ensure the old Makefile probing worked as expected. Looking around it doesn't seem that there is a way for conda build to rename the compiler toolchain used on the fly, as it generally expects the Makefiles to do the right thing (which old TBB does not).

Ultimately this all boils down to the original issue with TBB and GCC6+ and the -flifetime-dse flag to disable the compiler elision. This is the most obvious failure as it causes a segfault in RcppParallel, but there may be other issues, and given the size and complexity of the build system inside TBB, we're not super eager to start patching that without a lot more context.

dbast commented 5 years ago

Nice trick! Looks good. Intending to merge this. Thanks

dbast commented 5 years ago

The feedstock is now also updated to version 4.4.2, which also includes the fix. Maybe it makes sense to open an issue over there https://github.com/RcppCore/RcppParallel/issues and paste all the collected information here and the reverenced dada2 discussions to make RcppParallel upstream aware about it.