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

fix issue #1440, #1472: wrap BasedFileManager for jdk9 #1495

Merged
merged 4 commits into from Oct 25, 2017

Conversation

Projects
None yet
4 participants
@tmurakam
Contributor

tmurakam commented Oct 15, 2017

I've created a workaround to fix the issue #1440, #1472 for IntelliJ + Java9.
It works for me with IntelliJ 2017.3 EAP.

The wrappedManager is proxied with java.lang.reflect.Proxy, so it can't be cast to BaseFileManager.
So I created BaseFileManagerWrapper to wrap it.

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Oct 15, 2017

Collaborator

Thanks for the pull request.

Can you add your name the the AUTHORS file, so we are legally covered?

Also, there is still a TODO in the code. Did you have a look at how the charset is used?

Collaborator

rspilker commented Oct 15, 2017

Thanks for the pull request.

Can you add your name the the AUTHORS file, so we are legally covered?

Also, there is still a TODO in the code. Did you have a look at how the charset is used?

@tmurakam

This comment has been minimized.

Show comment
Hide comment
@tmurakam

tmurakam Oct 15, 2017

Contributor

I've added my name to AUTHORS.

And I updated the TODO code to use default charset of JVM.
In BaseFileManager class, the charset is used to decide charset in decode() method.

Contributor

tmurakam commented Oct 15, 2017

I've added my name to AUTHORS.

And I updated the TODO code to use default charset of JVM.
In BaseFileManager class, the charset is used to decide charset in decode() method.

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Oct 15, 2017

Collaborator

I think we need to get the encoding passed to the compiler. If you can find out how to do that, great. Otherwise I will probably be able to figure it out.

Collaborator

rspilker commented Oct 15, 2017

I think we need to get the encoding passed to the compiler. If you can find out how to do that, great. Otherwise I will probably be able to figure it out.

@tmurakam

This comment has been minimized.

Show comment
Hide comment
@tmurakam

tmurakam Oct 16, 2017

Contributor

I think we need to get the encoding passed to the compiler. If you can find out how to do that, great.

I digged the implementation of IntelliJ...

The wrappedManager is created in org.jetbrains.jps.javac.JavacMain.wrapWithCallDispatcher(), and it proxies JavaFileManager to org.jetbrains.jps.javac.JavacFileManager.

One possible way to get encoding is get OutputFileObject instance using JavaFileManager#getJavaFileForOutput(), and access myEncodingName private field.

Contributor

tmurakam commented Oct 16, 2017

I think we need to get the encoding passed to the compiler. If you can find out how to do that, great.

I digged the implementation of IntelliJ...

The wrappedManager is created in org.jetbrains.jps.javac.JavacMain.wrapWithCallDispatcher(), and it proxies JavaFileManager to org.jetbrains.jps.javac.JavacFileManager.

One possible way to get encoding is get OutputFileObject instance using JavaFileManager#getJavaFileForOutput(), and access myEncodingName private field.

@tmurakam

This comment has been minimized.

Show comment
Hide comment
@tmurakam

tmurakam Oct 16, 2017

Contributor

I successfully get the encoding. The code is 4298d97.
But it is very ugly, and depends intellij internal implementation...

Contributor

tmurakam commented Oct 16, 2017

I successfully get the encoding. The code is 4298d97.
But it is very ugly, and depends intellij internal implementation...

@wrprice

This comment has been minimized.

Show comment
Hide comment
@wrprice

wrprice Oct 17, 2017

Note that I get a Proxy-wrapped manager using Win10/Gradle and has nothing to do w/ IntelliJ. So there needs to be a wrapper solution that does not depend on IntelliJ internals. You can pass null as the charset and it will pick the platform default. I'm not saying that's proper behavior, but it is "legal" per the API.

wrprice commented Oct 17, 2017

Note that I get a Proxy-wrapped manager using Win10/Gradle and has nothing to do w/ IntelliJ. So there needs to be a wrapper solution that does not depend on IntelliJ internals. You can pass null as the charset and it will pick the platform default. I'm not saying that's proper behavior, but it is "legal" per the API.

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Oct 17, 2017

Collaborator

As far as I know, both compilers use JavaC uder the hood, I'll see if it is possible to get to the encoding passed to the compiler.

Collaborator

rspilker commented Oct 17, 2017

As far as I know, both compilers use JavaC uder the hood, I'll see if it is possible to get to the encoding passed to the compiler.

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Oct 17, 2017

Collaborator

Alternatively, and if everything else fails, we can use the default platform encoding unless a lombok specific property is set.

Collaborator

rspilker commented Oct 17, 2017

Alternatively, and if everything else fails, we can use the default platform encoding unless a lombok specific property is set.

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Oct 17, 2017

Collaborator

@wrprice Can you point me to the code that gradle uses to create the wrapper?

Collaborator

rspilker commented Oct 17, 2017

@wrprice Can you point me to the code that gradle uses to create the wrapper?

@wrprice

This comment has been minimized.

Show comment
Hide comment
@wrprice

wrprice Oct 17, 2017

I don't know at this time, but I'll start looking.

Per my comments on #1472 I was coming at this from the other side. I know the end result is essentially the same, but not the cause or its location.

wrprice commented Oct 17, 2017

I don't know at this time, but I'll start looking.

Per my comments on #1472 I was coming at this from the other side. I know the end result is essentially the same, but not the cause or its location.

@wrprice

This comment has been minimized.

Show comment
Hide comment
@wrprice

wrprice Oct 17, 2017

Well, that was quick. A GitHub search for StandardJavaFileManager in the Gradle repository turns up one hit: org.gradle.api.internal.tasks.compile.JdkJavaCompiler.java:64

That leads to two implementation classes in the org.gradle.api.internal.tasks.compile.reflect package.

wrprice commented Oct 17, 2017

Well, that was quick. A GitHub search for StandardJavaFileManager in the Gradle repository turns up one hit: org.gradle.api.internal.tasks.compile.JdkJavaCompiler.java:64

That leads to two implementation classes in the org.gradle.api.internal.tasks.compile.reflect package.

@rspilker

This comment has been minimized.

Show comment
Hide comment
@rspilker

rspilker Oct 17, 2017

Collaborator

Oh, cool. It even has the encoding in it.

Collaborator

rspilker commented Oct 17, 2017

Oh, cool. It even has the encoding in it.

@md-5

This comment has been minimized.

Show comment
Hide comment
@md-5

md-5 Oct 22, 2017

What about #1435, all these issues seem related regressions in .18

md-5 commented Oct 22, 2017

What about #1435, all these issues seem related regressions in .18

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