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

Force exact license match #11381

Merged

Conversation

scheibelp
Copy link
Member

Previous license checks have allowed for significant deviation in the license header as long as a few parts of the license matched the general format.

This updates the license check to require almost an exact match, ignoring leading/trailing whitespace, or leading #/. characters (to account for python/bash and rst files).

The check is updated to print a debug message if a date mismatch is detected (e.g. if the copyright ends in 2018 vs. 2019 at this point). A mismatch on the upper end of the date range is not considered an error (so long as it falls within [2017, 2029]).

As an example of how much stricter this is, Copyright 2018 Lawrence... from the simgrid package was considered a mismatch, and had to be updated to Copyright 2013-2019 Lawrence. This could be relaxed to allow for matching on a single year.

The only other package considered noncompliant (which will cause this PR to fail testing) was aom, which updates the first line of the license:

Copyright 2013-2019 Trinity College Dublin and other Spack Project

Copy link
Member

@tgamblin tgamblin left a comment

Choose a reason for hiding this comment

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

Looks mostly good -- two minor requests. Thanks!


def _check_license(lines, path):
license_lines = [
r'Copyright 2013-(?:201[789]|202\d) Lawrence Livermore National Security, LLC and other', # noqa: E501
Copy link
Member

Choose a reason for hiding this comment

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

Make it ensure the year of the most recent commit, with a grace period of January (so if the commit is in January 2020, 2019 is still an ok year, so we have a month to get it updated). Taking commit date ensures that past commits will continue to pass tests when checked out.

Copy link
Member Author

@scheibelp scheibelp May 7, 2019

Choose a reason for hiding this comment

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

So to be clear: it should fail if the date is not year of latest commit or (year-1 if latest commit is january)?

Right now it is less strict: it only prints a debug message if the end date doesn't match. About 20 files will have to change if "2018" is an error (which is fine, I'm just clarifying).

Copy link
Member

Choose a reason for hiding this comment

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

Someone could run this in a Spack instance stripped of its git history, I don't think we want to tie it to a commit.

Copy link
Member Author

@scheibelp scheibelp May 7, 2019

Choose a reason for hiding this comment

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

I'll note that doing any checking at all on the date is entirely new, and possibly worth handling in another PR, especially since both of you make good points: for example perhaps it should try to use git, and be more lenient if git is not available. Another possibility is that it could parse the date from a single "official" file that's part of the Spack core, so as long as we remember to update that, then the packages will be handled (and all without invoking git).

Side note: running spack license requires git to be available, but git is never actually run, so Spack instances stripped of git history should be able to run spack license (that's not to say it must be supported).

lib/spack/spack/cmd/license.py Outdated Show resolved Hide resolved
Copy link
Member

@becker33 becker33 left a comment

Choose a reason for hiding this comment

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

Only one minor nitpick.

lib/spack/spack/cmd/license.py Outdated Show resolved Hide resolved
@scheibelp
Copy link
Member Author

@smcgrat do you mind if I replace the modified license in https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/aom/package.py

# Copyright 2013-2019 Trinity College Dublin and other Spack Project
# Developers. See the top-level COPYRIGHT file for details.
#
# SPDX-License-Identifier: (Apache-2.0 OR MIT)

With the default license:

# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
# Spack Project Developers. See the top-level COPYRIGHT file for details.
#
# SPDX-License-Identifier: (Apache-2.0 OR MIT)

? Some authors have added an # Author:... comment just below, as in https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/perl/package.py - would that be an acceptable option?

@smcgrat
Copy link
Contributor

smcgrat commented May 9, 2019

@smcgrat do you mind if I replace the modified license in https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/aom/package.py

# Copyright 2013-2019 Trinity College Dublin and other Spack Project
# Developers. See the top-level COPYRIGHT file for details.
#
# SPDX-License-Identifier: (Apache-2.0 OR MIT)

With the default license:

# Copyright 2013-2019 Lawrence Livermore National Security, LLC and other
# Spack Project Developers. See the top-level COPYRIGHT file for details.
#
# SPDX-License-Identifier: (Apache-2.0 OR MIT)

No problem. Go for it. Sorry about that, I think I was just copying something else that was obviously incorrect.

? Some authors have added an # Author:... comment just below, as in https://github.com/spack/spack/blob/develop/var/spack/repos/builtin/packages/perl/package.py - would that be an acceptable option?

Would the following be acceptable:

# Author: Darach Golden <dgolden@tchpc.tcd.ie>,
# Institution: Trinity Centre for High Performance Computing, https://www.tchpc.tcd.ie/
# Date: May 09, 2019
# Author: Sean McGrath <smcgrat@tchpc.tcd.ie>
# Institution: Trinity Centre for High Performance Computing, https://www.tchpc.tcd.ie/
# Date: May 09, 2019

@scheibelp
Copy link
Member Author

Would the following be acceptable:

Yes! I'll add that here.

@scheibelp scheibelp force-pushed the bugfix/stricter-license-verification branch from f8fcdc2 to db526b0 Compare May 9, 2019 17:58
The license text is now expected to match almost exactly (not
accounting for formatting in different file types (e.g. rst vs.
bash script vs. python)
@scheibelp scheibelp force-pushed the bugfix/stricter-license-verification branch from db526b0 to b56f777 Compare May 9, 2019 18:04
@scheibelp scheibelp dismissed stale reviews from becker33 and tgamblin May 9, 2019 19:37

Review comments addressed

@scheibelp scheibelp merged commit 53ec16c into spack:develop May 9, 2019
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.

None yet

4 participants