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

Update script/licensed usage to accommodate upstream changes #4152

Merged
merged 3 commits into from
Jun 20, 2018

Conversation

lildude
Copy link
Member

@lildude lildude commented May 31, 2018

The next version of licensed will cause a breakage in our ability to add grammars, particularly around grabbing the license. This PR fixes that.

Description

licensed changed the method sources are defined in github/licensed#33. As we use a self-defined Filesystem source, we don't get the upstream changes for free so need to make changes ourselves else we'll get failures like this when users attempt to add or update a grammar:

$ script/add-grammar https://github.com/newgrammars/m3
Checking docker is installed and running
$ docker ps
Registering new submodule: vendor/grammars/m3
$ git submodule add -f https://github.com/newgrammars/m3 vendor/grammars/m3
$ script/grammar-compiler add vendor/grammars/m3
Confirming license
$ script/licensed --module /Users/lildude/github/linguist/vendor/grammars/m3
  > Caching licenes for linguist:
  > /Users/lildude/github/licensed/lib/licensed/command/cache.rb:21:in `block (3 levels) in run': undefined method `type' for Licensed::Source::Filesystem:Class (NoMethodError)
  > 	from /Users/lildude/github/licensed/lib/licensed/command/cache.rb:20:in `map'
  > 	from /Users/lildude/github/licensed/lib/licensed/command/cache.rb:20:in `block (2 levels) in run'
  > 	from /Users/lildude/github/licensed/lib/licensed/command/cache.rb:17:in `chdir'
  > 	from /Users/lildude/github/licensed/lib/licensed/command/cache.rb:17:in `block in run'
  > 	from /Users/lildude/github/licensed/lib/licensed/command/cache.rb:12:in `each'
  > 	from /Users/lildude/github/licensed/lib/licensed/command/cache.rb:12:in `flat_map'
  > 	from /Users/lildude/github/licensed/lib/licensed/command/cache.rb:12:in `run'
  > 	from script/licensed:53:in `<main>'

This PR fixes this by slightly modifying and simplifying our self-defined Filesystem source.

$ script/add-grammar https://github.com/newgrammars/m3
Checking docker is installed and running
$ docker ps
Registering new submodule: vendor/grammars/m3
$ git submodule add -f https://github.com/newgrammars/m3 vendor/grammars/m3
$ script/grammar-compiler add vendor/grammars/m3
Confirming license
$ script/licensed --module /Users/lildude/github/linguist/vendor/grammars/m3
Updating grammar documentation in vendor/README.md
$ bundle exec rake samples
$ script/sort-submodules
$ script/list-grammars
$
$ git status
On branch lildude/accom-new-licensed
Your branch is up to date with 'origin/lildude/accom-new-licensed'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git checkout -- <file>..." to discard changes in working directory)

	modified:   .gitmodules
	modified:   Gemfile
	modified:   grammars.yml

Untracked files:
  (use "git add <file>..." to include in what will be committed)

	vendor/grammars/m3/
	vendor/licenses/grammar/m3.txt

no changes added to commit (use "git add" and/or "git commit -a")
$

This will mean we need to enforce a minimum requirement for Licensed once a new release has been made that includes the changes from github/licensed#33 and github/licensed#41. I'll add this to this PR once the release has been made.

@jonabc do you see any problems with the approach I've taken here? Or maybe we should be changing the method we use entirely so we don't have to do this if licensed changes like this again in future?

Checklist:

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

@jonabc
Copy link

jonabc commented May 31, 2018

@lildude The changes look great.

Or maybe we should be changing the method we use entirely so we don't have to do this if licensed changes like this again in future?

I haven't tested this, but you might be able to remove the custom source entirely and use the manifest source type. You'd need a manifest file listing each dependent project, but I think it'll work if you have a single entry for each grammar, that is the path to the vendored grammar folder.

example manifest.json, yml also works 👍

{
  ...
  "vendor/grammars/llvm.tmbundle": "llvm.tmbundle",
  "vendor/grammars/m3": "m3",
  ...
}

Maintaining the manifest file would need to happen locally. Looks like that could be done with scripting via a re-mapping of grammars.yml.

@lildude
Copy link
Member Author

lildude commented Jun 1, 2018

Thanks @jonabc. That's an interesting approach. I'll investigate that avenue when I've got more time. For the moment, lets stick with what we've got.

@Alhadis @pchaigno see any problems with this PR?

@jonabc
Copy link

jonabc commented Jun 4, 2018

@lildude FYI, licensed-1.1.0 has been released going along with these changes

@Alhadis Alhadis mentioned this pull request Jun 4, 2018
15 tasks
@lildude
Copy link
Member Author

lildude commented Jun 18, 2018

This is looking good now and ready for review.

I've updated the licenses whitelist in this PR too as a recent update to licensee has introduced a change that has resulted in a slight variation in the hash for four of the grammars, resulting in all the CI failures that have been occurring in the last 2 weeks.

Copy link

@jonabc jonabc left a comment

Choose a reason for hiding this comment

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

Looks great from the licensed side :shipit:

@lildude
Copy link
Member Author

lildude commented Jun 20, 2018

Merging to get tests passing again.

@lildude lildude merged commit faa4de4 into master Jun 20, 2018
@lildude lildude deleted the lildude/accom-new-licensed branch June 20, 2018 16:14
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants