Allowing "download install" for petsc#1114
Conversation
…ory before the configure, and keep it after
| if LooseVersion(self.version) >= LooseVersion("3"): | ||
| # make the install dir first if we are doing a download install, then keep it for the rest of the way | ||
| if self.cfg["downloadinstall"]: | ||
| self.log.info("Creating the installation directory before the configure step because we are doing a download install.") |
There was a problem hiding this comment.
style nitpicking: this line is too long, should be <= 120 characters...
There was a problem hiding this comment.
The new comment line is also shorter.
|
@mboisson maybe the name of the new easyconfig parameter should be more directly related to what is actually happening, e.g. Either that, or enabling |
|
Well, it also prevents the install dir from being removed after the configure step, so I'm not sure the new name is better. Adding all the "--download-*" configuration option is not possible. There are ~300 of those, most of them not frequently used by end users. |
|
@mboisson the fact that the installdir isn't being removed is implied, without that there's little point in creating it early... I just find it confusing that the |
|
Would it be less confusing if there was a second parameter called |
|
@mboisson that makes sense, and then you don't need to have |
|
Allright. I'll modify the PR for that. |
|
I ended up adding 3 options |
| for p in self.cfg["download_packages"]: | ||
| self.cfg.update('configopts', '--with-download-%s=1' % p) | ||
| if self.cfg["download_packages_static"]: | ||
| for p in self.cfg["download_packages_static"]: |
There was a problem hiding this comment.
@mboisson maybe make the default value an empty list [], which avoids the extra if check?
| self.cfg.update('configopts', '--with-download-%s=1' % p) | ||
| self.cfg.update('configopts', '--with-download-%s-shared=0' % p) | ||
| if self.cfg["download_packages_shared"]: | ||
| for p in self.cfg["download_packages_shared"]: |
There was a problem hiding this comment.
no single-letter variables outside of list comprehensions please
There was a problem hiding this comment.
changed it for "dep" (and download_deps_* for the lists)
| self.cfg.update('configopts', '--with-download-%s-shared=0' % p) | ||
| if self.cfg["download_packages_shared"]: | ||
| for p in self.cfg["download_packages_shared"]: | ||
| self.cfg.update('configopts', '--with-download-%s=1' % p) |
There was a problem hiding this comment.
quite a bit of duplicate code here, so how about this (I also renamed to download_deps*, which is shorter and avoids confusion with 'packages' like extra Python packages/extensions, etc...)
for dep in self.cfg['download_deps'] + self.cfg['download_deps_shared'] + self.cfg['download_deps_static']:
self.cfg.update('configopts', '--with-download-%s=1' % dep)
for dep in self.cfg['download_packages_shared']:
self.cfg.update('configopts', '--with-download-%s-shared=1' % dep)
for dep in self.cfg['download_packages_static']:
self.cfg.update('configopts', '--with-download-%s-shared=0' % dep)There was a problem hiding this comment.
Done. I also added a "set" directive to make sure that if dependencies are listed multiple times, they are not included in the configure option multiple times.
| 'runtest': ['test', "Make target to test build", BUILD], | ||
| 'download_packages_static': [None, "Lists the packages that should be downloaded and installed without shared objects", CUSTOM], | ||
| 'download_packages_shared': [None, "Lists the packages that should be downloaded and installed with shared objects", CUSTOM], | ||
| 'download_packages': [None, "Lists the packages that should be downloaded and installed without specifying static or shared (used the default for the package)", CUSTOM], |
There was a problem hiding this comment.
keep the max. line length of 120 characters in mind please
There was a problem hiding this comment.
Shortened the lines
| self.cfg["keeppreviousinstall"] = True | ||
| deps = set( self.cfg["download_deps"] | ||
| + self.cfg["download_deps_static"] | ||
| + self.cfg["download_deps_shared"] ) |
There was a problem hiding this comment.
the wrapping makes this a bit messy, and you need to full list of deps in two places, so how about:
if LooseVersion(self.version) >= LooseVersion("3"):
deps = self.cfg["download_deps"] + self.cfg["download_deps_static"] + self.cfg["download_deps_shared"]
if deps:
# make the install dir first if we are downloading dependencies, then keep it for the rest of the way
...
for dep in set(deps):
...|
I believe all the changes requested have been done. |
|
lgtm, doesn't affect existing PETSc easyconfigs, and already tested in the wild (cfr. https://lists.ugent.be/wws/arc/easybuild/2017-03/msg00061.html) so, going in, thanks @mboisson! |
When compiling petsc with "--download-" configure options, these are installed in the installation directory in the configure step.
The installation directory should therefore be created (and purged if there was an existing one) BEFORE the configure step. It should also be kept after the configure step (rather than cleaned up before the install step).
This patch enables this when "downloadinstall" is set in the easy config.