add --check-contrib CLI option by boegel · Pull Request #2551 · easybuilders/easybuild-framework

boegel · 2018-08-22T10:21:32Z

--check-contrib is an extension of the already existing --check-style: it checks whether the specified easyconfigs conform to the expected style, and also runs additional checks that are relevant when contributing back easyconfigs.

Currently the only additional check being done is whether SHA256 checksums are in place in all specified easyconfig files, but other checks can be added easily (for example to ensure that easyconfigs using the PythonPackage easyblock have download_dep_fail = True or use_pip = True set, when we want to start enforcing that, or potentially also whether the dependency versions are what is expected when a common toolchain is used).

The plan is to leverage check_sha256_checksums function being added here in easybuilders/easybuild-easyconfigs#5005 to also enforce SHA256 checksums in all easyconfigs that are touched by PRs.

…syconfigs/tools.py (+ tests)

…t to --check-style for now

…tyle aspects of --check-contrib

…asybuilders/easybuild-framework#2551)

migueldiascosta · 2018-08-22T12:29:23Z

tested it on a couple of easyconfigs, with and without errors and missing checksums, for both sources an patches, also tried empty checksums, seems to work as advertised

damianam · 2018-08-22T16:14:46Z

I wonder if it makes sense to have --check-style and --check-contrib as 2 different options. They are too similar to be considered 2 different options, IMO. Are there test cases you can think of, where one might want to check the style, and just the style?

boegel · 2018-08-22T17:32:47Z

@damianam I was wondering about that for a while when working on this, and concluded (after consulting @migueldiascosta on it) that adding --check-contrib was the right way to go forward.

What we intend to do with --check-contrib is way broader than what --check-style was intended for. Whether or not SHA256 checkums are in place is not really a style concern (it's more of a security concern), and the same goes for other things we plan to include in --check-contrib (the PythonPackage specific checks mentioned in the description, checking the dependency versions in easyconfigs using a (recent) common toolchain, etc.).

Also, --check-style has been around for a while, so we can't just rename it to --check-contrib either, since we need to retain backward compatibility.

What we can do, on top of what this PR does already, is to deprecate --check-style (i.e. mark it to be removed in EasyBuild v4.0), and suggest using --check-contrib instead.

But maybe some people use --check-style to try and maintain a consistent style in their own easyconfigs repository, and they don't want to enforce all the other things we'll be adding in --check-contrib...

In short, --check-contrib is broader than --check-style, which we shouldn't remove in EasyBuild v3.x (and there's very little value in doing so anyway, imho).

damianam · 2018-08-23T10:35:27Z

What we can do, on top of what this PR does already, is to deprecate --check-style (i.e. mark it to be removed in EasyBuild v4.0), and suggest using --check-contrib instead.

That would make sense to me. It is a pity to be "hold hostage" of backwards compatibility. Because this PR is a bit more complicated than it could be, just to deal with that situation.

akesandgren · 2018-08-23T11:08:53Z

I haven't tried check-style yet, but by the sound of it I would prefer to keep it, unless check-contrib will be able to take arguments like --check-contrib=style so one can run a fast style check without the rest during development...

migueldiascosta · 2018-08-23T13:55:02Z

I think @akesandgren just answered the question of if it ever makes sense to check for style but not other things.

If the problem is only backward compatibility, we can leave --check-style merely as an alias to Ake's --check-contrib=style?

boegel · 2018-08-23T18:10:56Z

Based on @akesandgren's feedback, it seems like keeping --check-style (and hence not deprecating it) makes sense.

I started looking into making --check-contrib accept an optional list of checks to run, so you can do --check-contrib=style, but the fact that it's optional has a nasty side-effect due to a limitation in the option parser (sort of)...

When running something like eb --check-style bzip2.eb, the bzip2.eb would be considered an argument to --check-contrib, equivalent to using --check-style=bzip2.eb, which would then of course fail since bzip2.eb is not a known type of check (or because no easyconfig files are specified, which would be an even more confusing error to the end user)...

It's not that this can't be implemented, it's just that you'd always have to run eb bzip2-1.0.6.eb --check-style rather than eb --check-style bzip2-1.0.6.eb to avoid running into an error like "bzip2-1.0.6.eb' is not a known check type".

We have the same problem with other options, for example --inject-checksums and --robot, which also optionally take an argument (either a checksum type like md5 or sha256, or a (list of) directories, resp.), and I'm not inclined to add more situations like this, because I end up shooting myself in the foot with these options (running into an error) over and over again (muscle memory, I guess)...

By making run_contrib_check a bit smarter, we could see if a specified unknown check type corresponds to an existing (easyconfig) file, and then promote the not-a-check-type-but-a-file, but the implementation would be quite messy since we need a parsed easyconfig file, not a filepath, so... ugh.

Long story short: just keeping --check-style instead is a lot easier imho, since the work is already done... ;)

boegel · 2018-08-28T14:48:07Z

@damianam @akesandgren Any reason to hold this back?

This PR is a requirement to start automatic checking of SHA256 checksums in easyconfig PRs...

damianam · 2018-08-24T16:00:06Z

    style_check_passed = True
    for path in paths:
+        # if an EasyConfig instance is provided, just grab the corresponding file path
+        if isinstance(path, EasyConfig):


Not that doing this check is bad, but it is confusing that paths is a list of paths, yet we assume that EasyConfig objects can be passed here. Shouldn't we change the parameter description and name accordingly?

damianam · 2018-08-28T15:34:40Z

+        for (fn, chksum) in checksums:
+            # check that there's only one checksum specified per file (i.e. a string, not a list/tuple)
+            if not isinstance(chksum, basestring):
+                msg = "Exactly one (SHA256) checksum should be specified for %s in %s" % (fn, ec_fn)


Change the message to "One string containing a SHA256 checksum should be specified for %s in %s" % (fn, ec_fn)? That is clearer if someone mistakenly passes a string within a list/tuple.

Wouldn't "A string ..." be more correct language wise?
And ""must be specified" perhaps?

damianam · 2018-08-28T15:40:42Z

+def run_contrib_checks(ecs):
+    """Run contribution check on specified easyconfigs."""
+
+    def print_result(result, label):


Rename the result parameter to passed? Seems clearer to me.

damianam · 2018-08-28T15:46:07Z

+    print_msg("\nChecking for SHA256 checksums in %d easyconfig(s)...\n" % len(ecs), prefix=False)
+    sha256_checksums_ok = True
+    for ec in ecs:
+        sha256_res = check_sha256_checksums([ec])


rename sha256_res to sha256_issues?

damianam · 2018-08-28T15:52:06Z

    return res


+def check_contrib_or_style(ecs, check_contrib, check_style):


The current naming seems to indicate "Check if either contrib or style checks were done", as in "it is not possible to not run none of them" check_checks seems to be more consistent to me, but admittedly not ideal......

I see your point, I'll rename it run_contrib_style_checks

damianam · 2018-08-28T16:00:08Z

+        self.assertTrue(res)
+        # multiple checksums listed for source tarball, while exactly one (SHA256) checksum is expected
+        self.assertTrue(res[-1].startswith("Exactly one (SHA256) checksum should be specified for toy-0.0.tar.gz"))
+


Test of bundles is missing

Yeah, very good point, as I was fixing this I noticed the part where we verify that checksums for Bundle components didn't actually work... :)

I did have to copy-paste the bundle.py easyblock under test/ though in order to write a test for this...

houndci-bot · 2018-08-31T16:19:35Z

+        elif isinstance(ec, basestring):
+            path = ec
+        else:
+            raise EasyBuildError("Value of unknown type encountered in cmdline_easyconfigs_style_check: %s (type: %s)",


undefined name 'EasyBuildError'

houndci-bot · 2018-08-31T16:19:35Z

+        elif isinstance(ec, basestring):
+            path = ec
+        else:
+            raise EasyBuildError("Value of unknown type encountered in cmdline_easyconfigs_style_check: %s (type: %s)",


undefined name 'EasyBuildError'

boegel · 2018-08-31T17:16:29Z

I've marked this WIP again because I'm not happy at all with the way in which I'm dealing with checking whether Bundle components have checksums...

The "separation of concerns" principle is not honored by having to make the EasyBuild framework aware of the guts of a particular easyblock (i.e. the comp_cfgs bit), I'll look into a better way of dealing with this...

boegel · 2018-08-31T19:13:24Z

I've reworked this significantly, the implementation is a lot cleaner now since it supports leaving the stuff specific to the Bundle easyblock there, see easybuilders/easybuild-easyblocks#1496 .

@damianam, @akesandgren Please re-review?

…asyBlock, which can be extended in easyblocks (e.g. Bundle)

damianam · 2018-09-03T13:23:54Z

                else:
                    raise EasyBuildError("Checksum verification for %s using %s failed.", fil['path'], fil['checksum'])

+    def check_checksums_for(self, ent, sub='', source_cnt=None):


The source_cnt parameter seems buggy to me. It is used to limit the number of sources in extensions, right? But it actually doesn't do that if I understand the code correctly. You can have an extension with 2 sources, a single checksum, and it will be all fine, since we are passing source_cnt = 1, so the count is artificially limited to 1, but in reality 2 sources are listed. Then, there is just a single checksum, but we should have 2. Maybe it makes more sense to have a is_extension = False parameter, and if it is explicitly set to True, verify that the calculated source_cnt is exactly 1?

There's a hard assumption in easyblock.py currently that each extension only has a single source file. The extension handling mechanism doesn't pick up on sources (so you can't specify a list of sources), it only provides source_tmpl to tweak the name of the source tarball for the extension (and it has a proper default for common type of extensions like Python packages, etc.).

That's why the source_cnt=1 thing is being used here, since there simply isn't a list of sources to count for extensions...

damianam · 2018-09-03T13:35:42Z

-            elif not sha256_regex.match(chksum):
-                msg = "Checksum '%s' for %s in %s must be a SHA256 checksum" % (chksum, fn, ec_fn)
-                checksum_issues.append(msg)
+        klass = get_easyblock_class(ec['easyblock'], name=ec['name'])


eb_class?

boegel added 3 commits August 20, 2018 09:31

add implementation of check_sha256_checksums function in framework/ea…

6e71697

…syconfigs/tools.py (+ tests)

add preliminary support for new --check-contrib CLI option, equivalen…

4c4e567

…t to --check-style for now

rework test_check_style into test_check_contrib_style to also cover s…

14f8a7c

…tyle aspects of --check-contrib

boegel added the enhancement label Aug 22, 2018

boegel added this to the 3.7.0 milestone Aug 22, 2018

boegel added a commit to boegel/easybuild-easyconfigs that referenced this pull request Aug 22, 2018

leverage check_sha256_checksums function provided by framework (cfr. e…

c8590f5

…asybuilders/easybuild-framework#2551)

boegel mentioned this pull request Aug 22, 2018

add test to enforce SHA256 checksums in touched files in pull requests to develop (REVIEW) easybuilders/easybuild-easyconfigs#5005

Merged

2 tasks

also check SHA256 checksums in --check-contrib

a47b893

boegel force-pushed the check_contrib branch from 143168a to a47b893 Compare August 22, 2018 10:34

boegel requested a review from damianam August 22, 2018 13:07

damianam requested changes Aug 28, 2018

View reviewed changes

boegel added 2 commits August 31, 2018 09:03

fix style remarks

eec5caf

fix checking of checksums for components when Bundle easyblock is used

6d3aefc

houndci-bot reviewed Aug 31, 2018

View reviewed changes

fix missing import

b6fc8c2

boegel changed the title ~~add --check-contrib CLI option~~ add --check-contrib CLI option (WIP) Aug 31, 2018

add is_sha256_checksum function to filetools module

919dac1

boegel changed the title ~~add --check-contrib CLI option (WIP)~~ add --check-contrib CLI option Aug 31, 2018

easybuilders deleted a comment from boegelbot Aug 31, 2018

boegel mentioned this pull request Aug 31, 2018

customise check_checksums method in Bundle easyblock easybuilders/easybuild-easyblocks#1496

Merged

boegel force-pushed the check_contrib branch from 968de2f to 980fe48 Compare August 31, 2018 19:14

refactor check_sha256_checksums via check_checksums(_for) method in E…

7b94481

…asyBlock, which can be extended in easyblocks (e.g. Bundle)

boegel force-pushed the check_contrib branch from 980fe48 to 7b94481 Compare August 31, 2018 19:15

akesandgren reviewed Sep 3, 2018

View reviewed changes

Comment thread easybuild/framework/easyblock.py

damianam requested changes Sep 3, 2018

View reviewed changes

rename klass to eb_class in check_sha256_checksums function

3b5eff3

damianam approved these changes Sep 3, 2018

View reviewed changes

damianam merged commit c33b497 into easybuilders:develop Sep 3, 2018

boegel deleted the check_contrib branch September 4, 2018 07:11

boegel mentioned this pull request Sep 20, 2018

libXp and printproto added to X11 bundle easybuilders/easybuild-easyconfigs#6873

Merged

		return res


		def check_contrib_or_style(ecs, check_contrib, check_style):

Conversation

boegel commented Aug 22, 2018

Uh oh!

migueldiascosta commented Aug 22, 2018

Uh oh!

damianam commented Aug 22, 2018

Uh oh!

boegel commented Aug 22, 2018

Uh oh!

damianam commented Aug 23, 2018

Uh oh!

akesandgren commented Aug 23, 2018

Uh oh!

migueldiascosta commented Aug 23, 2018

Uh oh!

boegel commented Aug 23, 2018

Uh oh!

boegel commented Aug 28, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

akesandgren Aug 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

boegel commented Aug 31, 2018

Uh oh!

boegel commented Aug 31, 2018

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

akesandgren Aug 28, 2018 •

edited

Loading