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

8258925: configure script failed on WSL #1889

Closed
wants to merge 6 commits into from

Conversation

YaSuenag
Copy link
Member

@YaSuenag YaSuenag commented Dec 24, 2020

I ran configure script on WSL 1, but it failed as below:

$ bash configure --enable-debug --with-boot-jdk=/mnt/d/Java/jdk-15.0.1

    :

configure: Found potential Boot JDK using configure arguments
configure: The command for java_to_test, which resolves as "/mnt/d/Java/jdk-15.0.1/bin/java", can not be found.
configure: error: Cannot locate /mnt/d/Java/jdk-15.0.1/bin/java
configure exiting with result code 1

fixpath.sh attempts to run $PATHTOOL with .exe if it fails with original path, but .exe would not be added even if $path.exe exists.


Progress

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

Issue

Reviewers

Download

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

@bridgekeeper
Copy link

bridgekeeper bot commented Dec 24, 2020

👋 Welcome back ysuenaga! 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 bot commented Dec 24, 2020

@YaSuenag The following label will be automatically applied to this pull request:

  • build

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

@openjdk openjdk bot added the build build-dev@openjdk.org label Dec 24, 2020
@YaSuenag YaSuenag marked this pull request as ready for review December 24, 2020 09:31
@openjdk openjdk bot added the rfr Pull request is ready for review label Dec 24, 2020
@mlbridge
Copy link

mlbridge bot commented Dec 24, 2020

Webrevs

make/scripts/fixpath.sh Outdated Show resolved Hide resolved
@mlbridge
Copy link

mlbridge bot commented Jan 8, 2021

Mailing list message from David Holmes on build-dev:

On 8/01/2021 4:45 pm, Yasumasa Suenaga wrote:

On Fri, 8 Jan 2021 06:23:35 GMT, David Holmes <dholmes at openjdk.org> wrote:

I pushed to new commit to save original `$path` and restore it later if the path not found.
Could you review again?

Sorry but I'm really not following exactly what the problem is and thus the solution. How does setting path affect the running of PATHTOOL? Does the problem indicate we should be using winpath rather than path somewhere?

In this case `$PATHTOOL` points `wslpath`, and it tries to convert `$JAVA_HOME/bin/java` to Windows path. However `java` on Windows is `java.exe`, so we need to convert it.

Currently `fixpath.sh` adds `.exe` if `wslpath` fails, then attempt to do it again, but `$path` still lost extension ( `.exe`). So `configure` will fail on WSL because `java.exe` cannot be find.

Sorry but how does this:

winpath="$($PATHTOOL -w "$path.exe" 2>/dev/null)"

not add the .exe extension?

David

@YaSuenag
Copy link
Member Author

YaSuenag commented Jan 8, 2021

Sorry but how does this:

winpath="$($PATHTOOL -w "$path.exe" 2>/dev/null)"

not add the .exe extension?

Yes, but $path is finally to use, not $winpath.

jdk/make/scripts/fixpath.sh

Lines 165 to 166 in 712014c

# Make it lower case
path="$(echo "$path" | tr [:upper:] [:lower:])"

@mlbridge
Copy link

mlbridge bot commented Jan 8, 2021

Mailing list message from David Holmes on build-dev:

On 8/01/2021 5:36 pm, Yasumasa Suenaga wrote:

On Thu, 24 Dec 2020 08:04:34 GMT, Yasumasa Suenaga <ysuenaga at openjdk.org> wrote:

I ran configure script on WSL 1, but it failed as below:

$ bash configure --enable-debug --with-boot-jdk=/mnt/d/Java/jdk-15.0.1

 \:

configure: Found potential Boot JDK using configure arguments
configure: The command for java_to_test, which resolves as "/mnt/d/Java/jdk-15.0.1/bin/java", can not be found.
configure: error: Cannot locate /mnt/d/Java/jdk-15.0.1/bin/java
configure exiting with result code 1

`fixpath.sh` attempts to run `$PATHTOOL` with `.exe` if it fails with original path, but `.exe` would not be added even if `$path.exe` exists.

Sorry but how does this:

winpath="$($PATHTOOL -w "$path.exe" 2>/dev/null)"

not add the .exe extension?

Yes, but `$path` is finally to use, not `$winpath`.
https://github.com/openjdk/jdk/blob/712014c5956cf74982531d212b03460843e4e5b6/make/scripts/fixpath.sh#L165-L166

Exactly my point. Shouldn't this:

# Make it lower case
path="$(echo "$path" | tr [:upper:] [:lower:])"

be:

# Make it lower case
path="$(echo "$winpath" | tr [:upper:] [:lower:])"

?

David

@YaSuenag
Copy link
Member Author

YaSuenag commented Jan 8, 2021

Sorry but how does this:
winpath="$($PATHTOOL -w "$path.exe" 2>/dev/null)"
not add the .exe extension?

Yes, but $path is finally to use, not $winpath.

jdk/make/scripts/fixpath.sh

Lines 165 to 166 in 712014c

# Make it lower case
path="$(echo "$path" | tr [:upper:] [:lower:])"

Exactly my point. Shouldn't this:

# Make it lower case
path="$(echo "$path" | tr [:upper:] [:lower:])"

be:

# Make it lower case
path="$(echo "$winpath" | tr [:upper:] [:lower:])"

?

No, path points unix path, on the other hand, winpath points windows path.
For example:

  • path
    • /mnt/c/java/jdk-15.0.1/bin/java.exe
  • winpath
    • C:\Java\jdk-15.0.1\bin\java.exe

However I think we can simplify this change based on winpath. How about new change?

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Yasumasa,

Okay I see the problem case now, and the latest fix seems to fix things in a way that makes sense to me now. We still need to wait to see what Magnus or Erik think though.

Thanks,
David

@openjdk
Copy link

openjdk bot commented Jan 8, 2021

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

8258925: configure script failed on WSL

Reviewed-by: dholmes, 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 33 new commits pushed to the master branch:

  • 33fbc10: 8259025: Record compact constructor using Objects.requireNonNull
  • 23801da: 8259482: jni_Set/GetField_probe are the same as their _nh versions
  • 01b2804: 8237578: JDK-8214339 (SSLSocketImpl wraps SocketException) appears to not be fully fixed
  • 1bd015f: 8258407: Split up CompileJavaModules.gmk into make/modules/$M/Java.gmk
  • 2354882: 8250768: javac should be adapted to changes in JEP 12
  • 18a37f9: 8259368: Zero: UseCompressedClassPointers does not depend on UseCompressedOops
  • a03e22b: 8253910: UseCompressedClassPointers depends on UseCompressedOops in vmError.cpp
  • e0d748d: 8258606: os::print_signal_handlers() should resolve the function name of the handlers
  • bd34418: 8258445: Move make/templates to make/data
  • d21a0ea: 8258449: Move make/hotspot/symbols to make/data
  • ... and 23 more: https://git.openjdk.java.net/jdk/compare/712014c5956cf74982531d212b03460843e4e5b6...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 Pull request is ready to be integrated label Jan 8, 2021
@erikj79
Copy link
Member

erikj79 commented Jan 8, 2021

I initially wanted to leave this for Magnus to look at since he wrote all of this, and I know he put a lot of effort into fixpath.sh. It's not a simple script. Now I have stared at it for a while, I think I understand the problem.

One very important aspect of fixpath.sh is to not do heavy work unnecessarily. The latest solution unfortunately adds a call to $PATHTOOL when winpath does not contain forbidden characters. This is a pretty common case and will have performance impact.

The problem is that on line 152, if we find the executable by adding .exe, we need to remember this and use it on line 166, but we cannot modify the variable "path" as it's also used in the else block starting on 168. I would propose introducing a new variable "unixpath" before line 151. Add .exe to it before line 152. Then overwrite it on line 162, and finally use it as the source on line 166.

@YaSuenag
Copy link
Member Author

YaSuenag commented Jan 8, 2021

Thanks @erikj79 for proposing the fix! but I think we should not add variable(s) as possible.

We use $path as a result, and we use $path, $winpath (and $shortpath generated by $winpath) as input for it if fixpath.sh runs on WSL.
The cause of this problem is fixpath.sh modifies $winpath only. If we add new variable, similar issue(s) may happen in future. Fortunately we can convert to unix path from windows path. So it is easy to maintenance if we converge the path to modify to $winpath.

@erikj79
Copy link
Member

erikj79 commented Jan 8, 2021

In this case, I think introducing a variable is well worth it as it means we can eliminate a very common and unnecessary call to $PATHTOOL.

@YaSuenag
Copy link
Member Author

YaSuenag commented Jan 9, 2021

In this case, I think introducing a variable is well worth it as it means we can eliminate a very common and unnecessary call to $PATHTOOL.

Ok, I pushed new commit to use $unixpath instead of calling $PATHTOOL. Could you review again?
@dholmes-ora Are you still ok this change?

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

I think this is right but to be honest the multitude of variables adds to the confusion for me.

David

Comment on lines 154 to 155
unixpath="$unixpath.exe"
winpath="$($PATHTOOL -w "$path.exe" 2>/dev/null)"
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be a little cleaner/clearer to use unixpath on line 155.

Copy link
Member Author

@YaSuenag YaSuenag Jan 9, 2021

Choose a reason for hiding this comment

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

We will check the result from $PATHTOOL at L154, so I think current location is fine.

@mlbridge
Copy link

mlbridge bot commented Jan 9, 2021

Mailing list message from David Holmes on build-dev:

On 9/01/2021 11:33 pm, Yasumasa Suenaga wrote:

On Sat, 9 Jan 2021 09:21:20 GMT, David Holmes <dholmes at openjdk.org> wrote:

Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision:

Refactoring

make/scripts/fixpath.sh line 155:

153: if [[ $? -ne 0 ]]; then
154: unixpath="$unixpath.exe"
155: winpath="$($PATHTOOL -w "$path.exe" 2>/dev/null)"

I think it would be a little cleaner/clearer to use unixpath on line 155.

We will check the result from `$PATHTOOL` at L154, so I think current location is fine.

I'm not at all sure what you mean by the reference to PATHTOOL here, so
to be clear I was suggesting writing line 155 as:

winpath="$($PATHTOOL -w "$unixpath" 2>/dev/null)"

David

@YaSuenag
Copy link
Member Author

YaSuenag commented Jan 9, 2021

I think it would be a little cleaner/clearer to use unixpath on line 155.

We will check the result from $PATHTOOL at L154, so I think current location is fine.

I'm not at all sure what you mean by the reference to PATHTOOL here, so
to be clear I was suggesting writing line 155 as:

winpath="$($PATHTOOL -w "$unixpath" 2>/dev/null)"

Ah, I misunderstood.
I fixed it.

Copy link
Member

@erikj79 erikj79 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks!

@YaSuenag
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Jan 11, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jan 11, 2021
@openjdk
Copy link

openjdk bot commented Jan 11, 2021

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

Your commit was automatically rebased without conflicts.

Pushed as commit 712ea25.

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

@YaSuenag YaSuenag deleted the JDK-8258925 branch January 11, 2021 23:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build build-dev@openjdk.org integrated Pull request has been integrated
3 participants