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

Filter non-kotlin code out of generated sources #263

Merged
merged 11 commits into from Feb 24, 2020

Conversation

justhecuke-zz
Copy link
Contributor

@justhecuke-zz justhecuke-zz commented Jan 29, 2020

Filters out non-JVM generated sources.

Annotation processors (like dagger) can generate non-JVM code (like .dot files) and this can cause an IllegalStateException due to having a non-JVM source file.

Since we cannot expect all annotation processors to only generate JVM source code, and to be generally useful for generic annotation processors, we need to gracefully handle generated non-JVM code rather than having a strict requirement that only JVM code can be generated since that would probably preclude the use of many useful annotation processors.

Also, added a print of errors hit during compilation. Before it would simply error out mysteriously with no error message. Now, we will actually get the error message. It was very helpful in debugging this particular issue.

WORKSPACE Outdated Show resolved Hide resolved
@justhecuke-zz
Copy link
Contributor Author

Any way to get someone to take a quick look? It's a very minor change that unblocks usage of some annotation processors.

Copy link
Member

@jin jin left a comment

Choose a reason for hiding this comment

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

lgtm (cc @cgruber)

WORKSPACE Outdated Show resolved Hide resolved
Copy link
Collaborator

@cgruber cgruber left a comment

Choose a reason for hiding this comment

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

Looks right, but I'd like a quick nod from @restingbull because he's also working on perf improvements, and I want to make sure he's seeing this.

WORKSPACE Outdated Show resolved Hide resolved
Copy link
Collaborator

@cgruber cgruber left a comment

Choose a reason for hiding this comment

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

Also, can you please put in some sort of test for this behavior? I'm concerned that this will get broken quickly with some of the changes we're going to put in.

@justhecuke-zz
Copy link
Contributor Author

Also, can you please put in some sort of test for this behavior? I'm concerned that this will get broken quickly with some of the changes we're going to put in.

I'm in the process of adding tests. It's a bit more involved than I thought it would be.

@justhecuke-zz
Copy link
Contributor Author

Also, can you please put in some sort of test for this behavior? I'm concerned that this will get broken quickly with some of the changes we're going to put in.

I'm in the process of adding tests. It's a bit more involved than I thought it would be.

I added a unit test although I had to change the visibility of some methods to get everything working.

I can also make a simple java plugin that generates a non-jvm file and make a E2E test of the kotlin compilation rules, but I wasn't sure if that was something that would be OK.

@justhecuke-zz
Copy link
Contributor Author

@cgruber @restingbull I think this can be reviewed again.

@justhecuke-zz
Copy link
Contributor Author

@cgruber friendly ping since the last one I gave was late on a friday.

@restingbull
Copy link
Collaborator

@cgruber is unavailable at the moment. I'll squash and merge.

@restingbull restingbull merged commit 526efa9 into bazelbuild:master Feb 24, 2020
@justhecuke-zz
Copy link
Contributor Author

Thanks.

@cgruber
Copy link
Collaborator

cgruber commented Feb 27, 2020

Sorry for the dead air @justhecuke - was travelling. Thanks for picking up the slack @restingbull

cromwellian pushed a commit to cromwellian/rules_kotlin that referenced this pull request Mar 7, 2020
* Filter non-kotlin code out of generated sources
* Single-pass filter and vague android sdk
* Add test for filtering generated source files
@justhecuke-zz justhecuke-zz deleted the jw/filter-non-kotlin branch March 10, 2020 22:48
cgruber added a commit to cgruber/rules_kotlin that referenced this pull request Apr 14, 2020
* upstream/master:
  Fix non-reproducible archives (bazelbuild#304)
  Adds a kt_plugin rule (bazelbuild#308)
  Ensure that KotlionBuilder workers use a clean directory for each compilation. (bazelbuild#298)
  Apply autoformatting to all files. (bazelbuild#302)
  Optional outputs (bazelbuild#291)
  Change plugins to use depsets, as opposed to lists. (bazelbuild#292)
  Add Corbin to the codeowners. (bazelbuild#293)
  Update Protobuf to 3.11.3 (bazelbuild#286)
  Remove tree artifacts (bazelbuild#287)
  Cleanup src tree (bazelbuild#288)
  Update README.md (bazelbuild#285)
  Filter non-kotlin code out of generated sources (bazelbuild#263)
  Update readme so the dev instructions highlight using a local clone (bazelbuild#283)
  Remove third_party checked in jars, and properly pull maven dependencies. (bazelbuild#279)
  Only propagate srcjar if it isn't the default empty jar added in ae70089 to fix bazelbuild/intellij#1616 (bazelbuild#276)
  Allow resources to be in a kotlin directory (bazelbuild#268)
jongerrish added a commit to jongerrish/rules_kotlin that referenced this pull request Apr 16, 2020
* Filter non-kotlin code out of generated sources
* Single-pass filter and vague android sdk
* Add test for filtering generated source files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants