Skip to content

(correctly) define $ROSETTA3_DB in Rosetta easyblock#1092

Merged
boegel merged 2 commits intoeasybuilders:developfrom
boegel:rosetta_db
Jan 31, 2017
Merged

(correctly) define $ROSETTA3_DB in Rosetta easyblock#1092
boegel merged 2 commits intoeasybuilders:developfrom
boegel:rosetta_db

Conversation

@boegel
Copy link
Copy Markdown
Member

@boegel boegel commented Jan 31, 2017 •
edited
Loading

$ROSETTA_DB was being incorrectly defined in the Rosetta easyconfigs (cfr. easybuilders/easybuild-easyconfigs#4083)

@boegel boegel added this to the 3.1.0 milestone Jan 31, 2017
@boegel boegel mentioned this pull request Jan 31, 2017
Merged
@damianam
Copy link
Copy Markdown
Member

damianam commented Jan 31, 2017

@boegel your changes lgtm, even though I think the title of the PR is misleading (you are setting ROSETTA3_DB not ROSETTA_DB). But I have 2 comments regarding old pieces of code that you might want to fix. Since I can't add line comments on lines that haven't been changed on the PR, I'll write them here:

  • Line 70: Strictly speaking, that can be wrong. len(subdirs) could be 0, and the error would be misleading. I doubt you'll ever get to that though.

  • Method detect_cxx: Shouldn't you be checking for CXX_SEQ or CXX, rather than CC_SEQ or CC? If for whatever reason that is correct, I'd suggest to rename the method to detect_cc, or at least add a comment to clarify it.

@boegel boegel changed the title (correctly) define $ROSETTA_DB in Rosetta easyblock (correctly) define $ROSETTA3_DB in Rosetta easyblock Jan 31, 2017
@boegel
Copy link
Copy Markdown
Member Author

boegel commented Jan 31, 2017

@damianam thanks for the thorough review! Remarks fixed, please rereview?

@damianam
Copy link
Copy Markdown
Member

damianam commented Jan 31, 2017

@boegel lgtm

@boegel
Copy link
Copy Markdown
Member Author

boegel commented Jan 31, 2017

tested with easybuilders/easybuild-easyconfigs#4083 and easybuilders/easybuild-easyconfigs#4085, good to go, thanks @damianam!

@boegel boegel merged commit d7ebec1 into easybuilders:develop Jan 31, 2017
@boegel boegel deleted the rosetta_db branch January 31, 2017 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.1.0

Development

Successfully merging this pull request may close these issues.

2 participants

@boegel @damianam