Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:40:30 GMT, Magnus Ihse Bursie wrote:
>> Now that the javacserver no longer has any ambitions outside being a
>> buildtool customized for the JDK build process, a lot of abstractions and
>> generalizations can be removed.
>>
>> This will allow the actual behavior to be more clearly visible, and will
>> help debugging the issues we are still seeing (most likely race conditions),
>> and to convert the tool to use the ToolProvider API in the future.
>
> Magnus Ihse Bursie has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - Fix typo #2
>
>Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com>
> - Fix typo
>
>Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com>
make/langtools/tools/javacserver/server/Server.java line 69:
> 67: private static LazyInitFileLog errorLog;
> 68:
> 69: public static void main(String... args) {
Nit:
Suggestion:
public static void main(String... args) {
-
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:40:30 GMT, Magnus Ihse Bursie wrote: >> Now that the javacserver no longer has any ambitions outside being a >> buildtool customized for the JDK build process, a lot of abstractions and >> generalizations can be removed. >> >> This will allow the actual behavior to be more clearly visible, and will >> help debugging the issues we are still seeing (most likely race conditions), >> and to convert the tool to use the ToolProvider API in the future. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Fix typo #2 > >Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> > - Fix typo > >Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> Marked as reviewed by cstein (Committer). - PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:32:51 GMT, Magnus Ihse Bursie wrote:
>> make/langtools/tools/javacserver/client/Client.java line 148:
>>
>>> 146: cmd.addAll(Arrays.asList(conf.javaCommand().split(" ")));
>>> 147: // javacserver.server.Server is the server main class
>>> 148: cmd.add("javacserver.server.Server");
>>
>> Did you consider referencing the class name using `Server.class.getName()`?
>
> No, can't say I have. Do you think there is anything to gain by doing that?
> (Apart from possibly getting this right automatically if the class is renamed
> by IDE tools) (I'm not sure it would be a win in readability)
I like to reference class names that way so that it's typed instead of just a
string. If the class changes name, you get a compilation error, and the IDE
would handle refactoring automatically. Not a strong need in this particular
instance, just something I generally tend to do.
-
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
On Tue, 22 Nov 2022 23:15:20 GMT, Erik Joelsson wrote:
> Things like incremental builds, some forced failure conditions etc?
I tested simple incremental builds but I should probably make a bit more
advanced versions where I modify different files to trigger more real-life like
scenarios, including compilation errors. Good point.
> make/langtools/tools/javacserver/client/Client.java line 148:
>
>> 146: cmd.addAll(Arrays.asList(conf.javaCommand().split(" ")));
>> 147: // javacserver.server.Server is the server main class
>> 148: cmd.add("javacserver.server.Server");
>
> Did you consider referencing the class name using `Server.class.getName()`?
No, can't say I have. Do you think there is anything to gain by doing that?
(Apart from possibly getting this right automatically if the class is renamed
by IDE tools) (I'm not sure it would be a win in readability)
-
PR: https://git.openjdk.org/jdk/pull/11298
Re: RFR: 8297444: Refactor the javacserver build tool [v2]
> Now that the javacserver no longer has any ambitions outside being a > buildtool customized for the JDK build process, a lot of abstractions and > generalizations can be removed. > > This will allow the actual behavior to be more clearly visible, and will help > debugging the issues we are still seeing (most likely race conditions), and > to convert the tool to use the ToolProvider API in the future. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Fix typo #2 Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> - Fix typo Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> - Changes: - all: https://git.openjdk.org/jdk/pull/11298/files - new: https://git.openjdk.org/jdk/pull/11298/files/3dd8af2a..2a0e0975 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=11298&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/11298.diff Fetch: git fetch https://git.openjdk.org/jdk pull/11298/head:pull/11298 PR: https://git.openjdk.org/jdk/pull/11298
