conda-forge / ctng-compilers-feedstock

A conda-smithy repository for ctng-compilers.
BSD 3-Clause "New" or "Revised" License
12 stars 28 forks source link

Make user given rpaths have higher priority over PREFIX #72

Closed isuruf closed 3 years ago

isuruf commented 3 years ago

Checklist

conda-forge-linter commented 3 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.

isuruf commented 3 years ago

@beckermr, thoughts? Previously, user given rpaths (-Wl,-rpath,/foo) got lower priority than $PREFIX/lib.

beckermr commented 3 years ago

I can't say I've given this topic any thought at all. In fact, if you had asked me before, I would have said it was the opposite! ;p

The only issue I can think of is if somehow a build is linking to a system lib (and working) when it should have linked to a lib in conda (but would fail in that case). I would think we'd have wanted that to fail anyways.

I guess users of our compilers outside of conda might depend on this? I can't say either way.

What was the issue you found that prompted this change?

beckermr commented 3 years ago

Could accidentally vendored copies of key system libraries be more damaging with this change? Seems like yes?

beckermr commented 3 years ago

Would envs with the sysroot packages still link to system libraries correctly with this change?

I had a previous comment I deleted.

isuruf commented 3 years ago

The only change is when a user explicitly adds a rpath using -Wl,-rpath,/foo flag. To get the previous behaviour, they can do -Wl,-rpath,$PREFIX/lib -Wl,-rpath,/foo, but the new behaviour cannot be simulated with the old compilers.

The only issue I can think of is if somehow a build is linking to a system lib (and working) when it should have linked to a lib in conda (but would fail in that case). I would think we'd have wanted that to fail anyways.

This is impossible in previous case. You always load the lib in conda.

What was the issue you found that prompted this change?

In a project, they were building a package with our compilers and wanted to override one library in conda by adding -L/path/outside/conda -Wl,-rpath,/path/outside/conda, but it kept loading the conda library.

Could accidentally vendored copies of key system libraries be more damaging with this change? Seems like yes?

Not really. User would have to add a rpath explicitly, which IMO happens rarely and those who do should know what they are doing.

Would envs with the sysroot packages still link to system libraries correctly with this change?

I don't understand this question.

beckermr commented 3 years ago

Hmmm.

When we build with conda build, we add rpath flags. Won't those be treated differently too?

isuruf commented 3 years ago

When we build with conda build, we add rpath flags. Won't those be treated differently too?

With this change $PREFIX/lib will get higher precedence than $BUILD_PREFIX/lib (hardcoded in the compiler) which is the correct thing.

beckermr commented 3 years ago

I am still confused on one point. It may be me and I don't want to hold up this PR if it is not needed.

Some people install the compilers in their runtime environments and use them to build code. In this case, with this change, which copies of system libraries (eg libm) would be used to link when compiled code is run, the ones on the system or the ones in the sysroot packages?

isuruf commented 3 years ago

The ones on the system. (The ones in the sysroot are in $CONDA_PREFIX/$HOST/sysroot/usr/lib which is not added by rpath ever)

beckermr commented 3 years ago

OK. Thanks for answering my questions!

I clearly don't fully understand all of the implications of this change as well as you do.

If you are comfortable with it, then by all means go ahead.

We can always mark the builds broken later.

isuruf commented 3 years ago

Thanks for the review. I'm happy to chat more if you want.