Skip to content

Conversation

@kelebra
Copy link
Contributor

@kelebra kelebra commented Feb 22, 2018

This PR introduces integration with sbt-headers discussed in this PR based on implementation in another PR from akka-http in order to automate population of copyright header in source code.

Main points of changes: project/plugins.sbt, project/CopyrightHeader.scala, build.sbt. All other files are changed due to execution of headerCreate sbt command.

I am no sbt expert however this seemed like a low hanging fruit which could improve contribution experience for others especially the ones who contribute to akka for the first time.

@akka-ci
Copy link

akka-ci commented Feb 22, 2018

Thank you for your pull request! After a quick sanity check one of the team will reply with 'OK TO TEST' to kick off our automated validation on Jenkins. This compiles the project, runs the tests, and checks for things like binary compatibility and source code formatting. When two team members have also manually reviewed and (perhaps after asking for some amendments) accepted your contribution, it should be good to be merged.

For more details about our contributing process, check out CONTRIBUTING.md - and feel free to ask!

# Conflicts:
#	akka-remote/src/main/java/akka/remote/artery/aeron/AeronErrorLog.java
#	akka-testkit-typed/src/main/scala/akka/testkit/typed/javadsl/ExplicitlyTriggeredScheduler.scala
@johanandren
Copy link
Contributor

OK TO TEST

@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Feb 22, 2018
@ktoso
Copy link
Contributor

ktoso commented Feb 22, 2018

Excellent, I'm all for merging it -- perhaps after we merge things for today's release.

Copy link
Contributor

@patriknw patriknw left a comment

Choose a reason for hiding this comment

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

great, finally a smart pattern that preserves the original years

/**
* Copyright (C) 2017-2018 Lightbend Inc. <https://www.lightbend.com>
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

do we have to add these newlines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion that looks consistent with akka-http

Copy link
Contributor

Choose a reason for hiding this comment

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

They're also there in akka-http indeed, but they do look a bit strange to me as well. Do we want them?

@@ -1,3 +1,7 @@
/*
* Copyright (C) 2018 Lightbend Inc. <https://www.lightbend.com>
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

good one

* Written by Doug Lea with assistance from members of JCP JSR-166
* Expert Group and released to the public domain, as explained at
* http://creativecommons.org/publicdomain/zero/1.0/
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Good that it doesn't change that

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have any other headers that have both Lightbend and some other copyright notice? When we copy code from elsewhere (when the license permits) we preserve the original copyright notice.

Copy link
Contributor

Choose a reason for hiding this comment

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

akka-protobuf

Copy link
Contributor

Choose a reason for hiding this comment

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

akka-persistence

@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Feb 22, 2018
@akka-ci
Copy link

akka-ci commented Feb 22, 2018

Test FAILed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Feb 23, 2018
@akka-ci
Copy link

akka-ci commented Feb 23, 2018

Test FAILed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Feb 23, 2018
@kelebra
Copy link
Contributor Author

kelebra commented Feb 23, 2018

Things that changed after last commits:

  • In sync with master.
  • Added header generation on compilation in order to simplify regular development.
  • Added tests into scope. Actually more files are formatted now especially in akka-testkit-typed and other new modules.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Feb 23, 2018
@akka-ci
Copy link

akka-ci commented Feb 23, 2018

Test FAILed.

@akka-ci akka-ci added the needs-attention Indicates a PR validation failure (set by CI infrastructure) label Feb 23, 2018
@akka-ci
Copy link

akka-ci commented Feb 23, 2018

Test FAILed.

@akka-ci akka-ci added the validating PR is currently being validated by Jenkins label Feb 23, 2018
@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR is currently being validated by Jenkins labels Feb 23, 2018
@akka-ci
Copy link

akka-ci commented Feb 23, 2018

Test FAILed.


/**
* Copyright (C) 2009-2018 Lightbend Inc. <https://www.lightbend.com>
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

looks wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably something due to extra space in the beginning of each line. I would actually expect MiMa to format it correctly or complain about it. Anyway this could be also fixed with a bit of enhancing of detection regexp.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Feb 24, 2018
@akka-ci
Copy link

akka-ci commented Feb 24, 2018

Test FAILed.

@akka-ci akka-ci added validating PR is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR is currently being validated by Jenkins labels Feb 24, 2018
@akka-ci
Copy link

akka-ci commented Feb 24, 2018

Test PASSed.

@kelebra
Copy link
Contributor Author

kelebra commented Feb 24, 2018

Looks like all done. Sorry for mess, looks like I actually broke build somehow accidentally changing line endings from LF to CRLF (issues with local setup).

Please let me know if there is anything that should be done on this PR.

@@ -1,3 +1,7 @@
/*
* Copyright (C) 2018 Lightbend Inc. <https://www.lightbend.com>
Copy link
Contributor

@Philippus Philippus Mar 1, 2018

Choose a reason for hiding this comment

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

should this be 2011-2018 in this case and similarly for other files that did not have a copyright header yet? Or do we not care?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the sbt-header configuration just adding 2018 is OK. We could consider manually updating files we know are older, but I'm OK with the header like this as well. AFAIK the year in a copyright header is mostly there for informational purposes, not really legal ones.

Copy link
Contributor

@raboof raboof left a comment

Choose a reason for hiding this comment

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

We might want to remove the extra newlines and look into the problem @ktoso noticed, but in general super nice PR! Looking forward to having it in :)

@kelebra
Copy link
Contributor Author

kelebra commented Mar 2, 2018

Thanks for further steps, I will revisit this PR tomorrow. Probably line endings should not be an issue and will start from there.

@kelebra kelebra closed this Mar 3, 2018
@kelebra kelebra deleted the sbt-header-integration branch March 3, 2018 01:33
@kelebra
Copy link
Contributor Author

kelebra commented Mar 3, 2018

Actually, looks like it is an issue 😢 See this rejected PR for more info, looks like play and others are okay with it. I am trying to somehow hack around it but so far it looks like change cannot be done from plugin user side.

PR closed for now as I am reiterating in the same branch. Let me know if you will change your mind regarding line endings.

@kelebra
Copy link
Contributor Author

kelebra commented Mar 3, 2018

So unfortunately no way around it so far. Root cause of these extra line ending is here. Sorry guys but it is pretty much closed by final.

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

Labels

tested PR that was successfully built and tested by Jenkins

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants