@adamjstewart I am curious how you managed to build py-pyyaml+libyaml after #16301. In my case, since the linking is done without the wrapper, the linker cannot find -lyaml.
Maybe I missed something that would be relevant for this PR.

Magic?

$ otool -L ./lib/python3.7/site-packages/_yaml.cpython-37m-darwin.so
./lib/python3.7/site-packages/_yaml.cpython-37m-darwin.so:
	/Users/Adam/spack/opt/spack/darwin-catalina-x86_64/clang-11.0.3-apple/libyaml-0.2.4-hreynrt3r54ee6ieqqalj2buulmhdf5l/lib/libyaml-0.2.dylib (compatibility version 3.0.0, current version 3.8.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1281.100.1)

Btw, py-pyyaml has always had a dependency on libyaml, #16301 just made it optional.

@adamjstewart magic indeed. I will check, which versions of Python have this and update this PR accordingly.

For Python 2, the aforementioned magic was introduced in v2.7.3, inadvertently dropped in v2.7.4 and reintroduced in v2.7.6.
For Python 3, it was introduced in v3.2.3, dropped in v3.2.4 and v3.3.0, and reintroduced in v3.3.3.
See also https://bugs.python.org/issue18080.

I don't think that we need any special treatment for darwin here but I don't know whether we should unset LDSHARED when we don't set it.

Ping @scheibelp, any idea who wrote the existing logic in python and would be a good person to review this PR?

@adamjstewart I think the existing logic was introduced in #2506.

@skosukhin want to add yourself to the list of maintainers for this package? I could really use the help 😆

Btw, I noticed that some Python packages use the compiler registered in distutils at build-time. Worse yet, packages like py-torchvision use both the distutils compiler and the CXX compiler at build-time (pytorch/vision#2591). Does this PR address that, or is there a way to address that? I see we filter Spack's compiler wrappers after installation. Maybe we could re-add those temporarily during Spack builds?

@adamjstewart it's been a while I go but I will try to recall the details.

1. The most simple case is when we work with Python @2.7.5:2.7.999,3.6.0:. In this case, we back up _sysconfigdata.py (or whatever get_sysconfigdata_name() gives us) to get_spack_sysconfig_dirname() (somewhere in prefix/.spack/) before filtering Spack's compiler wrappers, and then tell the dependent package to use it instead of the default one (i.e. the one with filtered wrappers) by prepending the path to the backed-up file to PYTHONPATH.
2. _sysconfigdata.py hasn't always been there. As far as I recall, originally the data was collected at runtime from the Makefile. The generation of the file at build time was introduced in @2.7.5 and @3.3.0. Additionally, there was a period when sysconfig used the file but distutils.sysconfig didn't. To cover this case, together with the case of an externally installed Python, we rely on the fact that CC is set by Spack and additionally set LDSHARED (when we can be more or less sure that it won't hurt).

So, basically the answer to your question is yes, in one way or another we temporarily re-add Spack's compiler wrappers during Spack builds.

@adamjstewart I haven't followed the recent developments. Can you, please, tell whether this PR is still relevant? I saw that you tried to solve the RPATH problem in a different way.

The logic I introduced in #20765 doesn't seem to be working, at least for some packages. If merging this PR allows us to use Spack's compiler wrappers for all Python packages, that would be fantastic. I must admit I don't fully understand the logic that you introduced, or the logic it is designed to replace. Let me see if I can convince anyone else to review this PR. If not, I'll try building my Python stack with this PR (can you rebase on develop?) to make sure it doesn't break anything.

Let me see if I can convince anyone else to review this PR.

I assigned myself but lost track of it. It is back on my radar.

I must admit I don't fully understand the logic that you introduced, or the logic it is designed to replace.

I'll take a shot at this:

• The Spack Python package, as it exists, has some logic to attempt to use the Spack compiler wrappers when building dependents (e.g. py-* packages)
  • The logic as is takes LDSHARED collected from the Python installation (this is generally the Spack compiler wrapper) and sets it as an environment variable when running Python.setup_py
• You discovered in PythonPackage: add RPATHs, parallel builds #20765 (comment) that this wasn't working, although I think this PR description contains the additional insight that it was working in some cases but not others
  • This PR description points out that most py-* packages use PythonPackage.setup_py
• This PR resolves the discrepancy by setting LDSHARED universally (not just for Python.setup_py).
• Your PR resolves this by attaching RPATHs and includes directly

Generally I prefer this PR because I think it resolves some issues with redundancy. I'm not immediately sure that this would succeed where #20765 has not e.g. #20765 (comment) (since in many ways I think these approaches would have a similar effect, at least for Spack-built Python). FWIW I do think this PR would avoid some possible issues when using an external Python (a combination of Spack RPATHs with a different compiler could lead to compilation problems).

I'd like to try building a few packages that people were trying in #20765, but with this PR, I'll have a chance to do that on Tuesday (Jan. 19).

@adamjstewart @scheibelp

I wonder what should be the condition that triggers the additional logic in setup_dependent_build_environment. I have several options in mind:

1. if self.spec in dependent_spec.dependencies(deptype='link'). I thought this was the right choice. However, the updated version of py-pyyaml doesn't have python as a link dependency but still requires the compiler substitution. This means that this condition alone is wrong. At least, it should be extended with something else.
2. if self.spec in dependent_spec.dependencies(deptype='build'). This would work in the case above but there are packages that have python as a build dependency only to run some custom python scripts that are part of their building procedures. The additional logic is redundant in these cases. Moreover, it might trigger warning messages Failed to set LDSHARED environment variable..., which might be annoying and misleading in this context.
3. if dependent_spec.package.extends(self.spec). This might be the right option but I don't know whether there are packages that do not extend python but still use distutils to link whatever and therefore need the compiler substitution.
4. Run the logic unconditionally. The concerns regarding this option are the same as for number 2 in this list.

What do you think?

That is a great question. I think either 2 or maybe 3 makes the most sense. There is a 5th option where you only do it if type=build/run, but that's not drastically different than 3 and 4.

If a package has a build dep on Python and it doesn't use distutils, that isn't really a problem even if you patch it. I guess I don't fully understand your concern:

Moreover, it might trigger warning messages Failed to set LDSHARED environment variable..., which might be annoying and misleading in this context.

Why would it do that? Can we make it not do that?

I wonder what should be the condition that triggers the additional logic in setup_dependent_build_environment

I may be confused because I think we already call setup_dependent_build_environment in cases 1 and 2 - are you not seeing this happen in some case?

@scheibelp yes, we call setup_dependent_build_environment in cases 1 and 2 but I would like to avoid running the rpath-related logic when it is not needed, e.g. some packages do not extend python but need the interpreter at the build time.

@adamjstewart I wanted to warn the users for whatever reason but then I realised that the warning was almost always emitted if python was an external package. In my case, the warnings were irrelevant because I didn't build any extensions but packages that needed the interpreter only to run some scripts at the build time. So, I thought of triggering the whole logic (including the warning) only when it was relevant.

Anyway, this solution is overcomplicated (as the original one). I think #21149 is better. Please, take a look.

yes, we call setup_dependent_build_environment in cases 1 and 2 but I would like to avoid running the rpath-related logic when it is not needed, e.g. some packages do not extend python but need the interpreter at the build time.

I think I understand: if we have a build dependency on Python but we are not building a Python extension, then there is no need to set LDSHARED. We could avoid this by moving logic into PythonPackage (this would be similar to 3 in your proposal at #16527 (comment), but most packages with extension-specific logic place it in the associated build_systems/ module).

(also I'm looking at #21149)