Skip to content
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

added support to 'download' sources from git #2555

Merged
merged 23 commits into from
Sep 20, 2018
Merged
Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
0f9ce62
added support to 'download' sources from git
mboisson Aug 30, 2018
c657745
cosmetic changes
mboisson Aug 30, 2018
372b3c6
trying to appease the hound
mboisson Aug 30, 2018
7baa951
is the hound happy yet ?
mboisson Aug 30, 2018
75ef62c
Added my name in the list of authors... and hound: good boy
mboisson Aug 30, 2018
5abd3b7
fixed reference before initialization for git_config
mboisson Aug 30, 2018
ed59ede
moved code to get_source_from_git in filetools
mboisson Aug 31, 2018
4ce646f
fixed run_cmd => run.run_cmd when within filetools
mboisson Aug 31, 2018
8ff3559
fixed missing return
mboisson Aug 31, 2018
70c17b3
appeasing hound
mboisson Aug 31, 2018
b8d9a4e
Cosmetic changes.
mboisson Sep 5, 2018
e2f07a0
Switched back to rm -rf instead of remove_file. Changed parametrize to
mboisson Sep 5, 2018
ed74a75
keep imports sorted in easyblock.py
boegel Sep 19, 2018
8dbc954
minor cleanup in get_source_tarball_from_git + add & use remove_dir/r…
boegel Sep 19, 2018
51a4db3
fix merge conflicts with develop
boegel Sep 19, 2018
e205640
minor cleanup in get_source_tarball_from_git + add & use remove_dir/r…
boegel Sep 19, 2018
b1eec8b
fixing merge conflict
mboisson Sep 19, 2018
fa0389b
cherry-picked commit to resolve merge conflict
boegel Aug 31, 2018
6af5a0c
fixed one too many blank line
mboisson Sep 19, 2018
9c578cd
Merge branch 'sources_from_git' into sources_from_git
mboisson Sep 19, 2018
e3ab8df
Merge pull request #5 from boegel/sources_from_git
mboisson Sep 19, 2018
55f03be
use HTTPS url for test repo in tests to avoid cloning failures in Travis
boegel Sep 20, 2018
a0ed138
Merge pull request #6 from boegel/sources_from_git
mboisson Sep 20, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 50 additions & 2 deletions easybuild/framework/easyblock.py
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,7 @@ def fetch_sources(self, sources=None, checksums=None):
extract_cmd = source.pop('extract_cmd', None)
download_filename = source.pop('download_filename', None)
source_urls = source.pop('source_urls', None)
git_config = source.pop('git_config', {})
if source:
raise EasyBuildError("Found one or more unexpected keys in 'sources' specification: %s", source)

Expand All @@ -361,7 +362,7 @@ def fetch_sources(self, sources=None, checksums=None):
# check if the sources can be located
force_download = build_option('force_download') in [FORCE_DOWNLOAD_ALL, FORCE_DOWNLOAD_SOURCES]
path = self.obtain_file(filename, download_filename=download_filename, force_download=force_download,
urls=source_urls)
urls=source_urls, git_config=git_config)
if path:
self.log.debug('File %s found for source %s' % (path, filename))
self.src.append({
Expand Down Expand Up @@ -559,7 +560,7 @@ def fetch_extension_sources(self, skip_checksums=False):

return exts_sources

def obtain_file(self, filename, extension=False, urls=None, download_filename=None, force_download=False):
def obtain_file(self, filename, extension=False, urls=None, download_filename=None, force_download=False, git_config={}):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (125 > 120 characters)

"""
Locate the file with the given name
- searches in different subdirectories of source path
Expand All @@ -568,6 +569,7 @@ def obtain_file(self, filename, extension=False, urls=None, download_filename=No
:param extension: indicates whether locations for extension sources should also be considered
:param urls: list of source URLs where this file may be available
:param download_filename: filename with which the file should be downloaded, and then renamed to <filename>
:param git_config: dictionary to parametrize how to download the repository
mboisson marked this conversation as resolved.
Show resolved Hide resolved
mboisson marked this conversation as resolved.
Show resolved Hide resolved
:force_download: always try to download file, even if it's already available in source path
"""
srcpaths = source_paths()
Expand Down Expand Up @@ -663,6 +665,52 @@ def obtain_file(self, filename, extension=False, urls=None, download_filename=No
if self.dry_run:
self.dry_run_msg(" * %s found at %s", filename, foundfile)
return foundfile
elif git_config:
# if a non-empty dictionary was provided, download from git and archive
if not isinstance(git_config, dict):
raise EasyBuildError("Found a non null git_config in 'sources', but value is not a dictionary")
else:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for the else, the raise makes sure you'll never reach the below...

git_config = git_config.copy()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the copy()? I don't see the point of that...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took this strategy from similar code above (line 347).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, now it clicks... It's done to avoid modifying the value that is passed into obtain_file with the .pops...

Better add a comment to explain that (on line 347 too, while you're at it). ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

tag = git_config.pop('tag', None)
url = git_config.pop('url', None)
repo_name = git_config.pop('repo_name', None)
commit = git_config.pop('commit', None)
recursive = git_config.pop('recursive', False)
if git_config:
raise EasyBuildError("Found one or more unexpected keys in 'git_config' specification: %s",
git_config)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

continuation line over-indented for visual indent

if not repo_name:
raise EasyBuildError("git_config specified, but no repo_name specified")
if not tag and not commit:
raise EasyBuildError("git_config specified, but neither tag nor commit specified")
if tag and commit:
raise EasyBuildError("git_config specified, and both tag and commit specified. Specify only one")

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

line too long (121 > 120 characters)

if not url:
raise EasyBuildError("git_config specified, but no url specified")
if '.tar.gz' not in filename:
raise EasyBuildError("git_config only supports filename ending in .tar.gz")

targetdir = os.path.join(srcpaths[0], self.name.lower()[0], self.name)
mkdir(targetdir, parents=True)
targetpath = os.path.join(targetdir, filename)

change_dir(targetdir)
recursive = " --recursive " if recursive else ""
if tag:
cmd = "git clone --branch %s %s %s/%s.git " % (tag, recursive, url, repo_name)
else:
cmd = "git clone %s %s/%s.git" % (recursive, url, repo_name)
(cmdstdouterr, ec) = run_cmd(cmd, log_all=True, log_ok=False, simple=False, regexp=False)
if commit:
change_dir(os.path.join(targetdir,repo_name))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing whitespace after ','

recursive = " && git submodule update " if recursive else ""
cmd = "git checkout %s %s " % (commit, recursive)
(cmdstdouterr, ec) = run_cmd(cmd, log_all=True, log_ok=False, simple=False, regexp=False)
change_dir(targetdir)
cmd = "tar cfvz %s --exclude-vcs %s && rm -rf %s" % (targetpath, repo_name, repo_name)
(cmdstdouterr, ec) = run_cmd(cmd, log_all=True, log_ok=False, simple=False, regexp=False)
return targetpath
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mboisson Quick review: please move this to a dedicated function, the obtain_file method is already too big as is... Throwing that in filetools probably makes most sense? And then we also should have a dedicated test for it...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just this or the whole logic to download from git ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would create a function that takes git_config as an argument, and trim this down to something like:

elif git_config:
    return new_function(git_config)

(but find a better name than new_function, of course...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Moving it to filetools would require quite a few more parameters, because this bit of code uses a couple of variables internal to easyblock.py :

  • srcpaths[0]
  • self.name
    it also calls change_dir, I'll try to see how I can rework those.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but that's only 2 parameters to add, that's OK.

change_dir returns the original dir you were in, you could make sure you restore where you were in the function:

cwd = change_dir(new_dir)
...
change_dir(cwd)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


else:
# try and download source files from specified source URLs
if urls:
Expand Down