Skip to content

Suppress spurious SDK builds#288

Closed
kennknowles wants to merge 2 commits intoapache:masterfrom
kennknowles:exclude-package-info
Closed

Suppress spurious SDK builds#288
kennknowles wants to merge 2 commits intoapache:masterfrom
kennknowles:exclude-package-info

Conversation

@kennknowles
Copy link
Member

Be sure to do all of the following to help us incorporate your contribution
quickly and easily:

  • Make sure the PR title is formatted like:
    [BEAM-<Jira issue #>] Description of pull request
  • Make sure tests pass via mvn clean verify. (Even better, enable
    Travis-CI on your fork and ensure the whole test matrix passes).
  • Replace <Jira issue #> in the title with the actual Jira issue
    number, if there is one.
  • If this contribution is large, please file an Apache
    Individual Contributor License Agreement.

This is a low tech solution that mostly keeps the status quo but seems to eliminate the extra builds. The current method won't work if the SDK is ever shaded, since it depends on absolute paths in magic strings, but this PR doesn't try to solve that problem.

@kennknowles
Copy link
Member Author

R: @lukecwik

<artifactId>maven-compiler-plugin</artifactId>
<configuration>
<excludes>
<exclude>**/package-info.java</exclude>
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 new excludes section overrides the global one, so I had to repeat here. If you have a maven tip to avoid it, I'm happy to accept.

Copy link
Member

Choose a reason for hiding this comment

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

Not possible, AFAIK.

Copy link
Member Author

Choose a reason for hiding this comment

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

No longer needed. Thanks for the pkginfo tip.

@davorbonaci
Copy link
Member

R: @davorbonaci

May I ask what's the impact here? Those are static, rarely changing files. What would we gain with this?

@kennknowles
Copy link
Member Author

I presume you mean all the package-info.java, since sdk.properties is a nonce, always changing.

Since package-info.java does not produce a .class file, they are viewed as always out-of-date according to maven.

@lukecwik
Copy link
Member

lukecwik commented May 9, 2016

As Kenn said, the compiler always re-runs because it thinks there is a missing class file, alternatively we could tell the java compiler to always produce package-info classes via -Xpkginfo:always, but that is a non-standard option so I think filtering them out is better.

@kennknowles kennknowles force-pushed the exclude-package-info branch from 75ae620 to 481bfcb Compare May 19, 2016 18:44
@kennknowles
Copy link
Member Author

Ping, let's add R: @davorbonaci

This is a nearly no-op change that actually just suppresses a maven bug.

@davorbonaci
Copy link
Member

I think I'd prefer -Xpkginfo:always over the exclusion.

@kennknowles kennknowles force-pushed the exclude-package-info branch 2 times, most recently from 830a75c to c0e45f1 Compare May 19, 2016 20:28
@kennknowles
Copy link
Member Author

Agreed & done.

@kennknowles kennknowles force-pushed the exclude-package-info branch from c0e45f1 to 2d4643d Compare May 19, 2016 20:29
This file is generated exclusively through maven filtering, but
causes spurious rebuilds.
@kennknowles kennknowles force-pushed the exclude-package-info branch from 2d4643d to 31e15ea Compare June 9, 2016 18:07
@kennknowles
Copy link
Member Author

Pretty exciting. Looks like the current status causes a bug in the Java 7 compilers on Linux & Mac :-)

@kennknowles kennknowles force-pushed the exclude-package-info branch from 31e15ea to 8d90dd5 Compare June 10, 2016 02:37
@kennknowles
Copy link
Member Author

Please take another look. This can actually save many minutes for anyone using command-line mvn, which I have to do once in a while.

@lukecwik
Copy link
Member

The travis builds are failing with an exception in the compiler, I don't think we can check this in.

@kennknowles
Copy link
Member Author

Agreed. Really curious.

@kennknowles
Copy link
Member Author

Looks like it is just a total bug, not fixed until Java 8: https://bugs.openjdk.java.net/browse/JDK-8022161

@kennknowles kennknowles force-pushed the exclude-package-info branch from 8d90dd5 to 176bebc Compare June 10, 2016 21:05
@kennknowles
Copy link
Member Author

Looks like -Xpkginfo:nonempty is not going to cut it, so the idea of incremental builds requires littering annotations. I don't feel like doing that, so I'm just going to give up on fixing the build.

@kennknowles kennknowles deleted the exclude-package-info branch November 12, 2016 03:00
iemejia pushed a commit to iemejia/beam that referenced this pull request Jan 12, 2018
pl04351820 pushed a commit to pl04351820/beam that referenced this pull request Dec 20, 2023
Co-authored-by: Christopher Wilcox <crwilcox@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants