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

checksrc: repair the copyrightyear check #4549

Closed
wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Nov 1, 2019

I'm not sure why or how, but checksrc's copyrightyear check has stopped working and it seems to be because current git doesn't show the commit date for the commands used. #4547 made me realize.

This patch brings back the warnings for me. Please give it a look @danielgustafsson!

@bagder
Copy link
Member Author

bagder commented Nov 1, 2019

The problem showed up with git version 2.24.0.rc1 (and rc2) on Debian Linux on two of my machines.

Copy link
Member

@danielgustafsson danielgustafsson left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@bagder
Copy link
Member Author

bagder commented Nov 3, 2019

I haven't investigated exactly why, but when fixing this problem and reran make checksrc in the source root (with this warning enabled in a few source directories), it pointed out a large number of copyright range warnings. Several of them being of the kind where the range ends with year X (not 2019) but the file was last modified in year Y.

I will fix these by updating their ranges to - 2019 since we can't backdate the fixes and once I change these files, they will be updated in 2019!

I also think we need to put back the copyright year check in the CI to catch these earlier.

@danielgustafsson
Copy link
Member

I haven't investigated exactly why, but when fixing this problem and reran make checksrc in the source root (with this warning enabled in a few source directories), it pointed out a large number of copyright range warnings. Several of them being of the kind where the range ends with year X (not 2019) but the file was last modified in year Y.

IIRC (with a disclaimer on the c part), the COPYRIGHTYEAR check only runs when explicitly invoked or when in maintainer mode. It seems quite likely that these snuck through on patches which were developed outside of maintainer mode, perhaps trivial PRs which didn't end up being massaged in a maintainer tree where it would've been caught.

I will fix these by updating their ranges to - 2019 since we can't backdate the fixes and once I change these files, they will be updated in 2019!

I guess we could add logic to COPYRIGHTYEAR to cope with fixup commits like this one, but I doubt it's worth the effort.

I also think we need to put back the copyright year check in the CI to catch these earlier.

It was removed as it was "yelling at contributors" too much, causing red builds everywhere. We could shift the responsibility to the committer and not the contributor but it won't fix the red build. Not sure how to best tackle that.

@bagder
Copy link
Member Author

bagder commented Nov 3, 2019

It was removed as it was "yelling at contributors" too much, causing red builds everywhere

Yeah, and that's probably still the case. I should probably be fine with having checksrc fixed since then at least I will get these errors correctly in my local build - and then not add it to the CI builds for that reason.

@bagder
Copy link
Member Author

bagder commented Nov 4, 2019

Now this is really curious. This travis build warnings say the commit year is 2019 while it clearly is 2018 for several files (for example curl_range.c and arpa_telnet.h) and when I debug the checksrc on my host it says 2018... ?

@danielgustafsson
Copy link
Member

Now this is really curious. This travis build warnings say the commit year is 2019 while it clearly is 2018 for several files (for example curl_range.c and arpa_telnet.h) and when I debug the checksrc on my host it says 2018... ?

That's borderline bizarre. Since I assume that Travis isn't screwing around in the .git index, the only suggestion would be that the files somehow was perceived as edited by Git (the check falls back on the current year for any file with modifications). Perhaps a printf debug line which identifies which checksrc check decides on the year?

@mback2k
Copy link
Member

mback2k commented Nov 5, 2019

Are you working on a fresh checkout/clone? Git normally creates files with the current date on checkout and does not care about filesystem timestamps at all.

@danielgustafsson
Copy link
Member

danielgustafsson commented Nov 5, 2019 via email

@jay
Copy link
Member

jay commented Nov 7, 2019

hm
I get

$ git rev-list --max-count=1 --timestamp HEAD -- ./arpa_telnet.h
1539867109 ad547fcf7b3c0191f63396b94c797bfbb4147f62

yet the checksrc debug output shows:

y: ./arpa_telnet.h 2019 (1570699101 622cf7db6bd2d943e67660020b3984e6068f1084)
./arpa_telnet.h:10:25: error: Copyright year out of date, should be 2019, is 2018 (COPYRIGHTYEAR)
  * Copyright (C) 1998 - 2018, Daniel Stenberg, <daniel@haxx.se>, et al.

edit: super weird how all the bad ones seem to be 1570699101 622cf7db6bd2d943e67660020b3984e6068f1084)

@jay jay force-pushed the bagder/checksrc-copyrightyear branch from 3d4a6ac to cba44f5 Compare November 7, 2019 03:59
@jay
Copy link
Member

jay commented Nov 7, 2019

I added a commit and the CI results for it show the same weird nonsense BUT this time with 0dc14b83818f6166d60e939ce2cc1c4806d39c4a, which interestingly is the child of 622cf7db6bd2d943e67660020b3984e6068f1084 (the prior nonsense commit). I still have no idea what's going on.

@jay
Copy link
Member

jay commented Nov 8, 2019

I ran this command which should show the file history in reverse chronological order but instead it just shows one entry:

`git rev-list --max-count=99 --timestamp HEAD -- ./arpa_telnet.h 1>&2`
1570913113 e80b5c801652bdd8aa302345954c3ef8050d039a

12063.1:
y: ./arpa_telnet.h 2019 (1570699101 622cf7db6bd2d943e67660020b3984e6068f1084)
then 12070.1:
y: ./arpa_telnet.h 2019 (1570742415 0dc14b83818f6166d60e939ce2cc1c4806d39c4a)
then 12072.1:
y: ./arpa_telnet.h 2019 (1570911046 02e608f0bb42a7749a3737eea8ab28f87dcd8f46)
then 12076.1:
y: ./arpa_telnet.h 2019 (1570913113 e80b5c801652bdd8aa302345954c3ef8050d039a)

none of this makes any sense. each one of those commits is a direct descendant of the prior one and have nothing to do with that file.

@jay
Copy link
Member

jay commented Nov 8, 2019

I think I understand what's happening. Travis uses a truncated history on clone:

$ git clone --depth=50 https://github.com/curl/curl.git curl/curl

hm:

$ git rev-list --count e80b5c801652bdd8aa302345954c3ef8050d039a..bagder/checksrc-copyrightyear
51

repro:

$ git clone --depth=50 https://github.com/curl/curl.git
Cloning into 'curl'...
remote: Enumerating objects: 3654, done.
remote: Counting objects: 100% (3654/3654), done.
remote: Compressing objects: 100% (2989/2989), done.
remote: Total 3654 (delta 1342), reused 1340 (delta 636), pack-reused 0
Receiving objects: 100% (3654/3654), 3.87 MiB | 3.98 MiB/s, done.
Resolving deltas: 100% (1342/1342), done.
Checking connectivity... done.

$ cd lib

$ git rev-list --max-count=99 --timestamp HEAD -- ./arpa_telnet.h
1570913113 e80b5c801652bdd8aa302345954c3ef8050d039a

It appears if there are files that have not been modified in the truncated history then git returns the first commit it has. git rev-list has a --min-parents= option and if I set it to 1 then I don't get any results back. So that's a workaround unless you guys want to clone the complete history, but given that it's run all the time I don't know that it's necessary to do that as long as the initial copyright years for the old files were fixed.

I put in a commit that does it let's see what happens...

@bagder
Copy link
Member Author

bagder commented Nov 8, 2019

Aha! How silly that it took that long to figure out! Good catch there @jay!

  1. I'm not sure we need to work on this much more since I don't think we want this check done in travis anyway and it works locally if we just don't limit the clone depth
  2. Apparently it is possible to tell travis to not set the limit to 50: git: depth: false

.. because checksrc's copyright year check stopped working.

Ref: #4547

Closes #4549
- Consider a modified file to be committed this year.

- Make the travis CHECKSRC also do COPYRIGHTYEAR scan.

- Ignore 0 parents when getting latest commit date of file.

since in the CI we're dealing with a truncated repo of last 50 commits,
the file's most recent commit may not be available. when this happens
git log and rev-list show the initial commit (ie first commit not to be
truncated) but that's incorrect so ignore it.

Ref: #4547

Closes #4549
@jay jay force-pushed the bagder/checksrc-copyrightyear branch from 99aa1db to 2762efd Compare November 8, 2019 07:35
@jay
Copy link
Member

jay commented Nov 8, 2019

  • I'm not sure we need to work on this much more since I don't think we want this check done in travis anyway and it works locally if we just don't limit the clone depth
  • Apparently it is possible to tell travis to not set the limit to 50: git: depth: false

--min-parents=1 worked in Travis. As long as the change is in the last 50 commits, and since Travis runs every commit, and since you fixed all the current problems, it should catch any future problems. So I think increasing the depth is unnecessary.

I've squashed your commits into 2, if it passes we should be done.

@jay
Copy link
Member

jay commented Nov 8, 2019

Also for future reference if that didn't work my next step would've been to ssh into travis during the build (looks neat).

@bagder
Copy link
Member Author

bagder commented Nov 8, 2019

Right, this fix should make the build on travis happy. But I'm still not convinced we should add this copyright check to the travis build for the same reasons as before: it causes a great deal of (surprise) complaints on PRs for users. My intent was just to make sure that it works on travis and then not push it but only run these checks in my local builds.

@jay
Copy link
Member

jay commented Nov 8, 2019

Ok np I'll leave it to you to wrap up.

@bagder
Copy link
Member Author

bagder commented Nov 8, 2019

I will let the copyrightyear scan be enabled in include/curl and docs/examples for now to see if that's a good idea ...

@bagder bagder closed this in 0bc60d9 Nov 8, 2019
bagder added a commit that referenced this pull request Nov 8, 2019
- Consider a modified file to be committed this year.

- Make the travis CHECKSRC also do COPYRIGHTYEAR scan in examples and
  includes

- Ignore 0 parents when getting latest commit date of file.

since in the CI we're dealing with a truncated repo of last 50 commits,
the file's most recent commit may not be available. when this happens
git log and rev-list show the initial commit (ie first commit not to be
truncated) but that's incorrect so ignore it.

Ref: #4547

Closes #4549

Co-authored-by: Jay Satiro
@bagder bagder deleted the bagder/checksrc-copyrightyear branch November 8, 2019 13:52
@lock lock bot locked as resolved and limited conversation to collaborators Feb 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants