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

Archive easyblocks along with easyconfig #2653

Merged
merged 19 commits into from
Nov 21, 2018

Conversation

ocaisa
Copy link
Member

@ocaisa ocaisa commented Nov 5, 2018

Fixes #2633

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.

@ocaisa A bit of logging wouldn't hurt here imho...

easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
test/framework/toy_build.py Outdated Show resolved Hide resolved
@boegel boegel added this to the 3.8.0 milestone Nov 5, 2018
@easybuilders easybuilders deleted a comment from boegelbot Nov 6, 2018
@ocaisa ocaisa added the bug fix label Nov 6, 2018
@@ -2859,7 +2862,6 @@ def build_and_install_one(ecdict, init_env):
errormsg = "build failed (first %d chars): %s" % (first_n, err.msg[:first_n])
_log.warning(errormsg)
result = False
app.close_log()
Copy link
Member Author

Choose a reason for hiding this comment

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

Logging was being shut down too early, meaning all logging below this was lost

@ocaisa
Copy link
Member Author

ocaisa commented Nov 6, 2018

@boegel Cleaned up and good to go now

easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
easybuild/framework/easyblock.py Outdated Show resolved Hide resolved
@@ -2982,6 +2978,39 @@ def build_and_install_one(ecdict, init_env):
return (success, application_log, errormsg)


def reproduce_build(app, reprod_dir_root):
Copy link
Member Author

Choose a reason for hiding this comment

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

@boegel I need some help with how to unit test this, not sure what's required to get a proper app instance

Copy link
Member Author

Choose a reason for hiding this comment

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

...or perhaps what is already tested is good enough?

Copy link
Member

Choose a reason for hiding this comment

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

@ocaisa Once you have a parsed easyconfig (for example for toy-0.0.eb in the tests, you can just create an easyblock instance like MyEasyBlock(parsed_easyconfig) (e.g. Toy(toy_ec)).

But I guess there's no strong need to test this function in isolation, since it's always being called. Just make sure whatever it produces it being checked, which you're already doing I think.

@boegel
Copy link
Member

boegel commented Nov 13, 2018

@ocaisa I'll check why @boegelbot isn't picking up on this, but there's a failing test:

ERROR: Test toy build produces expected reproducability files
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/453993240/lib/python2.6/site-packages/easybuild_framework-3.7.2.dev0-py2.6.egg/test/framework/toy_build.py", line 1557, in test_reproducability
    for framework_easyblock in FRAMEWORK_EASYBLOCK_MODULES:
NameError: global name 'FRAMEWORK_EASYBLOCK_MODULES' is not defined

@easybuilders easybuilders deleted a comment from boegelbot Nov 13, 2018
@ocaisa
Copy link
Member Author

ocaisa commented Nov 13, 2018

To address easybuilders/easybuild-easyblocks#1574 , should I be doing the reproducability dump before app.run_all_steps()? That should save me from changes made to the instance by the easyblocks while preserving everything else. I'll have to do it in the temporary space and then move it if we succeed with the build.

@ocaisa
Copy link
Member Author

ocaisa commented Nov 13, 2018

@boegel After a quick test, the approach in the last comment looks like it would solve easybuilders/easybuild-easyblocks#1574 but I would like your opinion before I implement.

@ocaisa
Copy link
Member Author

ocaisa commented Nov 13, 2018

Ok, I've implemented the move. This covers the case where modifications come from the command line (or configuration) and dependency resolution but doesn't cover what hooks might do. I think I'd also like to archive those but I don't know how to access them.

@ocaisa
Copy link
Member Author

ocaisa commented Nov 13, 2018

Urgh, I already see a problem with the hook example https://easybuild.readthedocs.io/en/latest/Hooks.html?highlight=hooks#example-hook-to-inject-a-custom-patch-file
I guess patch files should also be copied over to the reprod folder?

@easybuilders easybuilders deleted a comment from boegelbot Nov 15, 2018
@ocaisa
Copy link
Member Author

ocaisa commented Nov 16, 2018

I've reverted this back to simply also dumping the used easyblocks. Fixing the deeper problems of this approach (as in easybuilders/easybuild-easyblocks#1574) is for another PR.

@boegel
Copy link
Member

boegel commented Nov 21, 2018

lgtm, thanks @ocaisa!

@boegel boegel merged commit 1c70aef into easybuilders:develop Nov 21, 2018
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