-
Notifications
You must be signed in to change notification settings - Fork 313
BinariesTarball #721
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
BinariesTarball #721
Changes from 2 commits
3d3eba1
0ce1c19
6f9be2e
f515103
7807b62
31210d8
074dd79
203513e
ae57a98
53cedde
6776f14
ca82164
ac74b85
fd107b0
6186b6d
46cde23
4e4326f
4fcb4e3
2eed6ea
2d54ce3
1c1d60f
da06bea
1514900
a9169d4
355dbcd
c83a126
97985d2
05c7ed1
f2465f4
ee7945a
2c86136
268e622
c10c518
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,7 @@ | |
| import stat | ||
|
|
||
| from easybuild.easyblocks.generic.tarball import Tarball | ||
| from easybuild.framework.easyconfig import CUSTOM | ||
| from easybuild.tools.build_log import EasyBuildError | ||
| from easybuild.tools.filetools import adjust_permissions | ||
|
|
||
|
|
@@ -40,21 +41,43 @@ class BinariesTarball(Tarball): | |
| """ | ||
| Support for installing a tarball of binaries | ||
| """ | ||
| @staticmethod | ||
| def extra_options(extra_vars=None): | ||
| """ | ||
| Define list of files to be copied during the installation process | ||
| """ | ||
| extra_vars = Tarball.extra_options(extra_vars) | ||
| # Allow specification of files to copy explicitly. | ||
| # If files_to_copy is a zero-length list, all files (but not | ||
| # directories, etc.) in start_dir will be copied with their | ||
| # current names. | ||
| extra_vars['files_to_copy'] = [ | ||
| [], | ||
| "List of optional (source_file, destination_file) tuples", | ||
| CUSTOM, | ||
| ] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. keep this on a single line, since it's really one entry (not three) |
||
| return extra_vars | ||
|
|
||
| def install_step(self): | ||
| """Install by copying unzipped binaries to 'bin' subdir of installation dir, and fixing permissions.""" | ||
|
|
||
| bindir = os.path.join(self.installdir, 'bin') | ||
| items_to_copy = [] | ||
| try: | ||
| os.makedirs(bindir) | ||
| for item in os.listdir(self.cfg['start_dir']): | ||
| if len(self.cfg['files_to_copy']) == 0: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if self.cfg['files_to_copy'] is None:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Only if initialising the value to None (see above) is considered more desirable.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using See also http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-arguments |
||
| for item in os.listdir(self.cfg['start_dir']): | ||
| items_to_copy.append((os.path.join(self.cfg['start_dir'], item), item)) | ||
| else: | ||
| items_to_copy = self.cfg['files_to_copy'] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this implies that the entries should be specified full path, like it would make more sense to also join these entries with
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, as far as I can tell, if |
||
| for (item, destname) in items_to_copy: | ||
| if os.path.isfile(item): | ||
| shutil.copy2(os.path.join(self.cfg['start_dir'], item), bindir) | ||
| shutil.copy2(item, os.path.join(bindir, destname)) | ||
| # make sure binary has executable permissions | ||
| adjust_permissions(os.path.join(bindir, item), stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH, add=True) | ||
| adjust_permissions(os.path.join(bindir, destname), stat.S_IXUSR|stat.S_IXGRP|stat.S_IXOTH, add=True) | ||
| self.log.debug("Copied %s to %s and fixed permissions" % (item, bindir)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
| else: | ||
| self.log.warning("Skipping non-file %s in %s, not copying it." % (item, self.cfg['start_dir'])) | ||
| self.log.warning("%s: not a file. Skipping." % item) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
| except OSError, err: | ||
| raise EasyBuildError("Copying binaries in %s to install dir 'bin' failed: %s", self.cfg['start_dir'], err) | ||
| raise EasyBuildError("Copying binaries to install dir 'bin' failed: %s" % err) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. use |
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use
Noneas default value?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Precedents elsewhere seem to be in favour of an empty list (see e.g. the
makesymlinksextra variable inrpm.py).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boegel: Further thoughts regarding
Noneas the default?