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

checksrc: add COPYRIGHT year check #3303

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
2 participants
@danielgustafsson
Member

danielgustafsson commented Nov 22, 2018

This is a patch which I had shelved deeming it too uninteresting/complicated to be useful, but when discussing the recent snprintf() switcheroo in #3297 the topic was brought up again. This is more of a discussion piece/show-and-tell PR than something I really think we should do, but working code is generally nicer to discuss around than handwaving so here goes.

The check for updated copyright year is currently not enforced on any file but only on files edited and/or committed locally. This is due to the amount of files which aren't updated with their correct copyright year at the time of their respective commit. If enforced for the correct year, there are hundreds of errors which is painful. We could of course just sed the list of files to the current year and be done with it, but then we could just as well bump every file in the tree and there are reasons why we don't do that.

Invoking the checking like in this patch is rather expensive, and obviously only works on cloned repos and not source tarballs (which in this day and age really isn't used for active development anyways).

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-checksrc_copyright_3 branch from 48336bb to 2e2098e Nov 23, 2018

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Nov 28, 2018

The good thing about this PR is that it seem to work, the bad part is that this option makes make checksrc really slow. Some unscientific wallclock testing puts the overhead at ~4x, going from ~4.2s to ~18s on my laptop. That's clearly pretty useless for normal development so it would need to be turned off by default IMO. The question is how it should be invoked? Should we have a make checksrs-release target with extended checks like this one to be run before shipping a release (and in Travis of course) to ensure we are up to date? Thoughts?

@danielgustafsson danielgustafsson changed the title from [RFC] checksrc: add COPYRIGHT year check to checksrc: add COPYRIGHT year check Nov 28, 2018

@bagder

This comment has been minimized.

Member

bagder commented Nov 28, 2018

I agree that this heavy impact would be unfortunate to impose on everyone by default. But I would like to come up with a system that allows me personally to have this run in my local dev environment! Possibly we can support this by simply making checksrc support a local file for extra options or something so that I can just put a "lib/.checksrc" file in there specifying the extra option to use in that dir?

I also think we should adjust the travis checksrc job to make sure it runs this copyright check as well.

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-checksrc_copyright_3 branch from 2e2098e to 52b2950 Dec 1, 2018

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Dec 1, 2018

Possibly we can support this by simply making checksrc support a local file for extra options or something so that I can just put a "lib/.checksrc" file in there specifying the extra option to use in that dir?

I like this idea so I've implemented it and pushed a rebase which contains support for enabling extended checks in $dir/.checksrc.

I also think we should adjust the travis checksrc job to make sure it runs this copyright check as well.

Unless I botched it from not really knowing Travis configuration very well, the second commit in this PR should enable it in Travis.

@bagder

bagder approved these changes Dec 2, 2018

I like it! The travis check should probably also be done for src/ and include/curl but this is great start!

@danielgustafsson

This comment has been minimized.

Member

danielgustafsson commented Dec 2, 2018

Thanks for the review! Just realized that I have forgotten to add this to docs/CHECKSRC.md, will do that before pushing.

danielgustafsson added some commits Nov 22, 2018

checksrc: add COPYRIGHTYEAR check
Forgetting to bump the year in the copyright clause when hacking has
been quite common among curl developers, but a traditional checksrc
check isn't a good fit as it would penalize anyone hacking on January
1st (among other things). This adds a more selective COPYRIGHTYEAR
check which intends to only cover the currently hacked on changeset.

The check for updated copyright year is currently not enforced on any
file but only on files edited and/or committed locally. This is due to
the amount of files which aren't updated with their correct copyright
year at the time of their respective commit.

To further avoid running this expensive check for every developer, it
adds a new local override mode for checksrc where a .checksrc file can
be used to turn on extended warnings locally.
travis: enable COPYRIGHTYEAR extended warning
The extended warning for checking incorrect COPYRIGHTYEAR is quite
expensive to run so rather than expecting every developer to do it
we ensure it's turned on locally for Travis.

@danielgustafsson danielgustafsson force-pushed the danielgustafsson:dg-checksrc_copyright_3 branch from 52b2950 to 687033b Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment