Skip to content

look for Python version directories with suffixes in ROOT easyblock#2010

Merged
lexming merged 1 commit intoeasybuilders:developfrom
kelseymh:2009_root_Python_dirs
Apr 12, 2020
Merged

look for Python version directories with suffixes in ROOT easyblock#2010
lexming merged 1 commit intoeasybuilders:developfrom
kelseymh:2009_root_Python_dirs

Conversation

@kelseymh
Copy link
Copy Markdown
Contributor

@kelseymh kelseymh commented Apr 3, 2020

Implementation adapted from blender.py. Addresses issue #2009 .

@boegel boegel changed the title In root.py, look for Python version directories with suffixes. Imple… look for Python version directories with suffixes in ROOT easyblock Apr 12, 2020
@boegel boegel added the bug fix label Apr 12, 2020
@boegel boegel added this to the next release (4.2.0) milestone Apr 12, 2020
Copy link
Copy Markdown
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

Tested ROOT-6.14.06-foss-2018b-Python-3.6.6.eb from easybuilders/easybuild-easyconfigs#10324 and paths to Python installation are correctly set

-DPYTHON_INCLUDE_DIR=/apps/brussel/CO7/ivybridge-ib/software/Python/3.6.6-foss-2018b/include/python3.6m 
-DPYTHON_LIBRARY=/apps/brussel/CO7/ivybridge-ib/software/Python/3.6.6-foss-2018b/lib/libpython3.6m.so

Copy link
Copy Markdown
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

Tested with ROOT-6.14.06-foss-2018b-Python-2.7.15.eb and the paths to Python installation were correctly set to

-DPYTHON_INCLUDE_DIR=/apps/brussel/CO7/ivybridge-ib/software/Python/2.7.15-foss-2018b/include/python2.7
-DPYTHON_LIBRARY=/apps/brussel/CO7/ivybridge-ib/software/Python/2.7.15-foss-2018b/lib/libpython2.7.so

Copy link
Copy Markdown
Contributor

@lexming lexming left a comment

Choose a reason for hiding this comment

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

Both builds finished without issues.

LGTM

@lexming
Copy link
Copy Markdown
Contributor

lexming commented Apr 12, 2020

Going in, thanks @kelseymh !

@lexming lexming merged commit 22761ce into easybuilders:develop Apr 12, 2020
@kelseymh kelseymh deleted the 2009_root_Python_dirs branch April 12, 2020 15:23
@mboisson
Copy link
Copy Markdown
Contributor

Instead of copying find_glob_pattern, this function should have been implemented in filetools.py and imported in both easyblocks.

@kelseymh
Copy link
Copy Markdown
Contributor Author

@mboisson You are quite right, of course. Since this PR is already merged, I will open a new one with that improvement.

@mboisson
Copy link
Copy Markdown
Contributor

mboisson commented Apr 22, 2020

I created a PR for the framework for it here already
easybuilders/easybuild-framework#3297

We only need the PR for the EasyBlocks (root.py and blender.py)

@kelseymh
Copy link
Copy Markdown
Contributor Author

I'll do that. Something I don't know how to deal with, though, is handling the cross-repository dependence. Should I just wait until your PR #3297 gets merged, and then submit mine? Or is there a way to specify the dependence as part of the PR process? Sorry for my GitHub ignorance :-/

@mboisson
Copy link
Copy Markdown
Contributor

mboisson commented Apr 22, 2020

I don't know that there is an official mechanism. I usually just comment on the PR that this PR requires that PR.

@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 23, 2020

Instead of copying find_glob_pattern, this function should have been implemented in filetools.py and imported in both easyblocks.

You're very right, and I thought of that, but I had to admit I was being lazy at the time (we were close to the EasyBuild v4.2.0 release, and I wanted this fix included in it).
I figured that if it came up again, or if somebody cared enough, we could still support it in framework later.

Thanks for bringing this up, and helping out!

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.

4 participants