added support to 'download' sources from git #2555

mboisson · 2018-08-30T17:54:28Z

This pull requests allows one to specify a dictionary of the following form to "download" sources from a git repository

sources = [{
        'filename':'%(namelower)s-%(version)s.tar.gz',
        'git_config': {
                'tag': 'v%(version)s',
                'repo_name': 'speedseq',
                'url': 'https://github.com/hall-lab',
                'recursive': True,
                }
        }]

houndci-bot · 2018-08-30T17:56:27Z

easybuild/framework/easyblock.py

+                        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))


missing whitespace after ','

houndci-bot · 2018-08-30T17:56:28Z

easybuild/framework/easyblock.py

+                    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")


line too long (121 > 120 characters)

houndci-bot · 2018-08-30T17:56:28Z

easybuild/framework/easyblock.py

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


continuation line over-indented for visual indent

houndci-bot · 2018-08-30T17:56:28Z

easybuild/framework/easyblock.py

@@ -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={}):


line too long (125 > 120 characters)

houndci-bot · 2018-08-30T18:00:45Z

easybuild/framework/easyblock.py

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


continuation line under-indented for visual indent

houndci-bot · 2018-08-30T18:00:45Z

easybuild/framework/easyblock.py

@@ -559,7 +560,8 @@ 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={}):


continuation line under-indented for visual indent

houndci-bot · 2018-08-30T18:00:46Z

easybuild/framework/easyblock.py

@@ -559,7 +560,8 @@ 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, 


trailing whitespace

houndci-bot · 2018-08-30T18:04:33Z

easybuild/framework/easyblock.py

@@ -559,7 +560,8 @@ 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={}):


continuation line over-indented for visual indent

boegel · 2018-08-31T19:37:19Z

easybuild/framework/easyblock.py

+                        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


@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...

Just this or the whole logic to download from git ?

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...)

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.

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)

boegel · 2018-08-31T19:38:27Z

easybuild/framework/easyblock.py

@@ -559,7 +561,8 @@ 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={}):


@mboisson Using a mutable value like {} as default value is evil (see https://docs.python-guide.org/writing/gotchas/#mutable-default-arguments).

You have a check if gitconfig, so just use None as default value?

boegel · 2018-08-31T19:38:46Z

easybuild/framework/easyblock.py

+                if not isinstance(git_config, dict):
+                    raise EasyBuildError("Found a non null git_config in 'sources', but value is not a dictionary")
+                else:
+                    git_config = git_config.copy()


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

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

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). ;)

boegel · 2018-08-31T19:39:19Z

easybuild/framework/easyblock.py

+                # 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:


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

houndci-bot · 2018-08-31T20:34:12Z

easybuild/tools/filetools.py

+        (cmdstdouterr, ec) = run.run_cmd(cmd, log_all=True, log_ok=False, simple=False, regexp=False)
+        change_dir(targetdir)
+
+    #create an archive and delete the git repo


block comment should start with '# '

houndci-bot · 2018-08-31T20:34:12Z

easybuild/tools/filetools.py

+        cmd = "git clone %s %s/%s.git" % (recursive, url, repo_name)
+    (cmdstdouterr, ec) = run.run_cmd(cmd, log_all=True, log_ok=False, simple=False, regexp=False)
+
+    #if a specific commit is asked for, check it out


block comment should start with '# '

houndci-bot · 2018-08-31T20:34:12Z

easybuild/tools/filetools.py

@@ -1627,6 +1628,63 @@ def copy(paths, target_path, force_in_dry_run=False):
        else:
            raise EasyBuildError("Specified path to copy is not an existing file or directory: %s", path)

+def get_source_from_git(filename, targetdir, git_config):


expected 2 blank lines, found 1

boegel

@mboisson We'll also need a dedicated test for get_source_from_git before this can be merged...

easybuild/framework/easyblock.py

easybuild/tools/filetools.py

boegel · 2018-09-01T07:41:19Z

easybuild/tools/filetools.py

+        change_dir(targetdir)
+
+    # create an archive and delete the git repo
+    cmd = "tar cfvz %s --exclude-vcs %s && rm -rf %s" % (targetpath, repo_name, repo_name)


@mboisson Please use the remove_file function rather than rm -rf... :)

fixed in upcoming commit

remove_file does not remove a directory recursively, unless I am missing something ?

You're right, we need a remove_dir as well...

easybuild/tools/filetools.py

"define"

mboisson · 2018-09-05T16:28:14Z

Regarding writing a test for the new function, I am quite puzzled how to test this. I guess I could try to download the EasyBuild repo, you are not using submodules. I probably should not test with a real application repository either.

Ideas about how/where to start ?

…emove function + add tests

houndci-bot · 2018-09-19T21:00:10Z

test/framework/filetools.py

+        run_check()
+
+
+    def test_is_sha256_checksum(self):


too many blank lines (2)

minor cleanup in get_source_tarball_from_git + add & use remove_dir/remove function + add tests

mboisson · 2018-09-19T21:28:03Z

I merged your PR @boegel even though I don't like merging develop, I don't have enough time to figure out this ninja-level git... I'll just close and remove this branch as soon as it's merged into upstream develop and not merge it with our master branch.

boegel · 2018-09-20T09:05:32Z

@mboisson tests fail because of problem in the Travis config which is being fixed in #2584, but also because of using a git URL for the testrepository in the tests, which is fixed in ComputeCanada#6

use HTTPS url for test repo in tests to avoid cloning failures in Travis

boegel · 2018-09-20T14:35:57Z

Going in, right in time to be included in EasyBuild v3.7.0, thanks a lot @mboisson!

boegel · 2018-09-20T14:36:32Z

Going in, thanks @mboisson!

added support to 'download' sources from git

0f9ce62

houndci-bot reviewed Aug 30, 2018

View reviewed changes

cosmetic changes

c657745

houndci-bot reviewed Aug 30, 2018

View reviewed changes

trying to appease the hound

372b3c6

houndci-bot reviewed Aug 30, 2018

View reviewed changes

mboisson added 3 commits August 30, 2018 18:05

is the hound happy yet ?

7baa951

Added my name in the list of authors... and hound: good boy

75ef62c

fixed reference before initialization for git_config

5abd3b7

boegel added the enhancement label Aug 31, 2018

boegel requested changes Aug 31, 2018

View reviewed changes

boegel added this to the next release milestone Aug 31, 2018

mboisson added 3 commits August 31, 2018 20:26

moved code to get_source_from_git in filetools

ed59ede

fixed run_cmd => run.run_cmd when within filetools

4ce646f

fixed missing return

8ff3559

houndci-bot reviewed Aug 31, 2018

View reviewed changes

appeasing hound

70c17b3

boegel requested changes Sep 1, 2018

View reviewed changes

akesandgren mentioned this pull request Sep 4, 2018

add support for specifying alternative download methods #112

Closed

mboisson added 2 commits September 5, 2018 16:14

Cosmetic changes.

b8d9a4e

Switched back to rm -rf instead of remove_file. Changed parametrize to

e2f07a0

"define"

boegel added 2 commits September 19, 2018 19:48

keep imports sorted in easyblock.py

ed74a75

minor cleanup in get_source_tarball_from_git + add & use remove_dir/r…

8dbc954

…emove function + add tests

boegel mentioned this pull request Sep 19, 2018

minor cleanup in get_source_tarball_from_git + add & use remove_dir/remove function + add tests ComputeCanada/easybuild-framework#5

Merged

boegel and others added 4 commits September 19, 2018 21:59

fix merge conflicts with develop

51a4db3

minor cleanup in get_source_tarball_from_git + add & use remove_dir/r…

e205640

…emove function + add tests

fixing merge conflict

b1eec8b

cherry-picked commit to resolve merge conflict

fa0389b

houndci-bot reviewed Sep 19, 2018

View reviewed changes

test/framework/filetools.py Outdated

run_check()

def test_is_sha256_checksum(self):

Copy link

houndci-bot Sep 19, 2018

Choose a reason for hiding this comment

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

too many blank lines (2)

mboisson added 3 commits September 19, 2018 21:02

fixed one too many blank line

6af5a0c

Merge branch 'sources_from_git' into sources_from_git

9c578cd

Merge pull request #5 from boegel/sources_from_git

e3ab8df

minor cleanup in get_source_tarball_from_git + add & use remove_dir/remove function + add tests

use HTTPS url for test repo in tests to avoid cloning failures in Travis

55f03be

Merge pull request #6 from boegel/sources_from_git

a0ed138

use HTTPS url for test repo in tests to avoid cloning failures in Travis

boegel modified the milestones: next release, 3.7.0 Sep 20, 2018

boegel approved these changes Sep 20, 2018

View reviewed changes

boegel merged commit 224f29f into easybuilders:develop Sep 20, 2018

mboisson deleted the sources_from_git branch September 20, 2018 14:38

boegel mentioned this pull request Sep 20, 2018

update DOLFIN easyblock for latest version (2018.1) easybuilders/easybuild-easyblocks#1521

Merged

mboisson mentioned this pull request Sep 20, 2018

download sources via git for XGBoost 20171120 easyconfig due to use of submodules easybuilders/easybuild-easyconfigs#6880

Merged

added support to 'download' sources from git #2555

added support to 'download' sources from git #2555

Conversation

mboisson commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boegel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mboisson commented Sep 5, 2018

Choose a reason for hiding this comment

mboisson commented Sep 19, 2018

boegel commented Sep 20, 2018

boegel commented Sep 20, 2018

boegel commented Sep 20, 2018