8294549: configure script should detect unsupported path #10477

bondolo · 2022-09-29T00:09:37Z

The OpenJDK build system does not support building when the source code resides on a path that contains a space. This requirement is documented in the build instructions but not enforced by the configure script.

This change adds an explicit checks to the wrapper configure script that fail the build if the source code to be built is located in a directory who's path contains a space character or the build path cannot be determined.

Includes some idiom modernization and shell scripting best practices changes.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8294549: configure script should detect unsupported path

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10477/head:pull/10477
$ git checkout pull/10477

Update a local copy of the PR:
$ git checkout pull/10477
$ git pull https://git.openjdk.org/jdk pull/10477/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 10477

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10477.diff

bridgekeeper · 2022-09-29T00:09:46Z

👋 Welcome back mduigou! 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 · 2022-09-29T00:11:11Z

⚠️ @bondolo a branch with the same name as the source branch for this pull request (master) is present in the target repository. If you eventually integrate this pull request then the branch master in your personal fork will diverge once you sync your personal fork with the upstream repository.

To avoid this situation, create a new branch for your changes and reset the master branch. You can do this by running the following commands in a local repository for your personal fork. Note: you do not have to name the new branch NEW-BRANCH-NAME.

$ git checkout -b NEW-BRANCH-NAME
$ git branch -f master 6f8f28e7566701b195ecc855f3e802cd7145e9aa
$ git push -f origin master

Then proceed to create a new pull request with NEW-BRANCH-NAME as the source branch and close this one.

openjdk · 2022-09-29T00:11:52Z

@bondolo 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.

Reviewed-by: mduigou, tbell

mlbridge · 2022-09-29T00:15:49Z

Webrevs

01: Full - Incremental (85ae7cc0)
00: Full (6197cf42)

vidmik · 2022-09-29T00:50:42Z

configure

@@ -26,8 +26,29 @@
 # make sure that is called using bash.

 # Get an absolute path to this script, since that determines the top-level directory.
-this_script_dir=`dirname $0`
-this_script_dir=`cd $this_script_dir > /dev/null && pwd`
+source_path="${BASH_SOURCE[0]:-$0}"


Can we (do we want to) rely the shell running this script itself supporting BASH_SOURCE and/or other potential bash-isms?

It is explicitly a bash script so I believe it is OK to use bash-isms. I usually write pure POSIX scripts because of the issues with MacOS compatibility (they use a very old GPLv2 version of BASH) and compatibility with other shells but didn't feel it was an issue here.

This script is just a very thin wrapper of the script in make/autoconf/configure. That script is explicitly called with bash so using bash-isms there (and in the rest of the configure script and build system) is fine. In this wrapper however, I think it's better to stick with compatible shell as much as possible. The shebang in this file isn't really relevant as we can't make files executable. I suspect plenty of users are still running this as sh configure out of habit.

I would recommend moving the space check to make/autoconf/configure instead.

magicus · 2022-10-12T11:38:10Z

I agree that configure should detect and enforce build requirements.

However, this is done in the wrong place. At the very least it should move to make/autoconf/configure as Erik says.

But I think we can do even better. Normally, all checks that requirements are fulfilled are done by the actual autoconf script. We already do some checking on the top-level dir in BASIC_SETUP_PATHS in basic.m4. This would be the proper place to also check for spaces.

Or is it the case that we cannot even start to execute the real autoconf script if there are spaces in the path? If so, I think we should fix those issues by proper quoting. We can't do that over the entire code base, but I think we can afford to do that in the bootstrapping part, so we can actually get to the real configure script.

magicus · 2022-10-12T11:38:53Z

Oh, and hi Mike! :) Long time no see... (Didn't at first understand this was you)

bondolo · 2022-10-31T19:18:01Z

My apologies for delay in responding. I had opted to make these changes to this file because I could isolate the changes to a single file. In order to implement the same logic in the "real" configure file at least some changes would be needed to this file in order to correctly pass a path containing spaces. I will look at moving the path check to the real configure file.

The OpenJDK build system does not support building when the source code resides on a path that contains a space. This requirement is documented in the build instructions but not enforced by the configure script. This change adds an explicit checks to the wrapper `configure` script that fail the build if the source code to be built is located in a directory who's path contains a space character or the build path cannot be determined.

openjdk-notifier · 2022-11-07T00:24:31Z

@bondolo Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

bondolo · 2022-11-07T00:38:23Z

I have removed the path checking from the top level configure script but changes are still required in the configure wrapper to correctly pass a potentially bad path for the real configure script. I considered adding the path checking to basic.m4 in BASIC_SETUP_PATHS but could not understand why UTIL_FIXUP_PATH(TOPDIR) doesn't already reject the bad path like it promises. (I was also reminded just how loathsome autoconf is as a language–all the worst parts of shell with even more cruft and weirdness).

magicus · 2022-11-07T12:27:03Z

configure

+this_script_dir="$(cd -- "${source_path}" > /dev/null && pwd)"
+
+if [ -z "${this_script_dir}" ]; then
+	echo "# Could not determine location of configure script"


Why the initial #?

Just habit. Some style guides for shell commands suggest putting a "#" before error output.

magicus · 2022-11-07T13:30:26Z

Does this patch work for you? I'm getting errors before even launching the autoconf script:

ihse@saturnus:~/testa mellanslag/jdk$ bash configure 
/Users/ihse/testa mellanslag/jdk/make/autoconf/configure: line 179: test: /Users/ihse/testa: binary operator expected
/Users/ihse/testa mellanslag/jdk/make/autoconf/configure: line 165: test: too many arguments
/Users/ihse/testa mellanslag/jdk/make/autoconf/configure: line 165: test: too many arguments
/Users/ihse/testa mellanslag/jdk/make/autoconf/configure: line 165: test: too many arguments
/Users/ihse/testa mellanslag/jdk/make/autoconf/configure: line 165: test: too many arguments
/Users/ihse/testa mellanslag/jdk/make/autoconf/configure: line 334: /Users/ihse/testa: No such file or directory
configure exiting with result code 1

(Also, in the future, please don't rebase once you've opened a PR, it makes it difficult to go back and check older versions of the patch)

magicus · 2022-11-07T13:41:52Z

I think you need to restore you original space-checking part, but put it in make/autoconf/configure. I tried to make this script pass on a path containing spaces to the autoconf configure script, but it was just too much work, for very little gain. Several places assumed space could be used to separate distinct pathes, and that would have forced a complete rewrite of that logic.

With the patch as currently in the PR, and this addition, this works fine for me:

diff --git a/make/autoconf/configure b/make/autoconf/configure
index 4b26e3d7061..e75dc3c7203 100644
--- a/make/autoconf/configure
+++ b/make/autoconf/configure
@@ -1,6 +1,6 @@
 #!/bin/bash
 #
-# Copyright (c) 2012, 2020, Oracle and/or its affiliates. All rights reserved.
+# Copyright (c) 2012, 2022, Oracle and/or its affiliates. All rights reserved.
 # DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
 #
 # This code is free software; you can redistribute it and/or modify it
@@ -39,6 +39,12 @@ if test "x$BASH" = x; then
   echo "Error: This script must be run using bash." 1>&2
   exit 1
 fi
+
+if [[ "$TOPDIR" =~ .*[[:space:]]+.* ]]; then
+  echo "Error: Build path containing space character is not supported" 1>&2
+  exit 1
+fi
+
 # Force autoconf to use bash. This also means we must disable autoconf re-exec.
 export CONFIG_SHELL=$BASH
 export _as_can_reexec=no

magicus · 2022-12-02T13:13:17Z

@bondolo Do you want me to take over this bug, add you as a contributor, and get it committed? Otherwise I think if you incorporate my patch above, you're good to go.

bondolo · 2022-12-03T22:06:01Z

I had gotten stuck at how to regenerate the configure script but based on your suggested edit it appears the configure script is not a generated configure. I will take a look a tomorrow and either accept your suggestion or propose an alternative.

bondolo · 2022-12-03T22:07:45Z

My apologies for the delays.

bridgekeeper · 2022-12-31T23:01:41Z

@bondolo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bondolo · 2022-12-31T23:07:14Z

@magicus I am fine with your solution, please proceed. Thank you for helping get this change over the line and I apologize for the slow followup.

bridgekeeper · 2023-01-28T23:47:54Z

@bondolo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

magicus · 2023-01-29T10:14:49Z

Stayin' alive, bot!

bridgekeeper · 2023-02-26T13:31:30Z

@bondolo This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

bridgekeeper · 2023-03-26T17:37:43Z

@bondolo This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

openjdk bot added the rfr Pull request is ready for review label Sep 29, 2022

openjdk bot added the build build-dev@openjdk.org label Sep 29, 2022

bondolo referenced this pull request Sep 29, 2022

8035495: Improvements in autoconf integration

e00f753

Reviewed-by: mduigou, tbell

vidmik reviewed Sep 29, 2022

View reviewed changes

bondolo force-pushed the master branch from 6197cf4 to 85ae7cc Compare November 7, 2022 00:22

magicus reviewed Nov 7, 2022

View reviewed changes

bridgekeeper bot closed this Mar 26, 2023

magicus mentioned this pull request Nov 23, 2023

8294549: configure script should detect unsupported path #16798

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8294549: configure script should detect unsupported path #10477

8294549: configure script should detect unsupported path #10477

bondolo commented Sep 29, 2022 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 29, 2022

openjdk bot commented Sep 29, 2022

openjdk bot commented Sep 29, 2022

mlbridge bot commented Sep 29, 2022 •

edited

Loading

vidmik Sep 29, 2022

bondolo Sep 29, 2022

erikj79 Oct 3, 2022

magicus commented Oct 12, 2022

magicus commented Oct 12, 2022

bondolo commented Oct 31, 2022

openjdk-notifier bot commented Nov 7, 2022

bondolo commented Nov 7, 2022

magicus Nov 7, 2022

bondolo Dec 3, 2022

magicus commented Nov 7, 2022

magicus commented Nov 7, 2022

magicus commented Dec 2, 2022

bondolo commented Dec 3, 2022

bondolo commented Dec 3, 2022

bridgekeeper bot commented Dec 31, 2022

bondolo commented Dec 31, 2022

bridgekeeper bot commented Jan 28, 2023

magicus commented Jan 29, 2023

bridgekeeper bot commented Feb 26, 2023

bridgekeeper bot commented Mar 26, 2023

8294549: configure script should detect unsupported path #10477

8294549: configure script should detect unsupported path #10477

Conversation

bondolo commented Sep 29, 2022 • edited by openjdk bot Loading

Progress

Issue

Reviewing

bridgekeeper bot commented Sep 29, 2022

openjdk bot commented Sep 29, 2022

openjdk bot commented Sep 29, 2022

mlbridge bot commented Sep 29, 2022 • edited Loading

Webrevs

vidmik Sep 29, 2022

Choose a reason for hiding this comment

bondolo Sep 29, 2022

Choose a reason for hiding this comment

erikj79 Oct 3, 2022

Choose a reason for hiding this comment

magicus commented Oct 12, 2022

magicus commented Oct 12, 2022

bondolo commented Oct 31, 2022

openjdk-notifier bot commented Nov 7, 2022

bondolo commented Nov 7, 2022

magicus Nov 7, 2022

Choose a reason for hiding this comment

bondolo Dec 3, 2022

Choose a reason for hiding this comment

magicus commented Nov 7, 2022

magicus commented Nov 7, 2022

magicus commented Dec 2, 2022

bondolo commented Dec 3, 2022

bondolo commented Dec 3, 2022

bridgekeeper bot commented Dec 31, 2022

bondolo commented Dec 31, 2022

bridgekeeper bot commented Jan 28, 2023

magicus commented Jan 29, 2023

bridgekeeper bot commented Feb 26, 2023

bridgekeeper bot commented Mar 26, 2023

bondolo commented Sep 29, 2022 •

edited by openjdk bot

Loading

mlbridge bot commented Sep 29, 2022 •

edited

Loading