RFR JDK-8173777: Merge javac -Xmodule into javac--patch-module

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR JDK-8173777: Merge javac -Xmodule into javac--patch-module

Jan Lahoda
Hi,

I'd like to ask for a review of a patch that merges the -Xmodule:
functionality into --patch-module. After this patch, the input source
files are matched against modules patched with --patch-module, and are
compiled as-if they were part of the module they are patching. (In the
multi-module mode, patches for more than one module can be compiled, in
the single-module mode, patches for only one module can be compiled.)

Removal of the -Xmodule: option will need adjustments in several
repositories, as tests in multiple repositories are currently using
-Xmodule:, and also jtreg is using the option. The current patch hence
preserves the existing -Xmodule: functionality, although the option is
hidden and no longer announced in help. I think it would be good to push
the javac/langtools change proposed below, and then continue with other
repositories.

The langtools patch is here:
http://cr.openjdk.java.net/~jlahoda/8173777/langtools.00/

(note it also contains changes to jshell and javadoc).

Proposed jdk repository changes are here:
http://cr.openjdk.java.net/~jlahoda/8173777/jdk.00/

How does this look?

Any feedback is welcome.

Thanks,
      Jan
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8173777: Merge javac -Xmodule into javac--patch-module

Jan Lahoda
I've updated the jdk repository webrev to wrap long lines:
http://cr.openjdk.java.net/~jlahoda/8173777/jdk.01/

Jan

On 6.2.2017 16:53, Jan Lahoda wrote:

> Hi,
>
> I'd like to ask for a review of a patch that merges the -Xmodule:
> functionality into --patch-module. After this patch, the input source
> files are matched against modules patched with --patch-module, and are
> compiled as-if they were part of the module they are patching. (In the
> multi-module mode, patches for more than one module can be compiled, in
> the single-module mode, patches for only one module can be compiled.)
>
> Removal of the -Xmodule: option will need adjustments in several
> repositories, as tests in multiple repositories are currently using
> -Xmodule:, and also jtreg is using the option. The current patch hence
> preserves the existing -Xmodule: functionality, although the option is
> hidden and no longer announced in help. I think it would be good to push
> the javac/langtools change proposed below, and then continue with other
> repositories.
>
> The langtools patch is here:
> http://cr.openjdk.java.net/~jlahoda/8173777/langtools.00/
>
> (note it also contains changes to jshell and javadoc).
>
> Proposed jdk repository changes are here:
> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.00/
>
> How does this look?
>
> Any feedback is welcome.
>
> Thanks,
>       Jan
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8173777: Merge javac -Xmodule into javac--patch-module

Alan Bateman


On 06/02/2017 18:41, Jan Lahoda wrote:
> I've updated the jdk repository webrev to wrap long lines:
> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.01/
>
> Jan
This looks okay to me.

-Alan

Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8173777: Merge javac -Xmodule into javac--patch-module

Jan Lahoda
Hi,

Thanks.

I did two small updates to the patches:
-for langtools repository, I added unwrapping to
ClientCodeWrapper.WrappedJavaFileManager.getLocationForModule - the
incoming JavaFileObject is unwrapped, so that we don't pass our wrappers
to the client's code. Direct link:
http://cr.openjdk.java.net/~jlahoda/8173777/langtools.01/src/jdk.compiler/share/classes/com/sun/tools/javac/api/ClientCodeWrapper.java.udiff.html

-for jdk repository, I fixed the:
test/javax/xml/jaxp/common/8035437/run.sh
to use a platform specific path separator. This is based on code from
jdk/test/java/lang/instrument/appendToClassLoaderSearch/CommonSetup.sh
Direct link:
http://cr.openjdk.java.net/~jlahoda/8173777/jdk.02/test/javax/xml/jaxp/common/8035437/run.sh.udiff.html

(Sorry that I missed this before.)

Updated webrevs:
-langtools repository:
http://cr.openjdk.java.net/~jlahoda/8173777/langtools.01/
-jdk repository:
http://cr.openjdk.java.net/~jlahoda/8173777/jdk.02/

Thanks,
    Jan

On 6.2.2017 21:42, Alan Bateman wrote:

>
>
> On 06/02/2017 18:41, Jan Lahoda wrote:
>> I've updated the jdk repository webrev to wrap long lines:
>> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.01/
>>
>> Jan
> This looks okay to me.
>
> -Alan
>
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8173777: Merge javac -Xmodule into javac--patch-module

Mandy Chung

> On Feb 7, 2017, at 10:11 AM, Jan Lahoda <[hidden email]> wrote:
>
> Updated webrevs:
> -langtools repository:
> http://cr.openjdk.java.net/~jlahoda/8173777/langtools.01/

test/tools/jdeps/jdkinternals/RemovedJDKInternals.java change looks okay to me.

> -jdk repository:
> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.02/


Looks okay too.

Mandy
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8173777: Merge javac -Xmodule into javac--patch-module

Jan Lahoda
In reply to this post by Jan Lahoda
Hi,

Based on some (offline) feedback and further testing, I've updated the
langtools patch. The new webrev is here:
http://cr.openjdk.java.net/~jlahoda/8173777/langtools.02/

Delta webrev compared to .01:
http://cr.openjdk.java.net/~jlahoda/8173777/langtools.02/delta/webrev/index.html

The changes in this updated version:
-the previous version didn't allow the same file to be under both
--patch-module and --module-source-path. The updated version allows
this, and the --patch-module will prevail. This is currently only
allowed when both --patch-module and --module-source-path agree on the
name of the module, but this could be trivially removed if desired.

-for incremental compilation, the previous version required that the
class output is also specified in --patch-module. The updated version
will automatically use class output to read classfiles for patch modules
(by virtue of ModuleSymbol.patchOutputLocation).

-the previous version didn't (always) report an error for module-info on
--patch-module in multi-module mode. This should be fixed now, and there
should be an error for module-info on patch location even in
multi-module mode.

I was looking at explicit uses of -Xmodule: in other repositories, and
there are some in the hotspot repository. All these appear to use a
utility from the top-level repository:
test/lib/jdk/test/lib/InMemoryJavaCompiler.java
(there are other tests that use @compile/module in various repositories,
including hotspot, but these need to be coordinated with jtreg). For
now, I propose to tweak the utility to interpret -Xmodule: itself:
http://cr.openjdk.java.net/~jlahoda/8173777/top-level.00/

Eventually, we could stop using -Xmodule: inside this utility and pass
the module name explicitly, e.g. encoded in the className, but for now
this seems acceptable to me.

Feedback is welcome.

Thanks!

Jan


On 7.2.2017 19:11, Jan Lahoda wrote:

> Hi,
>
> Thanks.
>
> I did two small updates to the patches:
> -for langtools repository, I added unwrapping to
> ClientCodeWrapper.WrappedJavaFileManager.getLocationForModule - the
> incoming JavaFileObject is unwrapped, so that we don't pass our wrappers
> to the client's code. Direct link:
> http://cr.openjdk.java.net/~jlahoda/8173777/langtools.01/src/jdk.compiler/share/classes/com/sun/tools/javac/api/ClientCodeWrapper.java.udiff.html
>
>
> -for jdk repository, I fixed the:
> test/javax/xml/jaxp/common/8035437/run.sh
> to use a platform specific path separator. This is based on code from
> jdk/test/java/lang/instrument/appendToClassLoaderSearch/CommonSetup.sh
> Direct link:
> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.02/test/javax/xml/jaxp/common/8035437/run.sh.udiff.html
>
>
> (Sorry that I missed this before.)
>
> Updated webrevs:
> -langtools repository:
> http://cr.openjdk.java.net/~jlahoda/8173777/langtools.01/
> -jdk repository:
> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.02/
>
> Thanks,
>     Jan
>
> On 6.2.2017 21:42, Alan Bateman wrote:
>>
>>
>> On 06/02/2017 18:41, Jan Lahoda wrote:
>>> I've updated the jdk repository webrev to wrap long lines:
>>> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.01/
>>>
>>> Jan
>> This looks okay to me.
>>
>> -Alan
>>
Reply | Threaded
Open this post in threaded view
|

Re: RFR JDK-8173777: Merge javac -Xmodule into javac--patch-module

Robert Field
JShell changes to the memory file manager are unchanged in this update
and so still look good.

-Robert


On 02/08/17 12:46, Jan Lahoda wrote:

> Hi,
>
> Based on some (offline) feedback and further testing, I've updated the
> langtools patch. The new webrev is here:
> http://cr.openjdk.java.net/~jlahoda/8173777/langtools.02/
>
> Delta webrev compared to .01:
> http://cr.openjdk.java.net/~jlahoda/8173777/langtools.02/delta/webrev/index.html 
>
>
> The changes in this updated version:
> -the previous version didn't allow the same file to be under both
> --patch-module and --module-source-path. The updated version allows
> this, and the --patch-module will prevail. This is currently only
> allowed when both --patch-module and --module-source-path agree on the
> name of the module, but this could be trivially removed if desired.
>
> -for incremental compilation, the previous version required that the
> class output is also specified in --patch-module. The updated version
> will automatically use class output to read classfiles for patch
> modules (by virtue of ModuleSymbol.patchOutputLocation).
>
> -the previous version didn't (always) report an error for module-info
> on --patch-module in multi-module mode. This should be fixed now, and
> there should be an error for module-info on patch location even in
> multi-module mode.
>
> I was looking at explicit uses of -Xmodule: in other repositories, and
> there are some in the hotspot repository. All these appear to use a
> utility from the top-level repository:
> test/lib/jdk/test/lib/InMemoryJavaCompiler.java
> (there are other tests that use @compile/module in various
> repositories, including hotspot, but these need to be coordinated with
> jtreg). For now, I propose to tweak the utility to interpret -Xmodule:
> itself:
> http://cr.openjdk.java.net/~jlahoda/8173777/top-level.00/
>
> Eventually, we could stop using -Xmodule: inside this utility and pass
> the module name explicitly, e.g. encoded in the className, but for now
> this seems acceptable to me.
>
> Feedback is welcome.
>
> Thanks!
>
> Jan
>
>
> On 7.2.2017 19:11, Jan Lahoda wrote:
>> Hi,
>>
>> Thanks.
>>
>> I did two small updates to the patches:
>> -for langtools repository, I added unwrapping to
>> ClientCodeWrapper.WrappedJavaFileManager.getLocationForModule - the
>> incoming JavaFileObject is unwrapped, so that we don't pass our wrappers
>> to the client's code. Direct link:
>> http://cr.openjdk.java.net/~jlahoda/8173777/langtools.01/src/jdk.compiler/share/classes/com/sun/tools/javac/api/ClientCodeWrapper.java.udiff.html 
>>
>>
>>
>> -for jdk repository, I fixed the:
>> test/javax/xml/jaxp/common/8035437/run.sh
>> to use a platform specific path separator. This is based on code from
>> jdk/test/java/lang/instrument/appendToClassLoaderSearch/CommonSetup.sh
>> Direct link:
>> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.02/test/javax/xml/jaxp/common/8035437/run.sh.udiff.html 
>>
>>
>>
>> (Sorry that I missed this before.)
>>
>> Updated webrevs:
>> -langtools repository:
>> http://cr.openjdk.java.net/~jlahoda/8173777/langtools.01/
>> -jdk repository:
>> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.02/
>>
>> Thanks,
>>     Jan
>>
>> On 6.2.2017 21:42, Alan Bateman wrote:
>>>
>>>
>>> On 06/02/2017 18:41, Jan Lahoda wrote:
>>>> I've updated the jdk repository webrev to wrap long lines:
>>>> http://cr.openjdk.java.net/~jlahoda/8173777/jdk.01/
>>>>
>>>> Jan
>>> This looks okay to me.
>>>
>>> -Alan
>>>