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

Move visualidiot license to its own type (#1999) #2001

Merged
merged 4 commits into from
Apr 14, 2020

Conversation

sutula
Copy link

@sutula sutula commented Apr 9, 2020

Per discussion in #1999, the now-defunct visualidiot.com license
should receive scrutiny by those who might be using code containing
such a license. It is currently mis-categorized as other-permissive,
inviting users to overlook the license. This commit moves the license
to it's own license type, visual-idiot.

Signed-off-by: Bryan Sutula sutula@redhat.com

Fixes #1999

Tasks

  • [x ] Reviewed contribution guidelines
  • [x ] PR is descriptively titled 📑 and links the original issue above 🔗
  • Tests pass -- look for a green checkbox ✔️ a few minutes after opening your PR
    Run tests locally to check for errors.
  • [x ] Commits are in uniquely-named feature branch and has no merge conflicts 📁

Per discussion in aboutcode-org#1999, the now-defunct visualidiot.com license
should receive scrutiny by those who might be using code containing
such a license.  It is currently mis-categorized as other-permissive,
inviting users to overlook the license.  This commit moves the license
to it's own license type, visual-idiot.

Signed-off-by: Bryan Sutula <sutula@redhat.com>
@pombredanne
Copy link
Member

@sutula Thank you ++ This is looking perfect.
I will merge tomorrow as soon as the tests are completed.

@sutula
Copy link
Author

sutula commented Apr 10, 2020

I am not sure I have the testing correct, but please do leave me any pointers for things I can do better. I'd like to learn in order to help with future license changes.

@pombredanne
Copy link
Member

@sutula you have this issue for now:
https://travis-ci.org/github/nexB/scancode-toolkit/jobs/673204599#L2331

E             AssertionError: Duplicate rules: 
E             file:///home/travis/build/nexB/scancode-toolkit/src/licensedcode/data/licenses/visual-idiot.LICENSE
E             file:///home/travis/build/nexB/scancode-toolkit/src/licensedcode/data/rules/visual-idiot_1.RULE

This is a consistency check done when the license index is being built and each text must be unique (ignoring formatting, case, spacing and punctuations).

To resolve it, remove the visual-idiot_1.RULE rule and its YAML file: they are now redundant since you created the .LICENSE record that has the same text.

The best way to catch these issues early is to run a test license scan locally after you made your updates in your local checkout.

Beyond that, you can also run a basic test suite too with this command:
pytest --verbose or pytest -vv -s (more verbose and dsiplay all outputs ) or pytest -vvs --numprocesses=6 (verbose and multiprocessing on 6 processes in ~ 60 seconds on my machine)

To run the extensive license+everyting test suite that runs on the CI you can use this:
pytest -vvs --numprocesses=5 --test-suite=validate
OR
pytest -vvs --numprocesses=5 --test-suite=validate tests/licensedcode to focus only on license tests

The whole test suite may take 20 to 30+ minutes or more as there about 28,000+ unit and integration tests that run in that case ...

@pombredanne
Copy link
Member

One thing I commonly do when adding a new license is to search for possible common notices to add a rules.

Looking at https://github.com/search?q=licence.visualidiot.com&type=Code and https://github.com/search?q=visualidiot&type=Code may provide a few interesting ones such as:

I would also likely add a simple licence.visualidiot.com as a reference. And may be a few more... May be visualidiot could be made as a rule too if all the visual production of this fine author are using the same license?

And all of these rules would be qualified with relevance: 100 since they are all highly/maximally relevant when detected and qualified them as is_license_tag, is_license_reference or is_license_notice as noted above.

See also https://github.com/jonstoler/The-Happy-License

Per discussion in aboutcode-org#1999, rules/visual-idiot_1.* duplicates the
license text that was placed in licenses/ and is not necessary.

Signed-off-by: Bryan Sutula sutula@redhat.com
@sutula
Copy link
Author

sutula commented Apr 10, 2020

Thanks so much for the feedback. I really appreciate it. What I'm now understanding is that scancode uses the union of both the texts in the licenses subdirectory as well as the contents of the rules subdirectory when it searches for licenses.

One thing I am struggling with is that I ran py.test before making any changes and there seemed to be lots of errors. Since I don't know the project, I captured the "before" and "after" errors and compared them. While there were 3 lines (out of 211) of differences, I could not see anything that would help me to understand that I had made a change that would make the test output worse. So I offer this as feedback to the project...it would be nice if py.test would run clean or if there was a current document describing current known errors. I understand that this type of work is both tedious and time-consuming. I suppose some of this is platform dependencies also, another nightmare.

I updated the pull request as you have suggested (remove visual-idiot_1). You mentioned other possible tags, notices, and references, as well as including relevance information. I'm afraid these concepts are above my current understanding of scancode, but I will try to study them. Please feel free to supplement this pull request with any of those things that you can add quickly. I also still don't know how to handle the existing test failures, but perhaps I can learn more about that in the future.

@sutula
Copy link
Author

sutula commented Apr 10, 2020

The current failing travis-ci test doesn't seem to be related to this proposed patch, as far as I can tell.

@pombredanne
Copy link
Member

One thing I am struggling with is that I ran py.test before making any changes and there seemed to be lots of errors. Since I don't know the project, I captured the "before" and "after" errors and compared them. While there were 3 lines (out of 211) of differences, I could not see anything that would help me to understand that I had made a change that would make the test output worse. So I offer this as feedback to the project...it would be nice if py.test would run clean or if there was a current document describing current known errors. I understand that this type of work is both tedious and time-consuming. I suppose some of this is platform dependencies also, another nightmare.

no nightmare, no worries! Feel free to paste your tests output. There should be no errors.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@sutula
Copy link
Author

sutula commented Apr 11, 2020

Thanks for offering to look at test output. I had run tests before and after like:

py.test -n 4 --log-file=../new_test_output

Attached find both a before making any changes and after making changes. Note that after making changes there is one less error. :-( This gives me the feeling that my environment is somehow different from what has been used before.
original_test_output.txt
new_test_output.txt

These are extra rules for a few common notices, tags and references
such that we cast a wider net on this license.

Signed-off-by: Philippe Ombredanne <pombredanne@nexb.com>
@pombredanne
Copy link
Member

@sutula both tests output are surprising and abnormal to me. I cannot fathom what would trigger this.
The only thing that would come to mind as a possible source would be if you use something like Windows WSL or Cygwin (or "git bash").
That said, I pushed a few extra rules to your branch and this is ready to merge now. Thank you++

@pombredanne pombredanne merged commit 1c80790 into aboutcode-org:develop Apr 14, 2020
@sutula sutula deleted the visual-idiot branch April 15, 2020 16:54
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.

other-permissive includes references to now-defunct visualidiot.com
2 participants