lildude
commented
6 years ago Is that expected?
No. This sounds like licensed's normalisation process. Will try get this fixed before the next release.
Closed pchaigno closed 5 years ago
lildude
commented
6 years ago Is that expected?
No. This sounds like licensed's normalisation process. Will try get this fixed before the next release.
Alhadis
commented
6 years ago More like abnormalisation. 😉
pchaigno
commented
6 years ago @lildude If I'm trying to change the source for a submodule, can I first do a PR with these changes? Is that how you plan to fix it?
lildude
commented
6 years ago Is that how you plan to fix it?
I have no idea how I'll address this just yet. 😉
lildude
commented
6 years ago So this is as a result of changes introduced in licensee/licensed like those in https://github.com/github/licensed/pull/21 and the recent normalisation changes and I've been caught being old and forgetting to do anything about it on the Linguist side. 😊
It removes newlines at the end of files, adds lines to license texts,
I think this is part of the recent normalisation improvements made to Licensee. Am I right here @benbalter, @mlinksva and/or @jonabc?
and even deletes a few files.
This is going to be because the two grammars, scala and matlab, have been removed as they were swapped out in https://github.com/github/linguist/pull/4044 and https://github.com/github/linguist/pull/4000 respectively but the old cached licenses weren't removed.
Is that expected?
It wasn't, but I think it is now :grin:
Should we be running ./script/licensed from time to time??
Yeah, I think I should probably be running this every time I make a release with updated grammars and when a grammar is swapped out as this is when any license change is likely to happen, though I was under the impression the running of script/licensed status during testing as defined at https://github.com/github/linguist/blob/b5e2687b2ebe02a6f30943b30ddd8ee08322881c/.travis.yml#L14 ... would do the trick and highlight such cases. 🤔
Something that is unexpected is the reviewed elements defined in the config file at https://github.com/github/linguist/blob/b5e2687b2ebe02a6f30943b30ddd8ee08322881c/vendor/licenses/config.yml#L21-L26 appear to be ignored (suggestion from https://github.com/github/licensed/issues/7#issuecomment-365000922), for example:
$ git --no-pager diff vendor/licenses/grammar/Sublime-Lasso.txt
diff --git a/vendor/licenses/grammar/Sublime-Lasso.txt b/vendor/licenses/grammar/Sublime-Lasso.txt
index b3595505..baac3ebe 100644
--- a/vendor/licenses/grammar/Sublime-Lasso.txt
+++ b/vendor/licenses/grammar/Sublime-Lasso.txt
@@ -1,10 +1,5 @@
---
type: grammar
name: Sublime-Lasso
-license: public domain
-curated: true
+license: none
---
-
-License Information
-
-This package is provided as-is and is placed in the Public Domain. Sublime Text itself is proprietary software and is probably copyrighted by Sublime HQ Pty LtdBut now I look at the source, this appears to be expected.
I now also realise the "better option" mentioned in https://github.com/github/licensed/issues/7#issuecomment-365000922 only works for grammars which have their licenses in a separate file which is why we're seeing this license getting trampled all over.
So for this final part of the problem, @jonabc I think we may need to change licensed's caching behaviour to consider reviewed and if a file is marked a reviewed and the caching produces an empty file, ignoring the metadata, leave the file as-is.
This should stop the changes and thus the "missing license text" warnings we see if we run script/licensed status immediately after script/licensed which introduces all the changes:
Warnings:
/Users/lildude/github/linguist/vendor/licenses/grammar/language-csharp.txt:
- license needs reviewed: other.
/Users/lildude/github/linguist/vendor/licenses/grammar/language-renpy.txt:
- license needs reviewed: other.
/Users/lildude/github/linguist/vendor/licenses/grammar/sublime-shen.txt:
- license needs reviewed: other.
/Users/lildude/github/linguist/vendor/licenses/grammar/Sublime-Lasso.txt:
- missing license text
/Users/lildude/github/linguist/vendor/licenses/grammar/ant.tmbundle.txt:
- license needs reviewed: other.
/Users/lildude/github/linguist/vendor/licenses/grammar/SCSS.tmbundle.txt:
- missing license text
- license needs reviewed: other.
/Users/lildude/github/linguist/vendor/licenses/grammar/creole.txt:
- missing license text
/Users/lildude/github/linguist/vendor/licenses/grammar/ioke-outdated.txt:
- license needs reviewed: other.
/Users/lildude/github/linguist/vendor/licenses/grammar/blitzmax.txt:
- missing license text
/Users/lildude/github/linguist/vendor/licenses/grammar/atom-fsharp.txt:
- license needs reviewed: other.
309 dependencies checked, 10 warnings found.I still need to look into the others as to why they've changed, but out of time for today.
jonabc
commented
6 years ago So for this final part of the problem, @jonabc I think we may need to change licensed's caching behaviour to consider reviewed and if a file is marked a reviewed and the caching produces an empty file, ignoring the metadata, leave the file as-is.
@lildude this question came up very recently in a different context and the answer was that every metadata file should have license text, even if it's set as reviewed. Failure to have license text should still result in the warnings shown.
@lildude @pchaigno what are your goals for using licensed? I'm happy to help work through any usage issues or questions, I'm just a little unclear on what you'd like it to be doing.
lildude
commented
6 years ago @lildude this question came up very recently in a different context and the answer was that every metadata file should have license text, even if it's set as reviewed. Failure to have license text should still result in the warnings shown.
That's the thing. The original license file has been manually populated, normally from the text in the README or another file with a name that doesn't match the normal license file nomenclature, and then reviewed. It's when the caching runs that we end up with a problem as it removes this already approved text.
In the example I highlighted above, the license file is populated with the license included in the original README. In another case, the repo uses a file named The MIT License (MIT) to hold the license.
@lildude @pchaigno what are your goals for using
licensed? I'm happy to help work through any usage issues or questions, I'm just a little unclear on what you'd like it to be doing.
In this scenario, ideally, multiple invocations shouldn't make changes to the cached files without good reason, like an actual change in license or content, especially across patch releases of licensed or licensee. Many of the changes pointed out by @pchaigno appear to be related to normalisation changes which shouldn't have affected our cached license files.
A quick review of the remaining warnings in the output above show a few obvious license mis-detections due to wild formatting changes and others aren't quite so obvious as to why they're now being mis-detected, even though the license is still the same.
jonabc
commented
6 years ago The original license file has been manually populated, normally from the text in the README or another file with a name that doesn't match the normal license file nomenclature, and then reviewed. It's when the caching runs that we end up with a problem as it removes this already approved text.
@lildude licensed uses logic based on version metadata to detect whether changes are needed. The issue here is that linguist uses a custom enumeration source that doesn't provide version metadata.
The solutions to this are
Filesystem source to give "version" => <version> metadata for each dependencymanifest dependency source Given that there was discussion about replacing some of the custom tests with more licensed integration, maybe switching to the built-in dependency sources + deeper integration is the best solution?
lildude
commented
6 years ago Given that there was discussion about replacing some of the custom tests with more licensed integration, maybe switching to the built-in dependency sources + deeper integration is the best solution?
Yes, this sounds like it might be the best thing going forward. I'll start a PR today to implement the manifest method as we've been wanting to move away from our custom filesystem method for a while (that comment at the top of our script has been there for as long as I've been working with Linguist 😁).
Running
./script/licensedon an unmodified clone of Linguist seems to create a huge mess (see below). It removes newlines at the end of files, adds lines to license texts, and even deletes a few files. Is that expected? Should we be running./script/licensedfrom time to time??Template removed because it doesn't apply.