Store grammar git version within license file when caching #4268

lildude · 2018-09-11T16:52:39Z

Description

As discovered by @pchaigno in #4265, running script/licensed results in some unexpected changes to the cached licenses. Some of this was because the grammars have been updated, but not the licenses (🙋‍♂️ my fault), and some due to stripping of newlines etc.

As pointed out by @jonabc at #4265 (comment):

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

update the Filesystem source to give "version" => <version> metadata for each dependency

switch to licensed's built-in manifest dependency source

I looked into the latter in #4267, and this proved way too complex for our needs so I've gone with the former.

At the same time, I've added script/licensed to be run just before script/licensed status in CI. This should cause the latter to flag any issues.

I've also updated the contributing guidelines so other Hubbers will remember to perform the script/licensed step each time the grammars are updated.

And of course, I've updated all of the licenses so they have a version flag. The only changes to actual licenses are where the license has legitimately changed within the grammar. Mr @Alhadis has done this a few times 😉

The curated flag has also been removed from the license files as this isn't honoured by anything and the version check should effectively lock us to an approved version. The script/licensed && script/licensed status should pick up any changes that'll need re-evaluating as-and-when the grammar is updated.

As licensed is performing a license check on our cached licenses and these are now tied to a version, I'm tempted to ditch the checks we're performing directly with licensee at https://github.com/github/linguist/blob/8bf9efa3702a1a43df85dc5cd72b63f3ff36871f/test/test_grammars.rb#L91-L142

Thoughts? @jonabc, I think the script/licensed status is effectively performing what ☝️ tests are. Am I right?

Checklist:

I am adding new or changing current functionality
- I have added or updated the tests for the new or changed functionality.

Fixes #4265

All licenses should remain the same except where there is clearly a change in license

lildude · 2018-09-11T16:55:41Z

script/licensed

@@ -1,7 +1,5 @@
 #!/usr/bin/env ruby

-# TODO: push these changes to licensor gem
-


We're not going to do this, so no need for this comment.

lildude · 2018-09-11T16:57:58Z

script/licensed

@@ -24,10 +22,10 @@ module Licensed

      def dependencies
        Dir.glob(@glob).map do |directory|
-          puts "caching #{directory}"


This is duplicating info that is already printed by Licensed. Licensed will print:

Caching licenses for linguist: grammar dependencies: Using sublime-rexx (2e304a34d3493e7453a430735191d91f356faa5a) Using Modelica (c841b17b0f441451b43a0b0e62bc6201a3456b83) Using atom-language-purescript (cc2cb471968eb331af7259aac219feb3017d709e) [...] Caching Sublime-Coq (94e43bdc194535cf5bae9306624039af9f38c014) [...]

Using signifies nothing has changed, Caching signifies a change.

lildude · 2018-09-11T16:58:46Z

vendor/licenses/config.yml

-    - Sublime-Lasso
+    - blitzmax        # No license file. License in README
+    - creole          # License filename is not LICENSE(.*) but rather "The MIT License (MIT)"
+    - Sublime-Lasso   # No license file. License in README


Equivalent of what we do in test_grammars.rb

jonabc · 2018-09-11T17:05:00Z

I think the script/licensed status is effectively performing what ☝️ tests are. Am I right?

@lildude licensed status covers the first three tests, but doesn't particularly care about the last three that check for unused values since that's related to the local repo and not the dependencies.

At the same time, I've added script/licensed to be run just before script/licensed status in CI. This should cause the latter to flag any issues.

I'm not sure this is entirely necessary. Now that you've built in versioning, licensed status will complain if a license is out of date, as a prompt to re-run licensed cache.

jonabc · 2018-09-11T17:07:30Z

vendor/licenses/grammar/ABNF.tmbundle.txt

 license: apache-2.0
 ---
-                                 Apache License
+Apache License


🤦‍♂️ ok licensed is too aggressive in stripping leading empty space

Oops. I meant to update our Licensed dependency too. If you're going to address this 🔜, I can hold off merging this PR and update the licenses again once a newer version of Licensed is available.

Hmm, I think you may not need to do anything @jonabc. Just updated the gemspec and I'm not seeing that over-stripping of whitespace with Licensed 1.3.3.

Hmmm, but now the version has changed 😕 Issue in Licensed coming up.

lildude · 2018-09-11T17:16:30Z

I'm not sure this is entirely necessary. Now that you've built in versioning, licensed status will complain if a license is out of date, as a prompt to re-run licensed cache.

Ah, cool. I'll remove that then. 🙇‍♂️

This reverts commit c86cfd1. This isn't needed now we're versioning out license files

Alhadis · 2018-09-11T17:23:52Z

The only changes to actual licenses are where the license has legitimately changed within the grammar. Mr @Alhadis has done this a few times 😉

This is to be expected. TTBOMK, year ranges in a project's license should be updated each time work has been made in a new year. 😉

Realistically, we should be expecting this of everybody...

lildude · 2018-09-13T14:04:23Z

After a little back-and-forth with @jonabc, I think I've come to a good solution which ties our cached license versions to the submodule SHAs. We don't update these independently of each other so it makes sense to use this tie.

Licensed may gain support for a submodule source which may allow us to simplify or even remove our Filesystem implementation in future. Until then, I think this does the trick.

Ready for reviews.

jonabc · 2018-09-13T14:52:42Z

I think the script/licensed status is effectively performing what ☝️ tests are. Am I right?

@lildude licensed status covers the first three tests, but doesn't particularly care about the last three that check for unused values since that's related to the local repo and not the dependencies.

@lildude I'm curious whether removing the tests is still being thought about. I'm curious whether licensed is able to cover those test cases or if there's something that it's not checking that it could and/or should.

lildude · 2018-09-13T14:58:31Z

Whoops. I meant to come back and remove those three tests. I think licensed status should be all we need for those. Thanks for the reminder.

lildude · 2018-09-13T16:24:49Z

I'm curious whether removing the tests is still being thought about.

🔥'd

jonabc · 2018-09-13T16:49:00Z

Are the remaining tests using PROJECT_WHITELIST, HASH_WHITELIST and LICENSE_WHITELIST still useful if license verification is done by licensed?

It looks like those tests only exist to ensure the hashes are valid for use in the tests that you removed in 7cf1233

lildude · 2018-09-14T15:38:33Z

Are the remaining tests using PROJECT_WHITELIST, HASH_WHITELIST and LICENSE_WHITELIST still useful if license verification is done by licensed?

🤔 good question. I believe they were originally written to keep track of when a whitelisted project gained a license and licensed status should definitely pick this up. The last test is skipped too. I think we can safely 🔥 those too, and remove the last of the licensee-based tests.

jonabc

❤️ licensed setup looks good. let me know if there's anything else you have questions on or want help with

lildude · 2018-09-19T12:39:38Z

@pchaigno or @Alhadis can one of your please give this PR a review to be sure I'm not missing anything. I'd like to get this in 🔜 so I can get a new release out. The next release is going to include quite a few changes so make take a bit longer to test.

🙇

lildude · 2018-09-19T12:41:40Z

I'm merging a ~~few~~ PR which will require this PR to be updated before merging too, but better they're in and only waiting on me to fix this PR 😄

lildude · 2018-09-19T12:47:07Z

which will require this PR to be updated before merging too

Done.

Alhadis

LGTM. 👍 I ran the tests after adding a grammar for a new language entry, and they all passed (the grammar is language-file-magic).

lildude · 2018-09-19T13:43:33Z

LGTM. 👍 I ran the tests after adding a grammar for a new language entry, and they all passed (the grammar is language-file-magic).

🎉 I've also tested with updating grammars and it does a good job of flagging changes there too. Gonna merge and start work on a new release.

lildude added 6 commits September 10, 2018 13:34

Add version when caching licenses

892f802

Add approved grammar licenses to config.yml

9c68fec

Add version and update cache of all licenses

53aa3c2

All licenses should remain the same except where there is clearly a change in license

Update release process with license caching deets

4368267

Double check licensed cached before checking status

c86cfd1

Only need to explicitly approve grammars without license files

f3ffe62

lildude commented Sep 11, 2018

View reviewed changes

jonabc reviewed Sep 11, 2018

View reviewed changes

Revert "Double check licensed cached before checking status"

f120841

This reverts commit c86cfd1. This isn't needed now we're versioning out license files

lildude mentioned this pull request Sep 11, 2018

Licensed::Git.version() is returning a bogus SHA from submodules github/licensed#80

Closed

lildude added 3 commits September 12, 2018 10:56

Change into submodule dir before getting SHA

de78626

Update Licensed min version requirement

58a5a58

Update license formats from Licensed 1.3.3 processing

026c6c0

Remove tests covered by "licensed status" test

7cf1233

🔥 more redundant tests and associated methods

e093035

jonabc approved these changes Sep 17, 2018

View reviewed changes

lildude requested review from pchaigno and Alhadis September 18, 2018 09:00

lildude added 2 commits September 19, 2018 13:44

Merge branch 'master' into lildude/store-version-with-license

9de1ebf

Update hy.tmLanguage cached license

aa21330

Alhadis approved these changes Sep 19, 2018

View reviewed changes

lildude merged commit 9720526 into master Sep 19, 2018

lildude deleted the lildude/store-version-with-license branch September 19, 2018 13:43

github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store grammar git version within license file when caching #4268

Store grammar git version within license file when caching #4268

lildude commented Sep 11, 2018

lildude Sep 11, 2018

lildude Sep 11, 2018

lildude Sep 11, 2018

jonabc commented Sep 11, 2018

jonabc Sep 11, 2018

lildude Sep 11, 2018

lildude Sep 11, 2018

lildude Sep 11, 2018

lildude commented Sep 11, 2018

Alhadis commented Sep 11, 2018

lildude commented Sep 13, 2018

jonabc commented Sep 13, 2018

lildude commented Sep 13, 2018

lildude commented Sep 13, 2018

jonabc commented Sep 13, 2018

lildude commented Sep 14, 2018

jonabc left a comment

lildude commented Sep 19, 2018

lildude commented Sep 19, 2018 •

edited

Loading

lildude commented Sep 19, 2018

Alhadis left a comment

lildude commented Sep 19, 2018

		@@ -1,7 +1,5 @@
		#!/usr/bin/env ruby

		# TODO: push these changes to licensor gem

Store grammar git version within license file when caching #4268

Store grammar git version within license file when caching #4268

Conversation

lildude commented Sep 11, 2018

Description

Checklist:

lildude Sep 11, 2018

Choose a reason for hiding this comment

lildude Sep 11, 2018

Choose a reason for hiding this comment

lildude Sep 11, 2018

Choose a reason for hiding this comment

jonabc commented Sep 11, 2018

jonabc Sep 11, 2018

Choose a reason for hiding this comment

lildude Sep 11, 2018

Choose a reason for hiding this comment

lildude Sep 11, 2018

Choose a reason for hiding this comment

lildude Sep 11, 2018

Choose a reason for hiding this comment

lildude commented Sep 11, 2018

Alhadis commented Sep 11, 2018

lildude commented Sep 13, 2018

jonabc commented Sep 13, 2018

lildude commented Sep 13, 2018

lildude commented Sep 13, 2018

jonabc commented Sep 13, 2018

lildude commented Sep 14, 2018

jonabc left a comment

Choose a reason for hiding this comment

lildude commented Sep 19, 2018

lildude commented Sep 19, 2018 • edited Loading

lildude commented Sep 19, 2018

Alhadis left a comment

Choose a reason for hiding this comment

lildude commented Sep 19, 2018

lildude commented Sep 19, 2018 •

edited

Loading