Modify robot so that it only appends dependencies of tweaked easyconfigs#2090
Modify robot so that it only appends dependencies of tweaked easyconfigs#2090boegel merged 12 commits intoeasybuilders:developfrom
Conversation
…n on the command line when using --try-toolchain
| tweaked_ecs_paths = None | ||
| if tweaked_ecs: | ||
| tweaked_ecs_path = os.path.join(tmpdir, 'tweaked_easyconfigs') | ||
| tweaked_ecs_paths = [os.path.join(tmpdir, 'tweaked_easyconfigs'), os.path.join(tmpdir, 'tweaked_dep_easyconfigs')] |
There was a problem hiding this comment.
I guess this is probably better off being a dictionary
| robot_path.insert(0, tweaked_ecs_paths['tweaked_ecs_path']) | ||
| # dependencies are always tweaked but we should only use them if there is no other option (so they come last) | ||
| robot_path.append(tweaked_ecs_paths['tweaked_ecs_deps_path']) | ||
| _log.info("Prepended list of robot search paths with %s and appended with %s: %s" |
There was a problem hiding this comment.
@ocaisa style nitpicking: use , rather than %
log.info("...",
arg1, arg2, arg3)| if tweaked_ecs: | ||
| tweaked_ecs_path = os.path.join(tmpdir, 'tweaked_easyconfigs') | ||
| tweaked_ecs_paths = {'tweaked_ecs_path': os.path.join(tmpdir, 'tweaked_easyconfigs'), | ||
| 'tweaked_ecs_deps_path': os.path.join(tmpdir, 'tweaked_dep_easyconfigs')} |
There was a problem hiding this comment.
@ocaisa using a dictionary here is overkill imho, I think a 2-tuple is more appropriate here
tweaked_ecs_paths = (os.path.join(tmpdir, 'tweaked_easyconfigs'), os.path.join(tmpdir, 'tweaked_dep_easyconfigs'))
There was a problem hiding this comment.
That's what I had originally but I felt the use later when constructing the robot path and dumping the tweaked easyconfigs wasn't very clear and could potentially lead to problems later. I don't mind changing it back but I thought this was a bit more explicit when it was being used.
There was a problem hiding this comment.
Using tweaked_ecs_paths[0]/target_dirs[0] and tweaked_ecs_paths[1]/target_dirs[1] is fairly clear to me ([0] for path to prepend, [1] for path to append)
Maybe you can also split it up like this to make it more clear:
tweaked_ecs_paths, tweaked_ecs_deps_path = target_dirsThat's even better, since it leaves no question how large the target_dirs tuple is, and it makes it clear it's not a list of directories as the name suggests.
The problem with a dictionary is that you could update it somewhere along the way to change the elements or add more etc., which is not the intention here. A tuple is immutable (although, admittedly, the list values in the tuple can be updated themselves).
|
@ocaisa Small remark. We'll need a (better) test for this too, let me know if you need any help with that. |
|
I'm not sure how to design the test for this. I can use the |
| """Tweak list of easyconfigs according to provided build specifications.""" | ||
|
|
||
| if targetdirs is not None: | ||
| tweaked_ecs_path, tweaked_ecs_deps_path = targetdirs |
There was a problem hiding this comment.
better initialise tweaked_ecs_path and tweaked_ecs_deps_path in case targetdirs is None, to avoid using undefined variables?
| if tweaked_ecs: | ||
| tweaked_ecs_path = os.path.join(tmpdir, 'tweaked_easyconfigs') | ||
| tweaked_ecs_paths = (os.path.join(tmpdir, 'tweaked_easyconfigs'), | ||
| os.path.join(tmpdir, 'tweaked_dep_easyconfigs')) |
There was a problem hiding this comment.
I think this fits on a single line in the 120-chars limit?
There was a problem hiding this comment.
Nope, 2 characters too many
| """Tweak list of easyconfigs according to provided build specifications.""" | ||
|
|
||
| if targetdirs is not None: | ||
| tweaked_ecs_path, tweaked_ecs_deps_path = targetdirs |
There was a problem hiding this comment.
better initialise tweaked_ecs_path and tweaked_ecs_deps_path in case targetdirs is None, to avoid using undefined variables?
|
@ocaisa w.r.t. the test, how about doing a |
|
@boegel The failing test is because it is failing to process the OpenMPI easyconfig on Travis. On my local machine this is not a problem...am I missing some kind of initialisation in the test? |
|
@boegel Ok, tests are now passing, ready for a final review |
boegel
left a comment
There was a problem hiding this comment.
just some minor code style remarks in the test, looks great!
|
|
||
| # Create directories to store the tweaked easyconfigs | ||
| eb_tmpdir = tempfile.gettempdir() | ||
| tweaked_ecs_paths, pr_path = alt_easyconfig_paths(eb_tmpdir, tweaked_ecs=True) |
There was a problem hiding this comment.
@ocaisa please use self.test_prefix instead of eb_tmpdir, which is a sandbox created for every test that is also cleaned up automagically
|
|
||
| # Parse the easyconfig that we want to tweak | ||
| untweaked_openmpi = os.path.join(test_easyconfigs, 'o', 'OpenMPI', 'OpenMPI-1.6.4-GCC-4.6.4.eb') | ||
| self.assertTrue(os.path.isfile(untweaked_openmpi)) |
There was a problem hiding this comment.
hmm, this is overkill, you'd get a failure anyway from parse_easyconfigs?
|
|
||
| # Check that all expected tweaked easyconfigs exists | ||
| tweaked_openmpi = os.path.join(tweaked_ecs_paths[0],'OpenMPI-1.6.4-GCC-4.7.2.eb') | ||
| tweaked_hwloc = os.path.join(tweaked_ecs_paths[1],'hwloc-1.6.2-GCC-4.7.2.eb') |
|
Going in, thanks @ocaisa! |
Modify the robot behaviour so that for dependencies it always check the existing easyconfigs first before resorting to using the ones that are generated by
--try-toolchainFixes #1372