8294549: configure script should detect unsupported path

[bondolo]

trafficstars

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)
• [x] Change must not contain extraneous whitespace
• [x] 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[bot]]

:wave: 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[bot]]

:warning: @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[bot]]

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

[mlbridge[bot]]

Webrevs

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

[magicus]

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]

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

[bondolo]

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.

[openjdk-notifier[bot]]

@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]

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]

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]

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]

@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]

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]

My apologies for the delays.

[bridgekeeper[bot]]

@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]

@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[bot]]

@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]

Stayin' alive, bot!

[bridgekeeper[bot]]

@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[bot]]

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