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

Moe sync 12/05/2016 #407

Merged
merged 3 commits into from
Dec 6, 2016
Merged

Moe sync 12/05/2016 #407

merged 3 commits into from
Dec 6, 2016

Conversation

ronshapiro
Copy link
Contributor

No description provided.

@eamonnmcmanus
Copy link
Member

I think we need to figure out why ExtensionTest is failing with this error: Implicitly compiled files were not subject to annotation processing. But only on Java 7, not on Java 6 nor on Java 8.

@ronshapiro
Copy link
Contributor Author

ronshapiro commented Dec 6, 2016

It's failing in openjdk7, but not oraclejdk7 or oraclejdk8... it's bizarre because it's only compiling that one file, and just in that one test.

Do you remember why you added -Xlint:-processing to those three tests? those seem to be the only ones that it's failing for.

Separately, do we care about compiling with jdk7 anymore? Everyone should be using javac8 even if they pass -source 7 -target 7. We dropped the javac7 requirement in Dagger and haven't received any complaints.

@eamonnmcmanus
Copy link
Member

The -Xlint:-processing turns off warnings about annotations that were not consumed by any annotation processor.

We can certainly discuss no longer compiling with jdk7. Do you want to open an issue for that? I'd like to fix the issue at hand if we can, though.

@tbroyer
Copy link
Contributor

tbroyer commented Dec 6, 2016

Everyone should be using javac8 even if they pass -source 7 -target 7.

Nobody should ever “pass -source and -target to javac unless they also use -bootclasspath; and actually javac8 warns you about it (afaik, javac9's -release should finally save us all from having many JDKs installed locally)
This is not only hypothetical/theoretical, it can really break at runtime: see http://developer-blog.cloudbees.com/2014/12/beware-siren-target-call.html and http://www.draconianoverlord.com/2014/04/01/jdk-compatibility.html

Do you remember why you added -Xlint:-processing to those three tests? those seem to be the only ones that it's failing for.

Those tests are checking for compileWithoutWarning (they're the only 3 tests doing so), so without -Xlint:-processing they'd have to check for the warning about annotations that haven't been claimed instead.

I wonder if those errors aren't false positives / flakes. It'd be interesting to know which class / source file was implicitly compiled. I actually wonder if compile-testing shouldn't default to using an empty source path to prevent those kind of implicit compilation to begin with.

@eamonnmcmanus
Copy link
Member

My inclination is to add "-implicit:class" to the tests, which is supposed to silence that particular warning. I agree that it would be interesting to know what the implicitly-compiled file in question is, but I'm not able to reproduce the failure on my own machine so I don't see a straightforward way to find out.

@tbroyer
Copy link
Contributor

tbroyer commented Dec 6, 2016

FWIW, I couldn't reproduce either [1], so it might be related to the version of Maven being used, or the way the build is done on Travis (building once without tests, then rebuilding with the tests but without cleaning; in my projects, I disabled the first run and just live with the Downloading:/Downloaded: lines in the log).

[1] I used Docker to get an OpenJDK 7, as my Ubuntu only has OpenJDK 8:

docker run -it --rm -v "$PWD":/usr/src/mymaven -w /usr/src/mymaven maven:3-jdk-7 mvn -B -U -f build-pom.xml clean  verify --fail-at-end -Dsource.skip=true -Dmaven.javadoc.skip=true

Now, in addition to being able to say immutableListBuilder() for a property of type ImmutableList<T>, you can say fooBuilder() for a property of an arbitrary type that has a builder following certain conventions. In particular, you can do this if the type of foo() is itself an @autovalue class with a builder.

API discussion: https://docs.google.com/document/d/1F-mP_pgLVIfaqm2A24Kt-2bkDkAVBQv88vSzAHZ7NOw/edit# (starting on page 5).

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=140584023
… private.

This constructor was never documented, and its presence means that we can't know whether @autovalue instances are converted back into builders or not. That in turn means that, currently, if we have `Foo foo()` in the @autovalue class and `FooBuilder fooBuilder()` in the @AutoValue.Builder class, then `Foo` must have a `FooBuilder toBuilder()` method, in case a `Foo` ever needs to be converted back into a `FooBuilder`.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=140958480
…around a compiler warning that causes test failures. See discussion in #407.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=141184041
@ronshapiro ronshapiro mentioned this pull request Dec 6, 2016
@ronshapiro
Copy link
Contributor Author

@eamonnmcmanus LGTY?

@eamonnmcmanus eamonnmcmanus merged commit 6dde490 into master Dec 6, 2016
@eamonnmcmanus eamonnmcmanus deleted the moe_sync_12/05/2016 branch December 6, 2016 21:15
@eamonnmcmanus
Copy link
Member

Great, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants