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

8269761: idea.sh missing .exe suffix when invoking javac on WSL #4656

Closed
wants to merge 5 commits into from

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Jul 1, 2021

From the JBS issue:

At the end, idea.sh tries to invoke javac, but when running on WSL this results in the following error:

bin/idea.sh: line 249: /mnt/c/progra~1/java/jdk-16/bin/javac: No such file or directory

Adding a .exe suffix to the javac path fixes this issue, which can be done just for WSL.


Progress

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

Issue

  • JDK-8269761: idea.sh missing .exe suffix when invoking javac on WSL

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4656/head:pull/4656
$ git checkout pull/4656

Update a local copy of the PR:
$ git checkout pull/4656
$ git pull https://git.openjdk.java.net/jdk pull/4656/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4656

View PR using the GUI difftool:
$ git pr show -t 4656

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4656.diff

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jul 1, 2021

/cc build
/cc ide-support

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 1, 2021

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

@openjdk openjdk bot added rfr build labels Jul 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 1, 2021

@JornVernee
The build label was successfully added.

@openjdk openjdk bot added the ide-support label Jul 1, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 1, 2021

@JornVernee
The ide-support label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 1, 2021

Webrevs

Copy link
Contributor

@mcimadamore mcimadamore left a comment

Nice! Thanks.

@mcimadamore
Copy link
Contributor

@mcimadamore mcimadamore commented Jul 1, 2021

Question though: with WSL, the boot JDK could also be a Linux JDK, no? If that's the case, then the .exe suffix would be bad?

@erikj79
Copy link
Member

@erikj79 erikj79 commented Jul 2, 2021

Question though: with WSL, the boot JDK could also be a Linux JDK, no? If that's the case, then the .exe suffix would be bad?

That's a good point, maybe check if BOOT_JDK/bin/java.exe exists and have that as the condition?

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/4655 to master Jul 5, 2021
@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented Jul 5, 2021

The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout Idea_Cygpath
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk
Copy link

@openjdk openjdk bot commented Jul 5, 2021

@JornVernee 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:

8269761: idea.sh missing .exe suffix when invoking javac on WSL

Reviewed-by: mcimadamore, erikj

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 15 new commits pushed to the master branch:

  • 4dfcf53: 8269935: ProblemList runtime/jni/checked/TestPrimitiveArrayCriticalWithBadParam.java on windows
  • 1578979: 8269917: Insert missing commas in copyrights in java.net
  • 326b2e1: 8253119: Remove the legacy PlainSocketImpl and PlainDatagramSocketImpl implementation
  • f485171: 8269692: sun.net.httpserver.ServerImpl::createContext should throw IAE
  • 16aa8cb: 8269697: JNI_GetPrimitiveArrayCritical() should not accept object array
  • e47803a: 8266310: deadlock between System.loadLibrary and JNI FindClass loading another class
  • 20eba35: 8269882: stack-use-after-scope in NewObjectA
  • df0e11b: 8269672: C1: Remove unaligned move on all architectures
  • 2926769: 8267956: C1 code cleanup
  • acc3d99: 8268860: Windows-Aarch64 build is failing in GitHub actions
  • ... and 5 more: https://git.openjdk.java.net/jdk/compare/76783cd8cbb390dc9ac1da72962ce15e98ea5d3c...master

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 Jul 5, 2021
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jul 5, 2021

I realized this as well, but it doesn't look like the script works when using a linux boot JDK any way currently. The source file that is passed to javac is a Windows path:

JAVAC_SOURCE_FILE=`$CYGPATH -am $IDEA_OUTPUT/src/idea/IdeaLoggerWrapper.java`
...
$BOOT_JDK/bin/$JAVAC$EXE_SUFFIX -d $JAVAC_CLASSES -sourcepath $JAVAC_SOURCE_PATH -cp $JAVAC_CP $JAVAC_SOURCE_FILE

So, I could change the test to look for BOOT_JDK/bin/java.exe, but then the script fails with:

error: file not found: H:/openjdk/git-jdk2/.idea/src/idea/IdeaLoggerWrapper.java

The rest of the script checks for "x$WSL_DISTRO_NAME" != "x" and then uses Windows style paths. So, I'd rather leave the current check for adding the .exe suffix to do the same (consistent with the rest of the script), and leave adding support for running the script with a linux boot JDK on WSL for another time, since that doesn't seem to be a quick fix.

@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jul 5, 2021

Hmm, now that I'm looking again, I see that an earlier block also tries to set JAVAC to javac.exe on WSL, but it never gets executed because CYGPATH is set in my environment:

if [ "x$CYGPATH" != "x" ] ; then ## CYGPATH may be set in env.cfg
  ...
  JAVAC=javac
elif [ "x$WSL_DISTRO_NAME" != "x" ]; then
  ...
  JAVAC=javac.exe
else

Of course, when I remove my fix, and move the "x$WSL_DISTRO_NAME" != "x" to the front, this also fails on a linux boot JDK because javac.exe doesn't exist. If I remove the .exe suffix then the usage of realpath in that same block makes the script work on a linux boot JDK, but fails again on a Windows boot JDK because of the missing .exe suffix.

I'll try to see if I can make the existing logic work on both JDK types

@JornVernee JornVernee marked this pull request as draft Jul 5, 2021
@openjdk openjdk bot removed the rfr label Jul 5, 2021
Copy link
Contributor

@mcimadamore mcimadamore left a comment

Looks good!

@JornVernee JornVernee marked this pull request as ready for review Jul 6, 2021
if [ -e $IDEA_OUTPUT ] ; then
rm -r $IDEA_OUTPUT
fi
Copy link
Member Author

@JornVernee JornVernee Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was getting some weird errors when the .idea folder already existed when running the script, so I added this. It seems useful, so I've left it in.

Copy link
Contributor

@mcimadamore mcimadamore Jul 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - noticed that, it's good

@openjdk openjdk bot added the rfr label Jul 6, 2021
erikj79
erikj79 approved these changes Jul 6, 2021
@JornVernee
Copy link
Member Author

@JornVernee JornVernee commented Jul 7, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jul 7, 2021

Going to push as commit 77a5b7b.
Since your change was applied there have been 33 commits pushed to the master branch:

  • 248aa50: 8269294: Verify_before/after_young_collection should execute all verification
  • 18b80c7: 8269908: Move MemoryService::track_memory_usage call into G1MonitoringScope
  • a685011: 8269022: Put evacuation failure string directly into gc=info log message
  • 72530ef: 8269574: C2: Avoid redundant uncommon traps in GraphKit::builtin_throw() for JVMTI exception events
  • 3d090e7: 8267625: AARCH64: typo in LIR_Assembler::emit_profile_type
  • a9e2010: 8268425: Show decimal nid of OSThread instead of hex format one
  • 01c29d8: 8266407: remove jdk.internal.javac.PreviewFeature.Feature.SEALED_CLASSES
  • 7a4f08a: Merge
  • 0d1cd3a: 8269825: [TESTBUG] Missing testing for x86 KNL platforms
  • e0c130f: 8269955: ProblemList compiler/vectorapi/VectorCastShape[64|128]Test.java tests on x86
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/76783cd8cbb390dc9ac1da72962ce15e98ea5d3c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 7, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jul 7, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 7, 2021

@JornVernee Pushed as commit 77a5b7b.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build ide-support integrated
3 participants