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

TagsCheck: handle license exception in first item of a grouping #1028

Conversation

tmzullinger
Copy link
Contributor

The change in 7d707f7 (TagsCheck: handle license exception in grouping, 2023-03-03) is insufficient when the exception is on the first item in a grouping, e.g.:

License: (Apache-2.0 WITH LLVM-exception OR NCSA) AND BSD-3-Clause

Adjust license_exception_regex to exclude a leading '(' as well as a trailing ')'.

This fixes a variation of the same issue which #1014 intended to fix. I don't know why I thought a leading parenthesis would be handle by the license_regex and not needed in license_exception_regex. (Just wishful thinking, perhaps.)

Here is the spec file used to create the additional test srpm:
valid-exception-begin-grouping.spec.txt

If this doesn't warrant a separate srpm from the previous test, I could replace the srpm in the existing test case. This new srpm has an exception at the start and end of the grouping (by virtue of being a grouping with only one entry).

As always, I'm happy to adjust things as needed. Thanks!

Copy link
Member

@danigm danigm left a comment

Choose a reason for hiding this comment

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

Looks good, but maybe it's worth to keep the \s exception in the regular expression? After the usage of this regex there's a str.strip so it's working as expected, but maybe it's a better regex this one:

r'([^(\s]+)\s(?:WITH|with)\s([^)\s]+)'

In 7d707f7 (TagsCheck: handle license exception in grouping,
2023-03-03), the regex lost the exclusion of space characters.  This
isn't immediately noticeable because we strip the strings generated by
the regex before use, but it's better to keep the regex more precise.

Suggested-by: Daniel Garcia Moreno <daniel.garcia@suse.com>
The change in 7d707f7 (TagsCheck: handle license exception in grouping,
2023-03-03) is insufficient when the exception is on the first item in a
grouping, e.g.:

    License: (Apache-2.0 WITH LLVM-exception OR NCSA) AND BSD-3-Clause

Adjust `license_exception_regex` to exclude a leading '(' as well as a
trailing ')'.
@tmzullinger tmzullinger force-pushed the license-exception-in-group-again branch from 322405d to 65abdbd Compare March 20, 2023 07:33
@tmzullinger
Copy link
Contributor Author

Looks good, but maybe it's worth to keep the \s exception in the regular expression? After the usage of this regex there's a str.strip so it's working as expected, but maybe it's a better regex this one:

r'([^(\s]+)\s(?:WITH|with)\s([^)\s]+)'

Yes, I think you're right about that. Relying on a later strip() to clean up the whitespace has the potential to cause problems if the code is re-arranged. That's worth avoiding. I added a preparatory commit which restores the \s exclusion to the existing regex at the trailing end and then amended the commit which aims to fix the beginning to include it.

Thanks!

@marxin marxin merged commit 1c8f7ca into rpm-software-management:main Mar 20, 2023
13 checks passed
@tmzullinger tmzullinger deleted the license-exception-in-group-again branch March 20, 2023 15:10
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

3 participants