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

8256308: Send arguments to javac server in a config file #1195

Closed
wants to merge 6 commits into from

Conversation

@magicus
Copy link
Member

@magicus magicus commented Nov 12, 2020

Currently, to use the javac server, a horrendously long command line option is created, looking like this: --server:portfile=<path to portfile>:sjavac=<command to launch server>, where the sjavac command has had all spaces replaced by %20. Since Project Jigsaw, the set of module arguments needed is huge to begin with, making this command line incomprehensible after mangling.

Apart from making java command lines hard to read (and copy/paste!) by developers, it also makes it hard for scripts to parse. The upcoming winenv rewrite is dependent on being able to differentiate between path names and other arguments, which is not possible in this mess.

So, instead, let's write it to a file, without any escaping, and just pass the configuration file name to the server.

Note that this will change the behavior of the javac server, but as the source code states this is not a documented or externally supported API no CSR is needed.

I also cleaned up some code in SjavacClient, in particular code relating to the passing of arguments. (We never change poolsize or keepalive when we call it.)


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8256308: Send arguments to javac server in a config file

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1195/head:pull/1195
$ git checkout pull/1195

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 12, 2020

👋 Welcome back ihse! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 12, 2020

@magicus The following labels will be automatically applied to this pull request:

  • build
  • compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@magicus magicus force-pushed the magicus:sjavac-argument-passing branch from 6226a51 to 26e5efd Nov 12, 2020
@magicus magicus changed the title 256308: Send arguments to javac server in a config file 8256308: Send arguments to javac server in a config file Nov 12, 2020
@openjdk openjdk bot added the rfr label Nov 12, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 12, 2020

@magicus magicus marked this pull request as draft Nov 13, 2020
@openjdk openjdk bot removed the rfr label Nov 13, 2020
@magicus magicus marked this pull request as ready for review Nov 13, 2020
@magicus
Copy link
Member Author

@magicus magicus commented Nov 13, 2020

/label add rfr

@openjdk openjdk bot added the rfr label Nov 13, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 13, 2020

@magicus The label rfr is not a valid label. These labels are valid:

  • serviceability
  • hotspot
  • sound
  • hotspot-compiler
  • kulla
  • i18n
  • shenandoah
  • jdk
  • javadoc
  • 2d
  • security
  • swing
  • hotspot-runtime
  • jmx
  • build
  • nio
  • beans
  • core-libs
  • compiler
  • net
  • hotspot-gc
  • hotspot-jfr
  • awt
# fixpath (on Windows) This will be executed by the client, if needed.
$1_JAVAC_SERVER_CMD := $$(JAVA_DETACH) $$($1_JAVA_FLAGS) $$($1_JAVAC)

$1_CONFIG_VARDEPS :=$$($1_JAVAC_PORT_FILE) $$($1_JAVAC_SERVER_CMD)

This comment has been minimized.

@erikj79

erikj79 Nov 13, 2020
Member

Space


$1_CONFIG_VARDEPS :=$$($1_JAVAC_PORT_FILE) $$($1_JAVAC_SERVER_CMD)
$1_CONFIG_VARDEPS_FILE := $$(call DependOnVariable, $1_CONFIG_VARDEPS, \
$$($1_BIN)$$($1_MODULE_SUBDIR)/_the.$1.config_vardeps)

This comment has been minimized.

@erikj79

erikj79 Nov 13, 2020
Member

Indentation 4

else
$1_ECHO_COMMAND := $(ECHO)
endif
$$($1_JAVAC_SERVER_CONFIG): $$($1_CONFIG_VARDEPS_FILE)

This comment has been minimized.

@erikj79

erikj79 Nov 13, 2020
Member

Indentation?

@erikj79
Copy link
Member

@erikj79 erikj79 commented Nov 13, 2020

Looks good in general, just some whitespace issues.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 16, 2020

Mailing list message from Magnus Ihse Bursie on compiler-dev:

I would appreciate if someone from langtools could have a look at this
as well. Thanks!

/Magnus

On 2020-11-12 23:23, Magnus Ihse Bursie wrote:

@openjdk
Copy link

@openjdk openjdk bot commented Nov 16, 2020

@magicus This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256308: Send arguments to javac server in a config file

Reviewed-by: erikj, jfranck

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 192 new commits pushed to the master branch:

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Nov 16, 2020
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

There seems to be much more here than there needs to be.

If the issue is just long command lines, then the obvious/conventional solution would be to use @files, which would be a tiny change to the sjavac source code, to insert a single call to CommandLine.parse to expand any @file arguments on the command line.

So, before reading all the various details in this proposed change, is there any reason why the simple @file solution cannot be used?

@magicus
Copy link
Member Author

@magicus magicus commented Nov 18, 2020

The main issue is not the long command lines. Getting shorter command lines is nice, but more a side-effect.

The real driver here is the awkward encoding of the command line, with spaces replaced by %20. This makes it impossible to parse the --server option to Java and properly replace unix paths with Windows paths, in the way that is needed by the upcoming changes to how we build on Windows.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 23, 2020

Mailing list message from Magnus Ihse Bursie on compiler-dev:

I hope I could adequately answer why @files is not a solution to the
problem I need to solve.

Since this is a blocker for the winenv rewrite, I'd very much like to
have this integrated. And I'd very much like to have a review from
anyone in the lang tools team, and not just Erik's.

Anyone, please?

/Magnus

On 2020-11-16 13:43, Magnus Ihse Bursie wrote:

I would appreciate if someone from langtools could have a look at this
as well. Thanks!

/Magnus

On 2020-11-12 23:23, Magnus Ihse Bursie wrote:

@jbf
jbf approved these changes Nov 24, 2020
}

public static String extractStringOptionLine(String opName, String s, String deflt) {
return extractStringOptionWithDelimiter(opName, s, deflt, '\n').strip();

This comment has been minimized.

@jbf

jbf Nov 24, 2020
Member

Is '\n' going to be problematic due to differences in line endings on various platforms?

This comment has been minimized.

@magicus

magicus Nov 24, 2020
Author Member

The JDK project is limited to unix-style line endings only. No other styles are supported, and any attempt to use such will break the build (often early on).

@magicus
Copy link
Member Author

@magicus magicus commented Nov 24, 2020

/integrate

@openjdk openjdk bot closed this Nov 24, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Nov 24, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Nov 24, 2020

@magicus Since your change was applied there have been 194 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 9e4944f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@magicus magicus deleted the magicus:sjavac-argument-passing branch Nov 24, 2020
openjdk-notifier bot referenced this pull request Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants