On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov <github.com+9004656+YaaZ@openjdk.org> wrote:

...

8267706: bin/idea.sh tries to use cygpath on WSL

Nikita Gubarkov has updated the pull request incrementally with one additional commit since the last revision:

8267706: Break long line in make/ide/idea/jdk/idea.gmk

Marked as reviewed by erikj (Reviewer). ------------- PR: https://git.openjdk.java.net/jdk/pull/4190

On Tue, 1 Jun 2021 22:10:41 GMT, Jonathan Gibbons <jjg@openjdk.org> wrote:

...

Nikita Gubarkov has updated the pull request incrementally with one additional commit since the last revision:

8267706: Break long line in make/ide/idea/jdk/idea.gmk

The fix fails on a Mac, where `realpath` is not available by default.

@jonathan-gibbons this can be fixed with `brew install coreutils`. We probably need to check `realpath` availability in idea.sh and suggest installing `coreutils` if it's not available ------------- PR: https://git.openjdk.java.net/jdk/pull/4190

On Wed, 2 Jun 2021 00:19:56 GMT, Jonathan Gibbons <jjg@openjdk.org> wrote:

...

@jonathan-gibbons this can be fixed with `brew install coreutils`. We probably need to check `realpath` availability in idea.sh and suggest installing `coreutils` if it's not available

@YaaZ I'm aware of the workaround, but the current situation is not acceptable and needs to be fixed. A change to fix functionality on Windows should not adversely affect users on other platforms.

@erikj79 is there precedent for requiring the use of `brew` to install packages?

@jonathan-gibbons sure, but these changes also improve project setup process on all platforms, so realpath is required here because it's needed on MacOS as well, not only Windows Also, `brew` is already required to instal `autoconf`: https://openjdk.java.net/groups/build/doc/building.html#autoconf As `idea.sh` only works when project is configured, running it implies that `brew` is already installed, so asking user to install `coreutils` is not a big deal IMO ------------- PR: https://git.openjdk.java.net/jdk/pull/4190

On Wed, 2 Jun 2021 06:30:11 GMT, Jonathan Gibbons <jjg@openjdk.org> wrote:

...

Nikita Gubarkov has updated the pull request incrementally with one additional commit since the last revision:

8267706: Break long line in make/ide/idea/jdk/idea.gmk

The fix was supposed to be just about disentangling cygwin and WSL on Windows ... not to "improve project setup on all platforms" ... and break project setup on macOS as a result. If nothing else, if you're changing the set of required dependencies, this needs to be documented. And installing `coreutils` seems to bring in a whole lot of unnecessary stuff.

@jonathan-gibbons my bad, I should have created another PR for this Regarding documenting changes in dependencies: AFAIK, IDEA project setup is not documented anywhere (at least "IDE support for Java code" section at https://github.com/openjdk/jdk/blob/master/doc/ide.md is WIP). Earlier `idea.sh` required Ant and you wouldn't know about it until it fails at the end of the script :) So could you please clarify, what do you mean by that? ------------- PR: https://git.openjdk.java.net/jdk/pull/4190

On Wed, 2 Jun 2021 13:24:00 GMT, Erik Joelsson <erikj@openjdk.org> wrote:

...

Nikita Gubarkov has updated the pull request incrementally with one additional commit since the last revision:

8267706: Break long line in make/ide/idea/jdk/idea.gmk

We do not require brew to install packages on mac, the build doc references brew as an easy way to get autoconf. I personally have always built autoconf from source instead (as it's a very simple build that takes a few seconds at most). I agree that adding a new dependency on the build is troublesome. We generally try really hard to avoid depending on tools that aren't default available on all the main build environments. I didn't check this review very closely for such things. I also did say that I wanted people who actually use this script to weigh in, which I didn't see happen before it was integrated.

Looking at the patch again, the first realpath use can easily be worked around by generating the relative path to SPEC_DIR from idea.gmk. In make we have a macro to generate relative paths, see RelativePath in common/Utils.gmk. The second realpath is operating on moduleSrcDirs, also generated in idea.gmk. The same thing can be applied here, the RelativePath macro together with a foreach can be used to export the src dirs as relative paths.

In general, I think we would have been much better off if this script was just written in make from the start, as we have more platform independent tools available there already. Rewriting it isn't trivial though, and requires quite a bit of knowledge both of GNU make as well as our macros, so I'm not asking for this to happen. I do think we should eliminate the use of realpath though.

@erikj79 thank you for the hint about `RelativePath` macro, I will rewrite this logic and get rid of `realpath`. Can I reference the same issue in new PR, or do I need a new one? ------------- PR: https://git.openjdk.java.net/jdk/pull/4190

On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov <github.com+9004656+YaaZ@openjdk.org> wrote:

...

8267706: bin/idea.sh tries to use cygpath on WSL

Nikita Gubarkov has updated the pull request incrementally with one additional commit since the last revision:

8267706: Break long line in make/ide/idea/jdk/idea.gmk

I filed a new issue yesterday, linked to the old one. There should be no need to create another. https://bugs.openjdk.java.net/browse/JDK-8268083 ------------- PR: https://git.openjdk.java.net/jdk/pull/4190