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-* (REVIEW) #1301

Merged
merged 13 commits into from
Jul 8, 2015
Merged

Conversation

boegel
Copy link
Member

@boegel boegel commented Jun 18, 2015

Add support to easily include additional custom easyblocks, module naming schemes and toolchains/toolchain components, without having to fiddle with $PYTHONPATH or Python packaging magic (__init__.py), etc.

TODO list:

  • --include-easyblocks
  • --include-module-naming-schemes
  • --include-toolchains
  • unit tests
    • include_* functions
    • --include-* options combined with --list-toolchains, --list-easyblocks, --avail-module-naming-schemes
  • updated documentation (see document --include (REVIEW) easybuild#129)

note: requires easybuilders/easybuild-easyblocks#617 in order to work correctly

@hpcugentbot
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1785/
EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1785/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member Author

boegel commented Jun 18, 2015

Currently only the --include-easyblocks part works:

$ eb --list-easyblocks | grep foo

$ eb --include-easyblocks=$PWD/foo.py,$PWD/generic/foobar.py --list-easyblocks | grep foo
|-- EB_foo
|-- foobar

If also enhanced the output of --list-easyblocks=detailed:

$ eb --include-easyblocks=$PWD/foo.py,$PWD/generic/foobar.py --list-easyblocks=detailed | grep foo
|-- EB_foo (easybuild.easyblocks.foo @ /var/folders/8s/_frgh9sj6m744mxt5w5lyztr0000gn/T/eb-yFeTNf/included-easyblocks/easybuild/easyblocks/foo.py)
|-- foobar (easybuild.easyblocks.generic.foobar @ /var/folders/8s/_frgh9sj6m744mxt5w5lyztr0000gn/T/eb-yFeTNf/included-easyblocks/easybuild/easyblocks/generic/foobar.py)

@@ -163,9 +163,6 @@ def main(testing_data=(None, None, None)):
new_umask = int(options.umask, 8)
old_umask = os.umask(new_umask)

# set temporary directory to use
eb_tmpdir = set_tmpdir(options.tmpdir)
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Member Author

Choose a reason for hiding this comment

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

I need a temporary directory to symlink the included easyblocks into the correct namespace.

When --include-easyblocks is combined with --list-easyblocks, this point is never reached (--list-easyblocks is handled via tools/options.py, and so we don't return back to main.py after parsing options).

I now set the temporary directory as a part of postprocessing the parsed options, so earlier than I was here. Should be fine (can't hurt to do it earlier).

@boegel
Copy link
Member Author

boegel commented Jun 18, 2015

When combined with easybuilders/easybuild-easyblocks#617, this also reflects easyblocks being overridden:

$ eb --include-easyblocks=$PWD/foo.py,$PWD/gcc.py,$PWD/generic/\*.py --list-easyblocks=detailed | grep -C 2 GCC
|   |-- EB_g2clib (easybuild.easyblocks.g2clib @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/g2clib.py)
|   |-- EB_g2lib (easybuild.easyblocks.g2lib @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/g2lib.py)
|   |-- EB_GCC (easybuild.easyblocks.gcc @ /var/folders/8s/_frgh9sj6m744mxt5w5lyztr0000gn/T/eb-5Zqzj1/included-easyblocks/easybuild/easyblocks/gcc.py)
|   |-- EB_GHC (easybuild.easyblocks.ghc @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/ghc.py)
|   |-- EB_Go (easybuild.easyblocks.go @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/go.py)


$ eb --list-easyblocks=detailed | grep -C 2 GCC
|   |-- EB_g2clib (easybuild.easyblocks.g2clib @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/g2clib.py)
|   |-- EB_g2lib (easybuild.easyblocks.g2lib @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/g2lib.py)
|   |-- EB_GCC (easybuild.easyblocks.gcc @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/gcc.py)
|   |-- EB_GHC (easybuild.easyblocks.ghc @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/ghc.py)
|   |-- EB_Go (easybuild.easyblocks.go @ /Users/kehoste/work/easybuild-easyblocks/easybuild/easyblocks/g/go.py)

__path__ = extend_path(__path__, __name__)
"""


Copy link
Collaborator

Choose a reason for hiding this comment

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

any double space to trim above, all is intentional?

Copy link
Member Author

Choose a reason for hiding this comment

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

we separate constants from functions (and top-level functions between each other) with a double newline

@fgeorgatos
Copy link
Collaborator

first visual review ok. very interesting PR, I am sure it will get quickly popular.

I hope the lack of --include-module-naming-schemes, --include-toolchains will not delay merging it in before 2.2.0 (if so, just turn the latter two into experimental facilities).

@boegel
Copy link
Member Author

boegel commented Jun 18, 2015

@fgeorgatos: the idea is to get some early feedback, finishing the other two options shouldn't be that hard, with what's in place already (it pretty analogous)

@boegel
Copy link
Member Author

boegel commented Jun 20, 2015

implementation finished, first stab at dedicated unit tests for include_* functions included

should also add tests for --include-* options combined with --list-toolchains, --list-easyblocks, --avail-module-naming-schemes

@hpcugentbot
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1788/
EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1788/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1789/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1789/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

handle = open(init_path, 'w')
handle.write(PKG_INIT_BODY)
handle.close()
except (IOError, OSError) as err:
Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine,
but it's sometimes easier to have
with open(newfile, 'w') as handle:

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I keep forgetting we're no longer stuck to Python 2.4 ;)

@hpcugentbot
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1800/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1800/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member Author

boegel commented Jun 29, 2015

@JensTimmerman: please rereview

@boegel boegel changed the title add support for --include-* (WIP) add support for --include-* (REVIEW) Jun 29, 2015
@JensTimmerman
Copy link
Contributor

ok

@hpcugentbot
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1838/
EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1838/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1842/
EasyBuild framework unit test suite FAILed.

See https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1842/console for more details.

Please fix the reported issues by pushing additional commits to the branch corresponding with this pull request; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1847/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1847/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@hpcugentbot
Copy link
Contributor

Refer to this link for build results (access rights to CI server needed):
https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1848/
EasyBuild framework unit test suite PASSed (see https://jenkins1.ugent.be/job/easybuild-framework-pr-builder/1848/console for more details).

This pull request is now ready for review/testing.

Please try and find someone who can tackle this; contact @boegel if you're not sure what to do.

@boegel
Copy link
Member Author

boegel commented Jul 8, 2015

OK, I'm calling this, merging to be included in EB v2.2.0.

All TODO's are ticked off, extensive unit tests are in place, and a documentation update is ready to go as well (see easybuilders/easybuild#129).

Thanks for the feedback on this @fgeorgatos, @JensTimmerman, @wpoely86 and @rjeschmi!

boegel added a commit that referenced this pull request Jul 8, 2015
add support for --include-* (REVIEW)
@boegel boegel merged commit 49c0f3c into easybuilders:develop Jul 8, 2015
@boegel boegel deleted the include branch July 8, 2015 07:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants