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

Fix copyright year issues #17427

Conversation

bernd-edlinger
Copy link
Member

Fixes: #13765

@bernd-edlinger bernd-edlinger added branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch labels Jan 5, 2022
Comment on lines -26 to +30
"git log -1 --date=format:%Y --format=format:%ad $file 2>/dev/null|"
"git log -1 --date=short --format=format:%cd $file 2>/dev/null|"
or return $YEAR;
my $LINE = <$FH>;
close $FH;
chomp($LINE);
$LINE =~ s/^([0-9]*)-.*/$1/;
Copy link
Member

Choose a reason for hiding this comment

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

@bernd-edlinger can you please explain the change? I.E. do you think it will help the next time?

Copy link
Member Author

Choose a reason for hiding this comment

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

At least some get versions don't understand the --date=format:%Y and frankly I don't find where this
form of the --date option is documented at all.
OTOH --date=short should always work. we just have to parse the YYYY-MM-DD format and extract the year.
When this command is not understood we have a different result which makes the make update change to 2022.
Additionally I think this is better to use the commit date instead of the author date, since the merge may be
in a later year than the original commit.

Copy link
Member Author

Choose a reason for hiding this comment

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

The update of the .pl files is only necessary because of #17398, otherwise that commit would need to be reverted.

Copy link
Member

@t8m t8m Jan 5, 2022

Choose a reason for hiding this comment

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

The update of the .pl files is only necessary because of #17398, otherwise that commit would need to be reverted.

That's actually a question - should it be reverted instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe, yes. I don't have a strong opinion.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you guys agree, I will update the PR to not touch the .pl files, and restore the 2021 copyright in the generated files
instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, the CI failure of 96b5b11 is a bit unexpected.
Apparently some commit dates are changed when the CI re-bases the PR,
but I am not able to reproduce that at home...

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, the CI uses git fetch --depth=1 this fetches no history beyond the merge commit, which is always new.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, the CI uses git fetch --depth=1 this fetches no history beyond the merge commit, which is always new.

That is pretty common. In Jenkins, I force all history and tags mostly to get the tags. Surely there must be a way to change this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, adding with: fetch-depth:0 in the right place fixes it.

@bernd-edlinger bernd-edlinger force-pushed the use_commit_date_for_update branch 3 times, most recently from 8f5752a to 1c1c698 Compare January 6, 2022 08:12
@bernd-edlinger
Copy link
Member Author

I checked that this should work for 3.0 too.
How about using the same mechanism in 1.1.1 ?

@t8m t8m added the approval: done This pull request has the required number of approvals label Jan 6, 2022
@t8m
Copy link
Member

t8m commented Jan 6, 2022

OK for master and 3.0. I do not think we should bother with 1.1.1.

@t8m t8m added the severity: urgent Fixes an urgent issue (exempt from 24h grace period) label Jan 6, 2022
@t8m
Copy link
Member

t8m commented Jan 6, 2022

Also marking as urgent as the CI on the pushes (not on pull requests) is broken currently due to this issue. If you agree, please merge right away.

@bernd-edlinger bernd-edlinger added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Jan 6, 2022
openssl-machine pushed a commit that referenced this pull request Jan 6, 2022
Fixes: #13765

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #17427)
@bernd-edlinger
Copy link
Member Author

OK, agreed.
Merged to master as fd84b9c.
Merged to 3.0 as ce2f4b6.
Thanks!

openssl-machine pushed a commit that referenced this pull request Jan 6, 2022
Fixes: #13765

Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #17427)

(cherry picked from commit fd84b9c)
@DDvO
Copy link
Contributor

DDvO commented Jan 6, 2022

Thanks a lot @bernd-edlinger for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approval: ready to merge The 24 hour grace period has passed, ready to merge branch: master Merge to master branch branch: 3.0 Merge to openssl-3.0 branch severity: urgent Fixes an urgent issue (exempt from 24h grace period)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

We're having a year XYZ problem ;-)
5 participants