Skip to content

allow setting parallel to 0 or False to disable adding the -j argument#2842

Merged
boegel merged 3 commits intoeasybuilders:developfrom
migueldiascosta:fix_parallel
Apr 30, 2019
Merged

allow setting parallel to 0 or False to disable adding the -j argument#2842
boegel merged 3 commits intoeasybuilders:developfrom
migueldiascosta:fix_parallel

Conversation

@migueldiascosta
Copy link
Copy Markdown
Member

motivation: ConfigureMake, MakeCp, etc. easyblocks allow setting build_cmd to something other than make, but without this change -j PARALLEL is always added anyway

this change is enough because self.cfg['parallel'] is tested again in the configuremake build step, before adding -j

@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 26, 2019

@migueldiascosta We should cover the use case of using parallel = False in the tests, see test_parallel in test/framework/easyblock.py

Comment thread test/framework/easyblock.py Outdated
# make sure 'parallel = False' is not overriden
test_eb = EasyBlock(EasyConfig(toy_ec3))
test_eb.check_readiness_step()
self.assertEqual(test_eb.cfg['parallel'], False)
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.

@migueldiascosta Looks good, but you should also do the same test in the context where --parallel is not used (i.e. copy-paste this stuff above line 1372 where --parallel 97 is put in place, to ensure parallel = False works as expected in both contexts.

Side-note: this test only passes because 0 == False evaluates to True in Python...

When --parallel is also specified, the current codes takes the minimum of that value (97 in the test) and the int values of the parallel easyconfig parameter; with int(False) returning 0, you get min(97, 0) or just 0.

That's not necessarily a problem though, but maybe we should check for 0 explicitly here instead?
We should keep checking against False in the context where --parallel is not set though...

@boegel
Copy link
Copy Markdown
Member

boegel commented Apr 30, 2019

Going in, thanks for the fix @migueldiascosta!

@boegel boegel merged commit 9bc93a8 into easybuilders:develop Apr 30, 2019
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.

2 participants