Skip to content

libint2: moved sanity check and update CPATH#300

Merged
boegel merged 3 commits intoeasybuilders:developfrom
wpoely86:libint2
Nov 12, 2013
Merged

libint2: moved sanity check and update CPATH#300
boegel merged 3 commits intoeasybuilders:developfrom
wpoely86:libint2

Conversation

@wpoely86
Copy link
Copy Markdown
Member

@wpoely86 wpoely86 commented Nov 8, 2013

No description provided.

@hpcugentbot
Copy link
Copy Markdown

Automatic reply from Jenkins: Can I test this?

Comment thread easybuild/easyblocks/l/libint2.py Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why did you remove this?

This acts as a (proper) default sanity check that is specific to Libint2 (i.e., not simply bin, lib).

You can always override sanity_check_paths in the easyconfig file, but having a proper default should remain so imho.

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.

I moved it to the easyconfig. I thought it is nicer to put this kind of thing in the config instead of the block?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There should be a good default in the easyblock, if there is an easyblock.

If sanity_check_paths is also defined in the easyconfig, then that overrides whatever is in the easyblock.

Don't remove this, it should be there, and you're also breaking backward compatibility this way.

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.

How am I breaking backwards compatibility here? It doesn't get checked but that shouldn't break anything? All the easyconfigs for libint2 have changed to include a sanity_check_paths.

It's no problem to move it back but I don't entirely see the point? Why put this in the easyblock? It seems more sensible to put this in the easyconfig as files and paths can change in the future?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There might be libint2 copies of easyconfig files out there that do not specify sanity_check_paths, and those will be broken (unless libint2 provides a non-empty bin and lib dir, which is what EB checks by default).

If there is an easyblock, having a good default for sanity_check_paths there allows to remove at least 4 lines from the easyconfig, thus cleans things up.
Even if files differ across versions, I would still prefer adding a version check in the easyblock rather than copying all the sanity check paths to the easyconfigs... Those should be as clean as possible, imho.

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.

OK, I have re-added the check (The bin dir will be empty).

But I still don't agree with doing this in the block. I think only install logic should be done here but this is just a list of files that can change from version to version. I would rather keep the block clean and move as much as possible to the general framework (with sensible defaults) and the easyconfigs.

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.

+1 for wpoely86's last comment;

my stance is, that if we don't push more of the easyblocks' code towards framework (and generic easyblocks), and of course easyconfigs, we have not really understood the generalization of build procedures well enough.

@boegel
Copy link
Copy Markdown
Member

boegel commented Nov 12, 2013

Jenkins: ok to test

@boegel
Copy link
Copy Markdown
Member

boegel commented Nov 12, 2013

retested with easybuilders/easybuild-easyconfigs#506, works fine, so merging in

boegel added a commit that referenced this pull request Nov 12, 2013
libint2: moved sanity check and update CPATH
@boegel boegel merged commit 496fa6b into easybuilders:develop Nov 12, 2013
@wpoely86 wpoely86 deleted the libint2 branch November 12, 2013 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants