Skip to content

make sure lib/python*/lib-dynload exists in Python installation#1966

Merged
akesandgren merged 3 commits intoeasybuilders:developfrom
boegel:python_opensuse_fix
Apr 6, 2020
Merged

make sure lib/python*/lib-dynload exists in Python installation#1966
akesandgren merged 3 commits intoeasybuilders:developfrom
boegel:python_opensuse_fix

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Feb 18, 2020

see #1957

@kserradell Are you up for testing this fix on OpenSUSE?

@boegel boegel added the bug fix label Feb 18, 2020
@boegel boegel added this to the next release (4.1.2?) milestone Feb 18, 2020
Comment thread easybuild/easyblocks/p/python.py
Comment thread easybuild/easyblocks/p/python.py
Comment thread easybuild/easyblocks/p/python.py
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Mar 3, 2020

@akesandgren This is good to go imho, the remaining problem in #1957 is something else entirely it seems...

Comment thread easybuild/easyblocks/p/python.py Outdated
mkdir(lib_dynload_parent, parents=True)
cwd = change_dir(lib_dynload_parent)
# use relative path as target, to avoid hardcoding path to install directory
target_lib_dynload = os.path.join('..', '..', 'lib64', 'python%s' % self.pyshortver, lib_dynload)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In line with your usual comments for code that repeats itself...
put os.path.join('lib64', 'python%s' % self.pyshortver, lib_dynload))
into some variable and reuse in both places
or even the ('python%s' % self.pyshortver, lib_dynload) part...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in b73b6ed

cwd = change_dir(lib_dynload_parent)
# use relative path as target, to avoid hardcoding path to install directory
target_lib_dynload = os.path.join('..', '..', 'lib64', 'python%s' % self.pyshortver, lib_dynload)
symlink(target_lib_dynload, lib_dynload)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this need to be symlink(target_lib_dynload, lib_dynload, use_abspath_source=False) to make sure it doesn't get fiddled with...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we want to avoid hardcoding the path to the installation directory in the symlink (we had a good reason to use ../.. rather than using the full path even)

…nce in lib-dynload check in Python easyblock
Copy link
Copy Markdown
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@akesandgren
Copy link
Copy Markdown
Contributor

Going in, thanks @boegel!

@akesandgren akesandgren merged commit 834c68d into easybuilders:develop Apr 6, 2020
@boegel boegel deleted the python_opensuse_fix branch April 7, 2020 07:53
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.

3 participants