Add settings for EasyBuild 5+ to EESSI-extend #45
Conversation
| setenv ("EASYBUILD_LOCAL_VAR_NAMING_CHECK", "error") | ||
| -- Set environment variables that are EESSI version specific | ||
| if convertToCanonical(eessi_version) > convertToCanonical("2023.06") then | ||
| setenv ("EASYBUILD_PREFER_PYTHON_SEARCH_PATH", "EBPYTHONPREFIXES") |
There was a problem hiding this comment.
I don't have strong feelings about this, and I do see the appeal to use $EBPYTHONPREFIXES rather than $PYTHONPATH (works better when using Python virtual environments), but this is in some sense also uncharted territory, so we're bound to run into surprises/problems because we're using this.
Also, not relying on the standard mechanism ($PYTHONPATH) is no doubt going to confuse end users at some point.
So, we should clearly motivate why we're going down this road, since we can't change our mind in the middle, this is an all-or-nothing choice...
There was a problem hiding this comment.
The motivation is exactly that, better venv behaviour, but I agree with the main concern about how hard it is to roll back. I was actually hoping you would be the one to sooth me.
End users can still use PYTHONPATH, but we wouldn't. Or are you more concerned about those using EESSI-extend?
There was a problem hiding this comment.
I'm all for EBPYTHONPREFIXES. We have yet to find issues and it allows using venvs which is essential at least to our users.
There was a problem hiding this comment.
I'm mostly concerned about people being confused as to how Python packages can be imported from non-standard locations while we're not using $PYTHONPATH.
The $EBPYTHONPREFIXES approach seems robust, but it's very custom.
That said, I'm not opposing it, just making sure we're aware that we're (yet again) taking a non-standard route here, and there may be dragons along that route...
We should probably add some documentation on the different ways in which we divert from default EasyBuild configuration, and explain the motivation behind those settings => EESSI/docs#519
There was a problem hiding this comment.
EBPYTHONPREFIXES isn't the default? I thought we would change that for EB 5
There was a problem hiding this comment.
It's not, we kept $PYTHONPATH as default search path (pretty much for the same concerns as I raised here):
--prefer-python-search-path=PREFER-PYTHON-SEARCH-PATH
Prefer using specified environment variable when
possible to specify where Python packages were
installed; see also https://docs.easybuild.io/python-
search-path (type choice; default: PYTHONPATH)
(choices: PYTHONPATH, EBPYTHONPREFIXES)
That option is there so you can opt-in to using it though
| if convertToCanonical(eessi_version) > convertToCanonical("2023.06") then | ||
| setenv ("EASYBUILD_PREFER_PYTHON_SEARCH_PATH", "EBPYTHONPREFIXES") | ||
| setenv ("EASYBUILD_MODULE_SEARCH_PATH_HEADERS", "include_paths") | ||
| setenv ("EASYBUILD_SEARCH_PATH_CPP_HEADERS", "include_paths") |
There was a problem hiding this comment.
Somewhat similar, why don't we just stick to the default (using $CPATH) here, is the case strong enough to go non-standard here by using $C_INCLUDE_PATH & co instead of $CPATH?
There was a problem hiding this comment.
I think this one is a motivated change IIRC, something related to the DPC++ compiler...will need to dig it out
There was a problem hiding this comment.
The issue and resolution is in easybuilders/easybuild-framework#3331 (comment) , but that doesn't really help me to be any clearer on what we should do.
@Flamefire You were involved in that discussion, what do you think? Is it enough to only change EASYBUILD_MODULE_SEARCH_PATH_HEADERS?
There was a problem hiding this comment.
I have seen issues fixed with the new EASYBUILD_MODULE_SEARCH_PATH_HEADERS but am not aware of the same for EASYBUILD_SEARCH_PATH_CPP_HEADERS but it seems sensible and I don't expect issues
I'd say this one is less problematic. If stuff really does break we can unset/change EASYBUILD_SEARCH_PATH_CPP_HEADERS because it isn't ingrained in module files except if the build did pick up wrong includes but I don't see how
There was a problem hiding this comment.
Same here, not opposing it, just making sure we understand the potential implications (which includes confusing users by using a not-so-common approach to specifying the paths to where header files can be found).
It's also worth being aware that $CPATH has precedence over the -isystem compiler option (which sometimes appears in Makefiles), while $C_INCLUDE_PATH do not (so it opens us up potential trouble/surprises in case something like -isystem /usr/include is used).
In this case, we can actually divert from it relatively easy where needed I think...
There was a problem hiding this comment.
I don't think it completely does go both ways. The problem is that CPATH is set in module files and that takes precedence, we can always increase precedence in our builds by setting CPATH in a build (to the value of C_INCLUDE_PATH) if we need to. We could indeed also handle the reverse.
The issue is more on the user side, you are asking them to diagnose and do the same thing if they run into the CPATH problem, whereas they won't see it in the other case.
There was a problem hiding this comment.
I'd say this one is less problematic. If stuff really does break we can unset/change EASYBUILD_SEARCH_PATH_CPP_HEADERS because it isn't ingrained in module files except if the build did pick up wrong includes but I don't see how
I'm confused. So does that mean that irrespective of this setting, in the modulefile EasyBuild will always set CPATH? But then if EASYBUILD_SEARCH_PATH_CPP_HEADERS=include_path, in builds done with EasyBuild, the contents of CPATH are moved to C_INCLUDE_PATH (i.e. CPATH is also unset)? The explaination of the option in EB doesn't really make this clear... But if that's indeed the case users that compile code get the CPATH set, regardless of this setting, right?
The issue is more on the user side, you are asking them to diagnose and do the same thing if they run into the CPATH problem, whereas they won't see it in the other case.
So this is not true then, i.e. for the user (that compiles something), this setting changes nothing.
Yes, you're overriding build system behavior with CPATH. Sometimes that's desirable, sometimes not - it often depends on if the programmer who wrote the build system config knew what they were doing. Some packages also insert flags like -march=native, just assuming that no one ever wants to cross-compile. I know that's not related to the include paths discussion, but just to illustrate that overriding what the build system config does by default is sometimes a good idea.
Anyway, I think both options have equal chances of causing issues, and therefore I am ok in keeping setting this to include_path. If nothing else, it'll allow us to build experience and see if this produces fewer issues. And if not, we switch back.
There was a problem hiding this comment.
No, EasyBuild does not always set CPATH, that (now) depends on value of EASYBUILD_MODULE_SEARCH_PATH_HEADERS (IIUC)
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm confused. So does that mean that irrespective of this setting, in the modulefile EasyBuild will always set
CPATH?
The variables used in module files are changed with EASYBUILD_MODULE_SEARCH_PATH_HEADERS
EASYBUILD_SEARCH_PATH_CPP_HEADERS governs how toolchain paths are set. I'm not fully sure what this actually means as everything should be done by the modules making up the components of the toolchain but seems there are some paths that get set additionally.
ocaisa
force-pushed
the
easybuild_config_updates
branch
from
9b652fe to
fd7ed92
Compare
@boegel Would be great if you can review this and see if I missed anything (and if my choices are "correct")