experimental support for --continue (WIP) #2385
Conversation
| app.cfg['stop'] = stop | ||
|
|
||
| continue_from = build_option('continue') | ||
| if continue_from is not None: |
There was a problem hiding this comment.
if continue_from: should be enough and is easier to read.
| if continue_from is not None: | ||
| _log.experimental("Continuing from step %s" % continue_from) | ||
| skipsteps = [] | ||
| for step in build_option('valid_stops'): |
There was a problem hiding this comment.
I haven't checked, but is build_option('valid_stops') guaranteed to be in the correct order?
There was a problem hiding this comment.
Yes, it will be in the correct execution order. The list in valid_stops is in the same order as get_steps.
Maybe it's worth the trouble to add an explicit test for this if we really on that, doesn't hurt...
|
|
||
| self.log.info("Using %s as start dir", self.cfg['start_dir']) | ||
|
|
||
| change_dir(self.start_dir) |
There was a problem hiding this comment.
@migueldiascosta Hmm, why move this? You only end up duplicating the change_dir because of it? Or do you need to be in the start dir first before running find_base_dir?
|
|
||
| if build_option('continue') and 'source' in self.cfg['skipsteps']: | ||
| self.cfg['start_dir'] = find_base_dir() | ||
| change_dir(self.cfg['start_dir']) |
There was a problem hiding this comment.
@migueldiascosta Please use the self.start_dir shorthand to get the value of start_dir (maybe we also need a self.set_start_dir?)
| cleanup_builddir is False, otherwise we remove the installation | ||
| """ | ||
| if not self.build_in_installdir and build_option('cleanup_builddir'): | ||
| if not self.build_in_installdir and build_option('cleanup_builddir') and not build_option('continue'): |
There was a problem hiding this comment.
@migueldiascosta Can you clarify why we shouldn't be cleaning up when --continue is used?
Cleaning up is only done at the very end, when all preceding steps have succeeded.
There was a problem hiding this comment.
well, if I properly interpret the intentions of my past self, the idea of --continue was in the context of a debug workflow, so it is assumed that one may need to continue to use --continue (pun intended)
once final, the changes should always be put in a patch and everything built from scratch.
but I have to say that my users have simply been debugging using buildenv or --dump-env-script on top of a manually prepared build dir, it's not exactly the same as this would allow but it fits most cases, so not sure if this is worth the trouble...
| raise EasyBuildError("Cleaning up builddir %s failed: %s", self.builddir, err) | ||
|
|
||
| if not build_option('cleanup_builddir'): | ||
| if not build_option('cleanup_builddir') or build_option('continue'): |
There was a problem hiding this comment.
Same here, why (claim to) keep the build dir under --continue?
| self.log.debug("Setting cleanupoldbuild to False in continue mode") | ||
| self.cfg['cleanupoldbuild'] = False | ||
|
|
||
| # make sure build dir is unique if cleanupoldbuild is False or not set |
There was a problem hiding this comment.
update comment, also mention or if --continue is used (or just drop the comment, maybe...)
| opts = OrderedDict({ | ||
| 'dry-run': ("Print build overview incl. dependencies (full paths)", None, 'store_true', False), | ||
| 'dry-run-short': ("Print build overview incl. dependencies (short paths)", None, 'store_true', False, 'D'), | ||
| 'continue': ("Continue the installation from certain step", |
| 'dry-run': ("Print build overview incl. dependencies (full paths)", None, 'store_true', False), | ||
| 'dry-run-short': ("Print build overview incl. dependencies (short paths)", None, 'store_true', False, 'D'), | ||
| 'continue': ("Continue the installation from certain step", | ||
| 'choice', 'store_or_None', SOURCE_STEP, 'c', all_stops), |
There was a problem hiding this comment.
Not sure if we want eb -c to be equivalent with eb --continue, maybe we should save up -c for something else?
It seems to me that --continue will always be a bit 'risky' (it may fail for all kinds of reasons that are going to be hard to fix)...
| skipsteps.append(step) | ||
| else: | ||
| break | ||
| app.cfg['skipsteps'] = skipsteps |
There was a problem hiding this comment.
This may result in the skipsteps being added to the saved easyconfig file, which doesn't seem like a good idea to me...
Maybe introduce a self.steps_to_skip, which can be used both for skipsteps and --continue?
| if continue_from is not None: | ||
| _log.experimental("Continuing from step %s" % continue_from) | ||
| skipsteps = [] | ||
| for step in build_option('valid_stops'): |
There was a problem hiding this comment.
Yes, it will be in the correct execution order. The list in valid_stops is in the same order as get_steps.
Maybe it's worth the trouble to add an explicit test for this if we really on that, doesn't hurt...
migueldiascosta
changed the title
|
While it's nice to be able to start from a particular step, I don't see that the patch validates the previous steps are completed successfully. Gentoo's Portage systems creates files like .unpacked, .configured, etc to mark that a step has completed correctly. In this case, you could even just specify --continue to have it try again from wherever it left off, although I do like the idea of having an optional argument in case you want to back up a step (like reapply a patch). |
4 participants
in case anyone else is interested
my use case is for users to be able to debug/devel long compilations without rebuilding everything from scratch (often they are changing only one file in a large package)
they would run e.g.
eb --disable-cleanup-builddir, change something in the build dir and then runeb --continue=build --experimental, eventually multiple timesthere may be other use cases