Details

Reviewers

Commits

rP22010: Add Coala Bear for the license header check.

Summary

Added LicenseBear. Probably there is a better way to give time of the last modification, also I don't how it's stored on the server.

Test Plan

Run coala

Diff Detail

Repository

rP 0 A.D. Public Repository

Lint

Automatic diff as part of commit; lint not applicable.

Unit

Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vladislavbelov created this revision.May 1 2017, 9:45 PM

vladislavbelov added a parent revision: D213: Linting with Coala.

Build is green

Updating workspaces.
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (306 tests)..................................................................................................................................................................................................................................................................................................................OK!

http://jw:8080/job/phabricator/958/ for more details.

Harbormaster completed remote builds in B1392: Diff 1590.May 1 2017, 11:28 PM

I didn't test yet but I have a remark concerning the use of the modification date in the file metadata: if somebody makes a checkout of the game, they will always have false positives for files older than the year of the checkout if they try to run coala on the entire repository. So the modification date is not a good choice indeed.

I think you should:

Make the bear only check the year is the current year, and the Jenkins script will only run the bear on the modified files so that's good
See if it's possible to enable the bear only if coala is called by the Jenkins script

On top of that, you should patch the Jenkins script now that it's under version control (that comes with the second bullet point).

I'd also rename the bear to LicenseYearBear because it doesn't check the license itself (and I'm afraid the coala devs use that name in the future for a bear that checks licenses). ?

Thank you very much for working on that!

This revision now requires changes to proceed.Nov 9 2017, 12:40 AM

You could do much better by git-logging that file and checking for the last date of change, imo.

edit: well svn logging but same idea.

In D404#40142, @wraitii wrote:

You could do much better by git-logging that file and checking for the last date of change, imo.
edit: well svn logging but same idea.

Wouldn't it bad by speed? Because git/svn isn't really fast, especially when many files are changed. So, why not Itms's suggestion to use current year? It's simple and it works. But with git/svn we need additional libraries or parsers.

Wouldn't it bad by speed? Because git/svn isn't really fast, especially when many files are changed. So, why not Itms's suggestion to use current year? It's simple and it works. But with git/svn we need additional libraries or parsers.

Well if you run it only in the jenkins script, I think it's generally fine, but itms might disagree - and you do have a point. Just saying that imo it's the only way to be entirely accurate, but maybe we don't need to be.

In D404#40207, @wraitii wrote:

Well if you run it only in the jenkins script, I think it's generally fine, but itms might disagree - and you do have a point. Just saying that imo it's the only way to be entirely accurate, but maybe we don't need to be.

"if you run it only in the jenkins script", you'll get wrong results on git/svn too. I think you know, that you can't trust git/svn too (i.e. https://stackoverflow.com/questions/454734/how-can-one-change-the-timestamp-of-an-old-commit-in-git).

In D404#40207, @wraitii wrote:

Well if you run it only in the jenkins script, I think it's generally fine, but itms might disagree - and you do have a point. Just saying that imo it's the only way to be entirely accurate, but maybe we don't need to be.

Also we can request current year from the wfg server. It has best precision than all solutions above, but it's too complicated.

Using svn might be fine but I'm afraid indeed it's a lot of work. If you can find a way perfect, but if it's too difficult, checking the current year is enough I think.

Adds the SVN support.

Successful build - Chance fights ever on the side of the prudent.

Updating workspaces...
Build (release)...
Build (debug)...
Running release tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Running debug tests...
Running cxxtest tests (307 tests)...................................................................................................................................................................................................................................................................................................................OK!
Checking XML files...
Relax-NG validity error : Extra element props in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element variant failed to validate content
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element group has extra content: variant
Relax-NG validity error : Extra element group in interleave
/public/art/actors/structures/carthaginians/stable_elephant.xml:0: Relax-NG validity error : Element actor failed to validate content

Executing section Default...
Executing section Source...
Executing section JS...

Harbormaster completed remote builds in B3689: Diff 4176.Nov 13 2017, 11:16 PM

Stan added a subscriber: Stan.Nov 13 2017, 11:46 PM

Stan added inline comments.

build/coala/LicenseYearBear.py
18 ↗	(On Diff #4176)	Any reason for this over anything else ?

vladislavbelov added inline comments.Nov 14 2017, 8:49 PM

build/coala/LicenseYearBear.py
18 ↗	(On Diff #4176)	This was or in some bear files or in some our files. So, I don't mind.

@Itms I guess you missed your deadline; Welcome to 2019 :)

This is a nice bear, and it works! All my apologies for letting this one aside for so long ?

I have only one request, in case someone runs the bear on the whole repo, they might have files not under control lying around. So you need to handle those files, or the svn module throws an exception.

I made the following modification locally and it works. If you include it, I'll accept the patch. Thanks again for the patience ?

build/coala/LicenseYearBear.py
8 ↗	(On Diff #4176)	from svn.exception import SvnException is needed by my change.
26 ↗	(On Diff #4176)	You could `break` out of the loop to save time here.
27 ↗	(On Diff #4176)	if not was_modified: try: return client.info()['commit_date'].year except SvnException: pass return date.today().year