-
Notifications
You must be signed in to change notification settings - Fork 720
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add CI test checking if the Python default source_urls are used and Fix CI check where use_pip=False was ignored #12456
Add CI test checking if the Python default source_urls are used and Fix CI check where use_pip=False was ignored #12456
Conversation
When that is used in an EC the check will handle that the same as if it was not set at all and read the exts_default_options instead which will make the test fail although it should not
test/easyconfigs/easyconfigs.py
Outdated
use_pip = ec.get('use_pip') or exts_default_options.get('use_pip') | ||
download_dep_fail = ec.get('download_dep_fail') | ||
exts_download_dep_fail = ec.get('exts_download_dep_fail') | ||
use_pip = ec.get('use_pip', default=exts_default_options.get('use_pip')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is correct...
ec.get('use_pip')
will always work for PythonBundle
, even if the easyconfig file does not define use_pip
to False
or True
, because use_pip
is a known easyconfig parameter for PythonBundle
:
$ eb -a -e PythonBundle | grep use_pip | head -1
use_pip* Install using 'pip install --prefix=%(prefix)s %(installopts)s %(loc)s' [default: None]
So you'll never fall back to exts_default_options.get('use_pip')
.
The existing line is incorrect too though, we should fall back to exts_default_options.get('use_pip')
when use_pip
is still set to None
(the default) rather than to False
or True
explicitly. It's currently wrong when use_pip = False
is used, but this isn't the correct way of fixing that problem...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, true. I forgot that those are then set by default
@boegel On a related issue: Why isn't If there is nothing I missed, I'd add that to this PR as it is closely related |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
I'm not sure it makes sense to run How would running I would keep that for a separate PR, since the default source URLs and using |
This is for #12451, #12452, #12453 to avoid reintroducing that
The 2nd commit fixes the failure seen in #12453: https://github.com/easybuilders/easybuild-easyconfigs/runs/2174370525