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

define $LDSHARED when installing Python packages when Python's value doesn't use toolchain compiler ($CC) #1455

Merged
merged 11 commits into from Jan 25, 2019

Conversation

Micket
Copy link
Contributor

@Micket Micket commented Jul 8, 2018

Fix for setuptools at GCCcore level for easybuilders/easybuild-easyconfigs#6537

easybuild/easyblocks/generic/pythonpackage.py Outdated Show resolved Hide resolved
@boegel boegel added this to the 3.7.0 milestone Aug 21, 2018
@boegel boegel modified the milestones: 3.7.0, next release Sep 18, 2018
@boegel boegel modified the milestones: 3.7.1, next release Oct 10, 2018
@boegel boegel modified the milestones: 3.8.0, 3.x Dec 12, 2018
@boegel boegel modified the milestones: 3.x, 3.8.1 Dec 20, 2018
@boegel
Copy link
Member

boegel commented Jan 24, 2019

While testing this a bit, I ran into a problem because an assumption is being made that $CC is always set:

  File "/tmp/eb-RG7KYO/included-easyblocks/easybuild/easyblocks/generic/pythonpackage.py", line 462, in build_step
    env.setvar("LDSHARED", curr_cc + " -shared")
TypeError: unsupported operand type(s) for +: 'NoneType' and 'str'

The problem is that $CC is not defined (by EasyBuild) for Python packages that get installed on top of the system Python, with a dummy toolchain.
That includes EasyBuild itself for example, but we also have easyconfigs for PyYAML, vsc-* tools, etc. like that.

Proposed fix in Micket#1 .

Another thing I mentioned is that we're only checking/setting $LDSHARED here when setup.py is being used. @Micket Is that intentional?
If not, we should move this block of code to the configure_step (outside of an if scope)?

take into account that $CC may not be set when checking value of Python's $LDSHARED
@Micket
Copy link
Contributor Author

Micket commented Jan 24, 2019

@boegel Regarding; setup.py
As far as I know, this should always apply whenever distutils is used to build the package.

So, if there are builds out there that doesn't use a setup.py, but still relies on this part of distutils, then it should indeed be placed outside the condition.

There certainly shouldn't be any downside, as any customized build systems would probably neither rely on LDSHARED, or should still use CC for linkage anyway.

@boegel
Copy link
Member

boegel commented Jan 24, 2019

@Micket What about if pip is used? I'm not sure that rules out that distutils is involved...

I would always set $LDSHARED when needed, regardless of whether setup.py is used or not. If that causes problems we can revisit this when we have examples where it's problematic?

@Micket
Copy link
Contributor Author

Micket commented Jan 24, 2019

@boegel Sorry, apparently i missed pushing the last update.

@boegel boegel dismissed damianam’s stale review January 24, 2019 17:42

remarks taken into account

boegel
boegel previously approved these changes Jan 24, 2019
@boegel boegel changed the title Add LDSHARE env.var. for setuptools @ GCCcore define $LDSHARED when installing Python packages when Python's value doesn't use toolchain compiler ($CC) Jan 25, 2019
@boegel
Copy link
Member

boegel commented Jan 25, 2019

@Micket I tested this with all the Python easyconfigs we have (see https://github.com/easybuilders/easybuild-easyconfigs/tree/develop/easybuild/easyconfigs/p/Python), and it turns out we'll need to be significantly more careful than we are now...

For older versions (up until Python 2.7.12 and 3.5.2 apparently), Python's $LDSHARED value always uses gcc, even when Python was installed with an Intel-based toolchain (ictce, intel, iomkl).

From the log (this was with Python-3.5.2-intel-2016b.eb, but same issue with Python versions 2.7.12 or older 2.x and 3.5.2 or older 3.x):

== 2019-01-25 11:06:02,619 pythonpackage.py:443 INFO Python's value for $LDSHARED ('gcc -pthread -shared -Wl,-z,relro') doesn't use current $CC value ('icc'), fixing
== 2019-01-25 11:06:02,619 environment.py:97 INFO Environment variable LDSHARED set to icc -shared (previously undefined)

This then causes problems when mpi4py is installed as a part of the Python module:

/prefix/software/impi/5.1.3.181-iccifort-2016.3.210-GCC-5.4.0-2.26/bin64/mpiicc icc -shared -I...
icc: error #10236: File not found:  'icc'

It seems like during the mpi4py installed, the value of $LDSHARED is taken, and then the gcc that is expected to be in there is replaced with /path/to/mpicc. And then this causes problems when gcc is no longer there, leading to a broken command.

So, I think we have two options:

i) We add a version check to only do this for recent Python versions (having to do that for both Python 2 & 3 complicates that a bit).

i)) We make it opt-in only, by adding a custom easyconfig parameter like check_ldshared, which you would then need to set like check_ldshared = True in easyconfig files where relevant.

In any case, we clearly need to be more careful with tweaking $LDSHARED.

I was hoping to get this in for EasyBuild v3.8.1, but now I'm not so sure to squeeze this in last-minute...

@Micket
Copy link
Contributor Author

Micket commented Jan 25, 2019

Just to keep a public record from our discussion: I vote for opt-in, at least for 3.8.1, as it at least allows conveniently testing out alternatives.

…ed' easyconfig parameter in PythonPackage easyblock
make checking/correcting of $LDSHARED opt-in via custom 'check_ldshared' easyconfig parameter in PythonPackage easyblock
@boegel
Copy link
Member

boegel commented Jan 25, 2019

Spent a bit more time testing this, both the (unchanged) default behavior and the use of check_ldshared via:

exts_default_options = {'check_ldshared': True}

in a Python easyconfig, works as expected, so good to go.

Thanks a lot for your work on this @Micket!

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

Successfully merging this pull request may close these issues.

None yet

4 participants