flux-core and flux-sched: disables environment variable setting for externally found Flux

[ilumsden]

While trying to test a Spack package I am currently developing for a new benchmark in Benchpark, I found that environments where Flux is found externally (i.e., flux-core in the packages section of spack.yaml) will fail to install. When digging into this issue, I found that the root of the problem was the use of self.spec["lua"] to set environment variables (e.g., LUA_PATH and LUA_CPATH) in the setup_run_environment function of the flux-core package. So, this issue I ran into is essentially a variant of #9149.

However, in the case of flux-core, this issue can be easily fixed by adding a conditional that disables the code using self.spec["lua"] when flux-core is external. Additionally, since the environment variables set in setup_run_environment should already by set for an external install, this change could avoid possible corner cases that might have popped up when using an external install of Flux (e.g., the system-wide install on Corona, which is where I encountered this issue).

Finally, I also noticed the same logic regarding self.spec["lua"] and environment variables in the flux-sched package, so I've also applied this fix to that package.

[grondo]

This LGTM, but I don't think I know enough about spack to fully review this one. @vsoch might be a better candidate for reivew.

For externally found packages, does spack really use the environment provided by the spack package? Shouldn't it just ignore the spack package for external packages, since they are coming from the default environment? (I'm wondering if we're doing something else wrong in the flux packages that caused this bug)

[grondo]

Also, I'm not sure if the spack project has any guidelines for commit messages, but I'd suggest shorting the commit subjects here and adding something to the body, e.g.

flux-core: fix environment for externally found package

Add a conditional that avoids setting package environment...
[and so on...]

[vsoch]

This change is saying that if flux is provided as an external, it's up to the external provider to set the LUA paths, which I think is logical. The issue is that adding it as an external is going to look for spec with lua, and that likely is not present if lua was only a dependent for flux. It would still not work to add it, because it might be an actual mismatch in version. My only critique might be to check for external and just return early, e.g.,:

# If flux-X is provided as an external, the developer is responsible for the
# install of LUA and setting these paths.
if self.spec.external:
    return

and then not indent the rest of the function. It would also be good to add a comment above that, explaining the logic for a future package.py reader.

[ilumsden]

Great points @grondo @vsoch. I'll make those changes.

[grondo]

Nice! I'd just squash to two commits for each package, since the second commit undoes most of the first. Thanks for doing this!

[ilumsden]

@grondo just squashed the commits

[grondo]

Looks great, thanks!