Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov 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
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov 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 We require a new issue to be filed. Thank you for looking into fixing this! - PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Wed, 2 Jun 2021 13:24:00 GMT, Erik Joelsson 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
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov 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 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. - PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Wed, 2 Jun 2021 06:30:11 GMT, Jonathan Gibbons 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
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov 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 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. - PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Wed, 2 Jun 2021 00:19:56 GMT, Jonathan Gibbons 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
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Tue, 1 Jun 2021 22:20:25 GMT, Nikita Gubarkov wrote: >> 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 @YaaZ I'm aware of the workaround, but the current situation is not acceptable and needs to be fixed. @erikj79 is there precedent for requiring the use of `brew` to install packages? - PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Tue, 1 Jun 2021 22:10:41 GMT, Jonathan Gibbons 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
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov 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 The fix fails on a Mac, where `realpath` is not available by default. - PR: https://git.openjdk.java.net/jdk/pull/4190
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
On Thu, 27 May 2021 17:45:40 GMT, Nikita Gubarkov 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
Re: RFR: 8267706: bin/idea.sh tries to use cygpath on WSL [v2]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4190/files - new: https://git.openjdk.java.net/jdk/pull/4190/files/e1617757..f8d6c405 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4190&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4190&range=00-01 Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/4190.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4190/head:pull/4190 PR: https://git.openjdk.java.net/jdk/pull/4190
