Update OpenSSL Copyright statements automatically using a bash script #5027

mspncp · 2018-01-07T01:50:54Z

This PR consists of two commits: The first one adds a helper script for updating the years of all OpenSSL copyright statements, the second commit is the result of executing it.

Add util/openssl-update-copyright shell script

Updates the year ranges of all OpenSSL copyright statements such that it ends at the current year. If only a single year was given, that year is converted to a range. Any (c) and (C) signs are preserved.

To update the copyright statements for all source files, run the shell script

 ./util/openssl-update-copyright

with no arguments.

Update all OpenSSL Copyright statements to 2018

Automatically converted by the shell script ./util/openssl-update-copyright

mspncp · 2018-01-07T02:09:37Z

The build failure of the clang Travis job seems to be unrelated:

$ if $make; then echo -e '+\057\057\057 MAKE OK'; else echo -e '+\057\057\057 MAKE FAILED'; false; fi;
make: *** No targets specified and no makefile found.  Stop.
+/// MAKE FAILED
The command "if $make; then echo -e '+/// MAKE OK'; else echo -e '+/// MAKE FAILED'; false; fi;" exited with 1.

https://travis-ci.org/openssl/openssl/jobs/325932772

levitte · 2018-01-07T08:56:54Z

The Travis error is a Travis glitch. I've restarted that particular build, just to make sure

levitte · 2018-01-07T09:03:45Z

This isn't quite the right approach... Files should get their copyright year range updated for sure... when there is an actual change in said file. Changing the whole source in one big swoop isn't the approach we've followed so far.

However, it's true that it's easy to forget to do those updates manually whenever (I just realised I've forgotten in recent changes I've made)... so we should probably have something that reminds us we need to do this, or that possibly does it for us on changed files every time we commit. I haven't looked much into the webhooks possibilities for github, but that could be one way...

mspncp · 2018-01-07T09:48:59Z

This isn't quite the right approach... Files should get their copyright year range updated for sure... when there is an actual change in said file. Changing the whole source in one big swoop isn't the approach we've followed so far.

Maybe you are right about the bulk changes. In this case, feel free to close the PR.

But similar arguments can be applied against your alternative approach of automatically updating the copyright note on a change. It forces unrelated diffs into a patch set and misuses the copyright note as a last-modification-marker. It creates unnecessary "diff noise", reminiscent of the good old $Id$ Markers of RCS/CVS.

So I doubt this alternative would be better. Maybe the best solution would have been to add a 'timeless' end marker such as "Copyright 1995-20xx" wich does not have to be updated. But I don't know whether this form is legally allowed. The last alternative would be to simply leave everything as it is and simply ignore outdated years. ;-)

t-j-h · 2018-01-07T10:02:55Z

Technically changing the copyright date is a change ... so all options are open - but we really should decide (and record) what we actually mean to do for this circumstance.

mspncp · 2018-01-07T10:21:23Z

Personally, I prefer a single separate commit doing a complete update. But it‘s a matter of taste and the OMC is free to choose its preferred method.

mspncp · 2018-01-07T10:26:01Z

I can also offer to just contribute the script and discard the second commit, such that the actual copyright change can be carried out by an OMC member.

levitte · 2018-01-07T11:47:02Z

@t-j-h while I agree with you, it becomes a bit silly to update the year range just because we... update the year range (and looking at it the other way around, to make a change of year range just so we can update the year tange feels equally absurd). Wouldn't you say?

richsalz · 2018-01-07T13:52:20Z

It seems to me that when a file is changed, the copyright should be changed; that should be easy to verify in a commit hook.

IBM's policy was small changes did not justify a copyright change. IBM takes copyright and patents and licenses very seriously and has many lawyers.

levitte · 2018-01-07T13:59:26Z

IBM's policy was small changes did not justify a copyright change.

Which would make sense for us as well for anything that's deemed "legally trivial" (i.e. non-copyrightable)

mspncp · 2018-01-07T14:32:15Z

Sorry for breaking loose such a fundamental discussion. I just thought my script might be useful. I will step back for the moment and await your decision.

richsalz · 2018-01-07T14:48:20Z

Don't apologize!! You brought up something that's important that we haven't thought about.

Thanks for doing this.

mspncp · 2018-01-07T15:41:42Z

that should be easy to verify in a commit hook.

IBM's policy was small changes did not justify a copyright change.

Having a commit hook that updates the license year is easy. But distinguishing a trivial from a non-trivial commit automatically is not so easy.

I assume you have some helper scripts to add these „reviewed-by“ and „merged from“ annotations to the final commits. Maybe one could add support for updating the license there?

FdaSilvaYY · 2018-01-07T16:00:08Z

Suggestion : not trivial when there is more than 3, 5 or 8 line of diff in a file ?

levitte · 2018-01-07T16:06:41Z

But distinguishing a trivial from a non-trivial commit automatically is not so easy.

Wellll, we have our CLA: trivial commit message line, so in our case, it's actually very easy to detect ;-)

I assume you have some helper scripts to add these „reviewed-by“ and „merged from“ annotations to the final commits.

Yes. It's addrev, which relies on gitaddrev
(and we have a git hook on our server that controls that the reviewer lines are valid or that there is a CLA: trivial, for each pushed commit)

Something similar could definitely be done for copyright notices, as you suggest.

levitte · 2018-01-07T16:20:01Z

util/openssl-update-copyright

As you use bashisms, this should be #!/bin/bash

#!/bin/bash is not portable; how about /usr/bin/env bash?

levitte · 2018-01-07T16:24:58Z

util/openssl-update-copyright

Could this be remade so the default is to get file names on the command line, but that --all will give all files? I'm guessing a poor man's implementation could be something like this:

if [ "$1" = "--all" ]; then set -- $(grep -rlE "Copyright ${some_year}.*The OpenSSL Project" "$ossldir") fi for f in "$@"; do

levitte · 2018-01-07T16:28:45Z

I'm thinking that the script itself has value regardless, and would rather have a review and approval of that alone.

levitte · 2018-01-07T16:29:09Z

... which means I'd like to see the bulk copyright change commit removed from this PR ;-)

mspncp · 2018-01-07T16:47:53Z

Ok, agreed. Just give me some hours.

mspncp · 2018-01-07T16:52:46Z

Wellll, we have our CLA: trivial commit message line, so in our case, it's actually very easy to detect ;-)

Nope, things are not that simple. People who already have signed a CLA. don’t add these „CLA: trivial“ markers. (And IMHO they are an ugly workaround)

levitte · 2018-01-07T17:16:40Z

D'oh. You're right, of course...

levitte · 2018-01-07T17:17:51Z

Ok, agreed. Just give me some hours.

No problem.

usage: openssl-update-copyright [-h|--help] [file|directory] ... Updates the year ranges of all OpenSSL copyright statements in the given files or directories. (Directories are traversed recursively.) Only copyright statements containing the string 'The OpenSSL Project' are affected. The copyright time range is adjusted to include the current year. If only a single year was specified, it is replaced by a time range starting at that year and ending at the current year. All '(c)' and '(C)' signs are preserved. Signed-off-by: Dr. Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com>

mspncp · 2018-01-08T00:58:33Z

Removed the bulk copyright update and reworked the script.

Instead of adding an --all option, I added support for recursing directories automatically. So in order to update the copyright statements for all source files, run the shell script

    util/openssl-update-copyright .

from the OpenSSL source directory.

mspncp · 2018-01-08T01:05:58Z

Once again, Travis did not find its Makefile: https://travis-ci.org/openssl/openssl/jobs/326192727

levitte · 2018-01-08T01:40:21Z

util/openssl-update-copyright

+			if [ -f "$arg" ]; then
+				sed -i "s/${search}/${replace}/g" "$arg"
+			elif [ -d "$arg" ]; then
+				find "$arg" -name '.[a-z]*' -prune -o -type f -exec sed -i "s/${search}/${replace}/g" {} +


What's that + at the end?

The plus terminates the -exec command and is an alternative to the semicolon (which is generally quoted as \;) . The difference is that instead of calling one instance of sed for every path argument, the args are gathered at {} in xargs-style. See 'man find'.

So it's almost the same like find .... | xargs, but it doesn't suffer the space-in-path problems.

I'm not used to +, but am used to print0. But since it only matters for our build platforms, I'm okay with this.

Was that an approval, @richsalz? Make it official, then

mspncp · 2018-01-08T07:53:30Z

util/openssl-update-copyright

@@ -0,0 +1,58 @@
+#!/bin/bash
+#
+# Copyright 2015-2018 The OpenSSL Project Authors. All Rights Reserved.


Fun fact: wrong copyright year in the copyright script: It should be 2018 instead of 2015-2018. (Header was copy&pasted from openssl-format-source). Will be fixed.

mspncp · 2018-01-08T12:16:38Z

@levitte will you do the autosquashing when it's going to be merged, or shall I do it right now?

levitte · 2018-01-08T12:24:42Z

I can do it.

However, this needs a second review and approval...

richsalz

sorry, thought i had approved.

levitte · 2018-01-08T13:53:56Z

That's all right ;-)

Merged. b26d696

usage: openssl-update-copyright [-h|--help] [file|directory] ... Updates the year ranges of all OpenSSL copyright statements in the given files or directories. (Directories are traversed recursively.) Only copyright statements containing the string 'The OpenSSL Project' are affected. The copyright time range is adjusted to include the current year. If only a single year was specified, it is replaced by a time range starting at that year and ending at the current year. All '(c)' and '(C)' signs are preserved. Signed-off-by: Dr. Matthias St. Pierre <Matthias.St.Pierre@ncp-e.com> Reviewed-by: Rich Salz <rsalz@openssl.org> Reviewed-by: Richard Levitte <levitte@openssl.org> (Merged from #5027)

mspncp · 2018-01-08T14:12:51Z

Thanks for reviewing!

kaduk · 2018-01-08T17:10:44Z

util/openssl-update-copyright

+			;;
+		*)
+			if [ -f "$arg" ]; then
+				sed -i "s/${search}/${replace}/g" "$arg"


This form of sed -i expression with no extension for a backup file is something of a GNU-ism, and is not necessarily portable to BSD sed.
This is probably not a fatal flaw, since this is expected to be a maintainer-only tool and we can deal with it; I just wanted to note the nonportability.

As far as I googled it, the only way to satisfy GNU and BSD sed is to add the backup extension without a space, like -i.bak, right?

It's a pity that your comments came too late for this PR. If the two portability issues (this one and the shebang line) disturb you then feel free to change them, I won't mind. Since I don't have a BSD system lying around, I can't test it myself.

I think that's right about -i.bak, yes.

But I do not plan to submit a follow-up; so far as I know all the people who might need to run this script have access to a system where it should work without modification.

mspncp mentioned this pull request Jan 7, 2018

Update LICENSE year to 2018. #5010

Closed

1 task

levitte requested changes Jan 7, 2018

View reviewed changes

mspncp force-pushed the pr-openssl-update-copyright branch from bfdab62 to 473650f Compare January 8, 2018 00:52

levitte reviewed Jan 8, 2018

View reviewed changes

mspncp commented Jan 8, 2018

View reviewed changes

fixup! Add util/openssl-update-copyright shell script

ca1f7fd

levitte approved these changes Jan 8, 2018

View reviewed changes

levitte added the approval: review pending This pull request needs review by a committer label Jan 8, 2018

richsalz approved these changes Jan 8, 2018

View reviewed changes

levitte closed this Jan 8, 2018

levitte added branch: master Applies to master branch and removed approval: review pending This pull request needs review by a committer labels Jan 8, 2018

kaduk reviewed Jan 8, 2018

View reviewed changes

mspncp mentioned this pull request Dec 11, 2018

copyright years after switch of license #7864

Closed

Uh oh!

Update OpenSSL Copyright statements automatically using a bash script #5027

Update OpenSSL Copyright statements automatically using a bash script #5027

Uh oh!

Conversation

mspncp commented Jan 7, 2018

Add util/openssl-update-copyright shell script

Update all OpenSSL Copyright statements to 2018

Uh oh!

mspncp commented Jan 7, 2018

Uh oh!

levitte commented Jan 7, 2018

Uh oh!

levitte commented Jan 7, 2018

Uh oh!

mspncp commented Jan 7, 2018

Uh oh!

t-j-h commented Jan 7, 2018

Uh oh!

mspncp commented Jan 7, 2018

Uh oh!

mspncp commented Jan 7, 2018

Uh oh!

levitte commented Jan 7, 2018

Uh oh!

richsalz commented Jan 7, 2018

Uh oh!

levitte commented Jan 7, 2018

Uh oh!

mspncp commented Jan 7, 2018

Uh oh!

richsalz commented Jan 7, 2018

Uh oh!

mspncp commented Jan 7, 2018

Uh oh!

FdaSilvaYY commented Jan 7, 2018

Uh oh!

levitte commented Jan 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Jan 7, 2018

Uh oh!

levitte commented Jan 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mspncp commented Jan 7, 2018

Uh oh!

mspncp commented Jan 7, 2018

Uh oh!

levitte commented Jan 7, 2018

Uh oh!

levitte commented Jan 7, 2018

Uh oh!

mspncp commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mspncp commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mspncp commented Jan 8, 2018

Uh oh!

levitte commented Jan 7, 2018 •

edited

Loading

mspncp commented Jan 8, 2018 •

edited

Loading

mspncp commented Jan 8, 2018 •

edited

Loading