Skip to content
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

enable 'check_ldshared' in generic PythonPackage easyblock by default #1788

Merged
merged 2 commits into from
Sep 15, 2019

Conversation

boegel
Copy link
Member

@boegel boegel commented Aug 27, 2019

replacement for #1781, now to correct branch (develop)

@Micket
Copy link
Contributor

Micket commented Aug 27, 2019

I'll just copy my previous comment:
So, do we not want to make this dependent on toolchain version?
For reference:
#1455 (comment)

Alternatively, we could go through old easyconfigs where it's currently not set and set check_ldconfig=False?
We could just find all PythonPackage/PythonBundle configs for intel/iccifort/iimpi/iimpic/intelcuda that currently doesn't have check_ldconfig set at all, and apply check_ldconfig = False

@boegel
Copy link
Member Author

boegel commented Aug 27, 2019

@Micket The implementation is pretty safe, since it checks first whether the $LDSHARED used to build Python packages already uses the toolchain compiler.

If not (when intel toolchain is used together with Python built with GCCcore), only then is a different $LDSHARED value set.

So, I don't think we need to make this specific for certain toolchains, or go back to easyconfigs and set check_ldshared = False specifically.

We can do that in case having this enabled by default is problematic, but it shouldn't be (and if it is, we can still try to fix that).

We're changing this default for the upcoming EasyBuild v4.0 release, which gives us an excuse to make "breaking" changes (although nothing should actually break due to this change).

@Micket
Copy link
Contributor

Micket commented Aug 27, 2019

I know, but, but the reason why we did it "opt-in" to start with hasn't actually changed; it broke some existing old configs. These old configs relies on broken behavior of distutils.

To be specific, merging this PR would break the build for: mpi4py-2.0.0-intel-2016b-Python-2.7.12.eb (and several others equally old configs).

I'm not sure if we really care about these old versions (I don't use them), but, it also wouldn't be that terribly hard to fix them. Or drop them.

@boegel
Copy link
Member Author

boegel commented Aug 27, 2019

@Micket OK, just re-discovered my own fairly thorough testing of this (cfr. #1455 (comment)), you're right...

I'm not a big fan of hunting down easyconfigs where we should set check_ldshared = False, so we should come up with an additional check before we fiddle with $LDSHARED...

Maybe something as simple as simply implementing a bypass for any Python 2.x versions older than 2.7.15 and Python 3.x versions older than 3.7.2 is enough?

@Micket
Copy link
Contributor

Micket commented Aug 27, 2019

But we also can't put a runtime check inside extra_options so shall we instead do:

'check_ldshared': [None, 'Check Python value of $LDSHARED, correct if needed to "$CC -shared"', CUSTOM],

and then, then used:

if (check_ldshared is None and old_broken_versions) or check_ldshared == False:
    # Not touching LDSHARED
else:
   # Do touch LDSHARED

(ugh.. so ugly, perhaps a 3 value enum would be better)

…versions if it is not explicitely enabled or disabled
@boegel
Copy link
Member Author

boegel commented Sep 15, 2019

@Micket Updated to check_ldshared is only set to True for sufficiently recent Python versions, should be good to go now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants