Fixes #1496 The version of asm that buck is using pre-dates Java 9... #1497
Conversation
|
Yeah I would think that |
|
@jkeljo Two issues... First, asm v5.0.3 cannot grok any Java 9 class files, which blows up the ABI calculation code in buck -- in my case when log4j2 v2.9.0 is used. Second, apparently the |
|
@ttsugriy It is absolutely not safe to ignore Presumably, by the time Java 9 goes GA, either asm or log4j will have gotten their act together, at which point the exclusion can be removed. Also presumably, additional buck changes are likely to be required to support Java 9. |
|
It should be noted, we couldn't care less about Java 9 support (we use Java 8). We just wanted to use log4j2 v2.9.0, which apparently contains both Java 7/8 as well as Java 9 classes that end up choking asm 5.0.3. |
|
Glancing at the history for the ASM 6.0 branch it looks to be mostly dealing with modules, so it seems pretty safe to use. If the ABI tests are passing, I'm good with that part. For |
|
I second @jkeljo's suggestion to not put module info work-around in buck's codebase and certainly not without a big all caps warning and follow-up task to fix this as soon as it's properly supported :) |
brettwooldridge
force-pushed
the
master
branch
from
525a7a3
to
51c2a8c
Compare
|
@brettwooldridge has updated the pull request. |
|
@jkeljo BTW, what is the Not to be too critical, but I can now count a handful of times when I've come across undocumented build rule properties mentioned in answers to issues etc. In my opinion, a property should either be supported and documented, or supported and documented as experimental -- or it shouldn't exist. If it exists in the documentation, even if flagged as experimental, the community has an opportunity to provide feedback on its usefulness or behavior. Otherwise, users end up baking undocumented properties into their BUCK files, laying landmines for future developers maintaining those files should the property disappear or the behavior be altered. |
facebook-github-bot
added
GH Review: accepted
Import Started
and removed
GH Review: review-needed
labels
|
@ttsugriy has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Looks like these changes break the build, @brettwooldridge : |
|
@ttsugriy This smells like the same error I got when I was not excluding |
brettwooldridge
force-pushed
the
master
branch
from
51c2a8c
to
d27097b
Compare
|
@brettwooldridge has updated the pull request. |
…ava 9, and chokes on any jar containing Java 9 class files, such as the latest log4j (v2.9.0).
brettwooldridge
force-pushed
the
master
branch
from
d27097b
to
8140fd5
Compare
|
@brettwooldridge has updated the pull request. |
|
@ttsugriy Awaiting the outcome of the automated builds, but I think I've addressed the issue. |
|
@brettwooldridge, it's on As to the documentation thing. I'm not a core Buck team member so don't take the following as authoritative. I agree that we shouldn't be suggesting undocumented features as solutions (at least without filing a task to document them). I think that requiring everything to be supported (with some things marked experimental) would require a change in how Buck is developed, such as using feature branches instead of developing things in master under feature flags or simply undocumented until they're stable, and would probably slow down feature development. |
|
@ttsugriy has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
|
Awesome, thank you for working on this, @brettwooldridge! Java 9 was released yesterday, so I was about to start working on this, but I hope that you nailed it :) I'm kicked off internal builds and if they pass, will merge your changes in. Thanks again! |
|
@ttsugriy Cool. My local test run passed without error (er, at least without related errors). |
|
Looks like one test is failing on Linux: |
|
I was able to reproduce it on my mac with: |
|
Looks like it's a trivial test fix. I'll give it a try and rerun all tests again. |
|
@ttsugriy I reproduced the test failure on my Mac as well. I am assuming it is not transient. However, it may also be the case that the testcase needs to be updated for ASM 6.0/Java 9 behavior. It's 4am here in Tokyo, so I'll take a look over the weekend. If you nail it down or find anything interesting, let me know. |
|
@brettwooldridge, sure, no rush. I fixed the test, but now I'm getting many other errors like: Investigating... |
|
hm, looks like this error happens when attempting to build buck with older version of buck - other integration and end to end tests seem to work well. Will try to land this change by the end of day. |
|
@ttsugriy Yeah, I didn't do that. I always run ... it does look like an additional semicolon is emitted (by ASM6?) and is causing that particular test to fail a literal text match. I suspect that it is safe to simply update the expected text. |
|
yep, @brettwooldridge, the semicolon looks harmless to me. Looks like the issue I pasted above was just an issue with one of the internal jobs we use for static analysis and these changes (+ test fix) are ready to be committed. I'm working on that... :) |
and chokes on any jar containing Java 9 class files, such as the latest log4j (v2.9.0).
This pull request updates the asm dependency, and ignores "module-info.class" files when calculating the ABI. I'm sure these changes will be revisited when Java 9 officially releases.