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

New languages: Modula-3 and Quake #4139

Merged
merged 6 commits into from
Jul 21, 2018
Merged

New languages: Modula-3 and Quake #4139

merged 6 commits into from
Jul 21, 2018

Conversation

ajcz
Copy link
Contributor

@ajcz ajcz commented May 24, 2018

This commits adds new language support for Modula-3 and Quake, a small scripting language primarily used for writing makefiles for Modula-3 projects.

Description

Please see issue #4136 for details.

Checklist:

@ajcz
Copy link
Contributor Author

ajcz commented May 24, 2018

The CI test failed because “cached license data” was missing.

Warnings:
/home/travis/build/github/linguist/vendor/licenses/grammar/m3.txt:

  • cached license data missing
    /home/travis/build/github/linguist/vendor/licenses/grammar/quake.txt:
  • cached license data missing
    307 dependencies checked, 2 warnings found.

Am I supposed to add these files manually? @lildude

@lildude
Copy link
Member

lildude commented May 24, 2018

The license should have automatically put in the right place when you ran script/add-grammar when adding the grammar. You may have missed adding it when committing all the files.

@lildude
Copy link
Member

lildude commented May 24, 2018

Sample license(s): I can’t be sure just yet. But since these files are published on GitHub they should have been permissibly licensed.

Unfortunately you can’t make that assumption.

Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

I've got a single comment below. LGTM otherwise!

- m3makefile
- m3overrides
extensions:
- ".tmpl"
Copy link
Contributor

Choose a reason for hiding this comment

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

The original post is missing a search link for this extension.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I’ll add any missing info later.

Copy link
Contributor Author

@ajcz ajcz May 24, 2018

Choose a reason for hiding this comment

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

I added a search link for .tmpl files, as well as other previously missing info. @pchaigno

@lildude
Copy link
Member

lildude commented May 24, 2018

I’ll take a closer look later, but from a quick glance several of these extensions don’t appear to meet the minimum “hundreds of repos” popularity requirement at the moment.

@ajcz
Copy link
Contributor Author

ajcz commented May 24, 2018

Let’s resolve this “missing cached license” issue first. I tried to run add-grammar again, but the license was still missing.

jc@[******]:~/work/linguist$ git checkout -- .

jc@[******]:~/work/linguist$ sudo ./script/add-grammar --replace m3 https://github.com/newgrammars/m3
Checking docker is installed and running
$ docker ps
Deregistering: vendor/grammars/m3
$ git submodule deinit vendor/grammars/m3
$ git rm -rf vendor/grammars/m3
$ script/grammar-compiler update -f
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
Updating grammar documentation in vendor/README.md
$ bundle exec rake samples
$ script/sort-submodules
$ script/list-grammars

jc@[******]:~/work/linguist$ ls ./vendor/licenses/grammar/*m3*
ls: cannot access './vendor/licenses/grammar/*m3*': No such file or directory

Or is there something wrong with https://github.com/newgrammars/m3 or https://github.com/newgrammars/quake?

@ajcz ajcz mentioned this pull request May 24, 2018
@lildude
Copy link
Member

lildude commented May 24, 2018

Ooof. Looks like Licensed 1.0.1 breaks things for us:

$ script/licensed --module /Users/lildude/tmp/trash/jcchu-linguist/vendor/grammars/m3
Caching licenes for jcchu-linguist:
  grammar dependencies:
caching /Users/lildude/tmp/trash/jcchu-linguist/vendor/grammars/m3
    Using m3 ()
License caching complete!
* jcchu-linguist grammar dependencies: 1
$ ls vendor/licenses/grammar/m3.txt
ls: vendor/licenses/grammar/m3.txt: No such file or directory
$

If I force the use of Licensed 1.0.0:

$ git diff github-linguist.gemspec
diff --git a/github-linguist.gemspec b/github-linguist.gemspec
index 246bfd66..951928e2 100644
--- a/github-linguist.gemspec
+++ b/github-linguist.gemspec
@@ -27,6 +27,6 @@ Gem::Specification.new do |s|
   s.add_development_dependency 'rake'
   s.add_development_dependency 'yajl-ruby'
   s.add_development_dependency 'color-proximity', '~> 0.2.1'
-  s.add_development_dependency 'licensed', '~> 1.0.0'
+  s.add_development_dependency 'licensed', '= 1.0.0'
   s.add_development_dependency 'licensee'
 end
$ bundle update
Fetching gem metadata from https://rubygems.org/.........
Resolving dependencies...
[...]
Fetching licensed 1.0.0 (was 1.0.1)
Installing licensed 1.0.0 (was 1.0.1)
[...]
Bundle updated!
$

... and try again, it works...

$ script/licensed --module /Users/lildude/tmp/trash/jcchu-linguist/vendor/grammars/m3
Caching licenes for jcchu-linguist:
  grammar dependencies:
caching /Users/lildude/tmp/trash/jcchu-linguist/vendor/grammars/m3
    Caching m3 ()
License caching complete!
* jcchu-linguist grammar dependencies: 1
$ ls vendor/licenses/grammar/m3.txt
vendor/licenses/grammar/m3.txt
$

@jonabc Any idea what's going on here? My suspicion is github/licensed#21 may be related.

@ajcz
Copy link
Contributor Author

ajcz commented May 24, 2018

How about I handwrite m3.txt and quake.txt and check them in now?

@jonabc
Copy link

jonabc commented May 30, 2018

@lildude thanks for the report!

It looks like there's two things going on here

  • licensed doesn't check whether there is a pre-existing license file when looking to reuse content.

This is related to the PR you linked in licensed. I'm looking to push out a new licensed release within the next day or two and I'll include a fix for that.

  • licensed metadata in linguist doesn't include version metadata, so the comparison of prior version metata (nil) to current version metata (nil) will always look like the content hasn't changed

This is also related to the PR you linked and will be fixed to force an evaluation if the current version string is nil.

On the linguist side, it might be good if linguist provided version metadata for each grammar's license content. In similar situations I've used the most recent commit SHA for a given folder as the version string. This would allow licensed to know if/when the license data has changed. It would also add chattiness around version updates in the license files as submodules are synced and SHAs updated. Completely up to you whether the cost/benefit makes sense.

@lildude
Copy link
Member

lildude commented May 31, 2018

On the linguist side, it might be good if linguist provided version metadata for each grammar's license content. In similar situations I've used the most recent commit SHA for a given folder as the version string. This would allow licensed to know if/when the license data has changed. It would also add chattiness around version updates in the license files as submodules are synced and SHAs updated. Completely up to you whether the cost/benefit makes sense.

Interesting idea. Lets discuss this outside the context of this PR.

@ajcz
Copy link
Contributor Author

ajcz commented Jun 3, 2018

Is there any other missing information? @pchaigno

@pchaigno
Copy link
Contributor

pchaigno commented Jun 3, 2018

https://github.com/search?utf8=✓&q=extension%3Atmpl+"readonly+proc"+NOT+nothack&type=Code [Not all Quake files are covered by this search.]

According to this search query, the .tmpl extension is in use in 127 repositories across 120 users---a bit short according to our contribution guidelines. Any other common keywords we could use to try and get a more exhaustive list of Quake files?

@ajcz
Copy link
Contributor Author

ajcz commented Jun 4, 2018

Unfortunately, the 14 keywords and those of the CM3-specific builtins that are really used in most makefiles (like interface, implementation, module, and perhaps import) are all pretty common, everyday-ish words and not specific enough to Quake.

Realistically, though, the number of Quake files isn’t going to be in the same order of magnitude as Modula-3 source files. Each CM3 program or library exists in a package—see any folder here for an example—which may have just a single “m3makefile”.

@jonabc jonabc mentioned this pull request Jun 4, 2018
15 tasks
@ajcz
Copy link
Contributor Author

ajcz commented Jun 5, 2018

@pchaigno The extension “.tmpl” has been removed, but CI is blocked by the same issue affecting #4157.

@lildude
Copy link
Member

lildude commented Jun 18, 2018

The test failures appear to be caused by recent changes to licensee which has resulted in a slightly modified hash for the four licenses in question. I'll address this in #4152.

@lildude
Copy link
Member

lildude commented Jun 21, 2018

Merging master into your branch should fix the failing tests.

@pchaigno
Copy link
Contributor

@lildude This looks good to go!

@lildude lildude merged commit 21568a7 into github-linguist:master Jul 21, 2018
@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.

None yet

4 participants