Skip to content

allow including easyblocks from multiple locations #3311

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.

Sign up for GitHub

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

Merged
boegel merged 9 commits into from
Sep 2, 2020
Merged

allow including easyblocks from multiple locations #3311

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
971ac5d
expand glob paths when checking for multiple inclusion of easyblocks
migueldiascosta Apr 30, 2020
d925a64
sync with develop
migueldiascosta Aug 27, 2020
7a7b8a1
restore sys.modules at the end of verify_imports
migueldiascosta Aug 27, 2020
60f71a3
allow including the same easyblock from a file and from a PR, warn th…
migueldiascosta Aug 27, 2020
150d911
update include_easyblocks tests
migueldiascosta Aug 27, 2020
37abe78
make messages and warnings more explicit
migueldiascosta Sep 2, 2020
92c97dc
mention PR id in warning message when easyblocks are included from mu…
boegel Sep 2, 2020
292b7ba
catch & check output produced by --include-easyblocks-from-pr in test
boegel Sep 2, 2020
efe98ba
Merge pull request #14 from boegel/20200430163846_new_pr_cworcWSdYy
migueldiascosta Sep 2, 2020
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
6 changes: 4 additions & 2 deletions easybuild/tools/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,8 @@
from easybuild.tools.docs import avail_toolchain_opts, avail_easyconfig_params, avail_easyconfig_templates
from easybuild.tools.docs import list_easyblocks, list_toolchains
from easybuild.tools.environment import restore_env, unset_env_vars
from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256, CHECKSUM_TYPES, install_fake_vsc, move_file, which
from easybuild.tools.filetools import CHECKSUM_TYPE_SHA256, CHECKSUM_TYPES, expand_glob_paths, install_fake_vsc
from easybuild.tools.filetools import move_file, which
from easybuild.tools.github import GITHUB_EB_MAIN, GITHUB_EASYCONFIGS_REPO
from easybuild.tools.github import GITHUB_PR_DIRECTION_DESC, GITHUB_PR_ORDER_CREATED, GITHUB_PR_STATE_OPEN
from easybuild.tools.github import GITHUB_PR_STATES, GITHUB_PR_ORDERS, GITHUB_PR_DIRECTIONS
Expand Down Expand Up @@ -1432,7 +1433,8 @@ def set_up_configuration(args=None, logfile=None, testing=False, silent=False):
if eb_go.options.include_easyblocks:
# make sure we're not including the same easyblock twice
included_from_pr = set([os.path.basename(eb) for eb in easyblocks_from_pr])
included_from_file = set([os.path.basename(eb) for eb in eb_go.options.include_easyblocks])
included_paths = expand_glob_paths(eb_go.options.include_easyblocks)
included_from_file = set([os.path.basename(eb) for eb in included_paths])
included_twice = included_from_pr & included_from_file
if included_twice:
raise EasyBuildError("Multiple inclusion of %s, check your --include-easyblocks options",
Copy link
Member

@boegel boegel May 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@migueldiascosta Rather than raising an error, I think we should let the easyblocks that are included from the PR win over any local easyblocks included with --include-easyblocks. That makes perfect sense to me, since I don't see a use case where you would be using --include-easyblocks-from-pr but you would still like to see local easyblocks get precedence?

It's probably also a good idea to print a message for the easyblocks that get pulled in from a PR, to make that a bit more visible?

== easyblock example.py included from PR #1234

Copy link
Member Author

@migueldiascosta migueldiascosta May 2, 2020

Choose a reason for hiding this comment

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

Actually, the easyblocks included from the PR should already take precedence, because they are included later and their path is prepended.

I hadn't realised this before but when the local easyblocks are included, they are immediately imported in verify_imports and they remain in sys.modules. When the same easyblock is included later from a PR, verify_imports fails because it continues to use previously imported module.

Not sure if this would be the best approach, but at least to demonstrate what is happening, if we add sys.modules.pop(pymod_spec) at the end of the loop in verify_imports, then the error reported by @zao goes away, the easyblocks from a PR take precedence, and we can stop raising this error

Expand Down
2 changes: 1 addition & 1 deletion test/framework/options.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2734,7 +2734,7 @@ def test_xxx_include_easyblocks_from_pr(self):

# including the same easyblock twice should fail
args = [
'--include-easyblocks=%s/cmakemake.py' % self.test_prefix,
'--include-easyblocks=%s/*.py' % self.test_prefix,
'--include-easyblocks-from-pr=1915',
'--list-easyblocks=detailed',
'--unittest-file=%s' % self.logfile,
Expand Down