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

add support for --include-easyblocks-from-pr (REVIEW) #3206

Merged

Conversation

migueldiascosta
Copy link
Member

No description provided.

easybuild/main.py Outdated Show resolved Hide resolved
@@ -199,6 +200,10 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
eb_go, cfg_settings = set_up_configuration(args=args, logfile=logfile, testing=testing)
options, orig_paths = eb_go.options, eb_go.args

if options.include_easyblocks_from_pr:
included_easyblocks = fetch_easyblocks_from_pr(options.include_easyblocks_from_pr)
include_easyblocks(options.tmpdir, included_easyblocks)
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure if this is the correct tmpdir

Copy link
Member

Choose a reason for hiding this comment

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

You better create a subdirectory in self.tmpdir, to avoid that --include-easyblocks and --include-easyblocks-from-pr step on each others toes if both options are used at the same time.

Or we tweak the include_easyblocks function to use tempfile.mkdtemp to create a subdirectory in the path that it is given, that's probably a better options in the long run:

easyblocks_path = tempfile.mkdtemp(prefix=tmpdir, os.path.join(dir=tmpdir, prefix='included-easyblocks-')

Ideally, we also have this use case covered of using both --include-easyblocks* options in the tests...

Copy link
Member Author

Choose a reason for hiding this comment

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

hm, when looking into adding a test for the use of both --include-easyblocks* options, I noticed that --list-easyblocks doesn't take into account the easyblocks from a PR, because it's done in the options.postprocess step, before set_up_configuration...

test/framework/github.py Outdated Show resolved Hide resolved
test/framework/github.py Show resolved Hide resolved
test/framework/github.py Show resolved Hide resolved
@migueldiascosta migueldiascosta added this to the next release (4.1.2?) milestone Mar 1, 2020
@migueldiascosta migueldiascosta changed the title add support for --include-easyblocks-from-pr (WIP) add support for --include-easyblocks-from-pr (REVIEW) Mar 1, 2020
@@ -199,6 +200,10 @@ def main(args=None, logfile=None, do_build=None, testing=False, modtool=None):
eb_go, cfg_settings = set_up_configuration(args=args, logfile=logfile, testing=testing)
options, orig_paths = eb_go.options, eb_go.args

if options.include_easyblocks_from_pr:
included_easyblocks = fetch_easyblocks_from_pr(options.include_easyblocks_from_pr)
include_easyblocks(options.tmpdir, included_easyblocks)
Copy link
Member

Choose a reason for hiding this comment

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

You better create a subdirectory in self.tmpdir, to avoid that --include-easyblocks and --include-easyblocks-from-pr step on each others toes if both options are used at the same time.

Or we tweak the include_easyblocks function to use tempfile.mkdtemp to create a subdirectory in the path that it is given, that's probably a better options in the long run:

easyblocks_path = tempfile.mkdtemp(prefix=tmpdir, os.path.join(dir=tmpdir, prefix='included-easyblocks-')

Ideally, we also have this use case covered of using both --include-easyblocks* options in the tests...

easybuild/main.py Outdated Show resolved Hide resolved
easybuild/tools/github.py Outdated Show resolved Hide resolved
Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

fix for small issue implemented in migueldiascosta#12

if github_repo == GITHUB_EASYCONFIGS_REPO:
path = build_option('pr_path')
elif github_repo == GITHUB_EASYBLOCKS_REPO:
path = 'ebs_pr%s' % pr
Copy link
Member

Choose a reason for hiding this comment

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

This should be a subdirectory of the temporary directory set up for this EasyBuild session.

Right now, it's a relative path, which means this directory gets created in the current working directory (and worse, stays there).

use subdir in temp dir for easyblocks downloaded from PR
@boegel boegel merged commit c55ff35 into easybuilders:develop Apr 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants