configure: Remove --source-path option

[Qemu-devel] [PATCH] configure: Remove --source-path option

Posted by Peter Maydell 5 years, 1 month ago

Normally configure identifies the source path by looking
at the location where the configure script itself exists.
We also provide a --source-path option which lets the user
manually override this.

There isn't really an obvious use case for the --source-path
option, and in commit 927128222b0a91f56c13a in 2017 we
accidentally added some logic that looks at $source_path
before the command line option that overrides it has been
processed.

The fact that nobody complained suggests that there isn't
any use of this option and we aren't testing it either;
remove it. This allows us to move the "make $source_path
absolute" logic up so that there is no window in the script
where $source_path is set but not yet absolute.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
Since this is a "noticed while reading code" issue rather than
one that's actually causing a problem, it's probably 4.1
material at this point.

Cc'ing Antonio since they also have a patch to configure
which this will affect.
---
 configure | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/configure b/configure
index 7071f525843..bc2953e6114 100755
--- a/configure
+++ b/configure
@@ -278,6 +278,8 @@ ld_has() {
 
 # default parameters
 source_path=$(dirname "$0")
+# make source path absolute
+source_path=$(cd "$source_path"; pwd)
 cpu=""
 iasl="iasl"
 interp_prefix="/usr/gnemul/qemu-%M"
@@ -519,8 +521,6 @@ for opt do
   ;;
   --cxx=*) CXX="$optarg"
   ;;
-  --source-path=*) source_path="$optarg"
-  ;;
   --cpu=*) cpu="$optarg"
   ;;
   --extra-cflags=*) QEMU_CFLAGS="$QEMU_CFLAGS $optarg"
@@ -599,9 +599,6 @@ if test "$debug_info" = "yes"; then
     LDFLAGS="-g $LDFLAGS"
 fi
 
-# make source path absolute
-source_path=$(cd "$source_path"; pwd)
-
 # running configure in the source tree?
 # we know that's the case if configure is there.
 if test -f "./configure"; then
@@ -945,8 +942,6 @@ for opt do
   ;;
   --interp-prefix=*) interp_prefix="$optarg"
   ;;
-  --source-path=*)
-  ;;
   --cross-prefix=*)
   ;;
   --cc=*)
@@ -1624,7 +1619,6 @@ $(echo Available targets: $default_target_list | \
   fold -s -w 53 | sed -e 's/^/                           /')
 
 Advanced options (experts only):
-  --source-path=PATH       path of source code [$source_path]
   --cross-prefix=PREFIX    use PREFIX for compile tools [$cross_prefix]
   --cc=CC                  use C compiler CC [$cc]
   --iasl=IASL              use ACPI compiler IASL [$iasl]
-- 
2.20.1

Re: [Qemu-devel] [PATCH] configure: Remove --source-path option

Posted by Daniel P. Berrangé 5 years, 1 month ago

On Mon, Mar 18, 2019 at 01:40:19PM +0000, Peter Maydell wrote:
> Normally configure identifies the source path by looking
> at the location where the configure script itself exists.
> We also provide a --source-path option which lets the user
> manually override this.
> 
> There isn't really an obvious use case for the --source-path
> option, and in commit 927128222b0a91f56c13a in 2017 we
> accidentally added some logic that looks at $source_path
> before the command line option that overrides it has been
> processed.
> 
> The fact that nobody complained suggests that there isn't
> any use of this option and we aren't testing it either;
> remove it. This allows us to move the "make $source_path
> absolute" logic up so that there is no window in the script
> where $source_path is set but not yet absolute.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Since this is a "noticed while reading code" issue rather than
> one that's actually causing a problem, it's probably 4.1
> material at this point.

I guess --source-path will still work in 4.0, as long as it is given a
path that points to where 'configure' is.

> Cc'ing Antonio since they also have a patch to configure
> which this will affect.
> ---
>  configure | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH] configure: Remove --source-path option

Posted by Antonio Ospite 5 years, 1 month ago

On 18/03/19 14:40, Peter Maydell wrote:
> Normally configure identifies the source path by looking
> at the location where the configure script itself exists.
> We also provide a --source-path option which lets the user
> manually override this.
> 
> There isn't really an obvious use case for the --source-path
> option, and in commit 927128222b0a91f56c13a in 2017 we
> accidentally added some logic that looks at $source_path
> before the command line option that overrides it has been
> processed.
> 
> The fact that nobody complained suggests that there isn't
> any use of this option and we aren't testing it either;
> remove it. This allows us to move the "make $source_path
> absolute" logic up so that there is no window in the script
> where $source_path is set but not yet absolute.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> Since this is a "noticed while reading code" issue rather than
> one that's actually causing a problem, it's probably 4.1
> material at this point.
> 
> Cc'ing Antonio since they also have a patch to configure
> which this will affect.

Thanks for CC-ing me, I will send a v2 of my patch after this one gets in.

A minor comment below.

> ---
>   configure | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/configure b/configure
> index 7071f525843..bc2953e6114 100755
> --- a/configure
> +++ b/configure
> @@ -278,6 +278,8 @@ ld_has() {
>   
>   # default parameters
>   source_path=$(dirname "$0")
> +# make source path absolute
> +source_path=$(cd "$source_path"; pwd)

While we are at it, can't source_path be set only once?

And probably $(dirname -- "$0") is a little more robust, it covers the 
case of directories starting with a dash, so maybe:

# default parameters
# make source path absolute
source_path=$(cd "$(dirname -- "$0")"; pwd)
...

>   cpu=""
>   iasl="iasl"
>   interp_prefix="/usr/gnemul/qemu-%M"
> @@ -519,8 +521,6 @@ for opt do
>     ;;
>     --cxx=*) CXX="$optarg"
>     ;;
> -  --source-path=*) source_path="$optarg"
> -  ;;
>     --cpu=*) cpu="$optarg"
>     ;;
>     --extra-cflags=*) QEMU_CFLAGS="$QEMU_CFLAGS $optarg"
> @@ -599,9 +599,6 @@ if test "$debug_info" = "yes"; then
>       LDFLAGS="-g $LDFLAGS"
>   fi
>   
> -# make source path absolute
> -source_path=$(cd "$source_path"; pwd)
> -
>   # running configure in the source tree?
>   # we know that's the case if configure is there.
>   if test -f "./configure"; then
> @@ -945,8 +942,6 @@ for opt do
>     ;;
>     --interp-prefix=*) interp_prefix="$optarg"
>     ;;
> -  --source-path=*)
> -  ;;
>     --cross-prefix=*)
>     ;;
>     --cc=*)
> @@ -1624,7 +1619,6 @@ $(echo Available targets: $default_target_list | \
>     fold -s -w 53 | sed -e 's/^/                           /')
>   
>   Advanced options (experts only):
> -  --source-path=PATH       path of source code [$source_path]
>     --cross-prefix=PREFIX    use PREFIX for compile tools [$cross_prefix]
>     --cc=CC                  use C compiler CC [$cc]
>     --iasl=IASL              use ACPI compiler IASL [$iasl]
>

Re: [Qemu-devel] [PATCH] configure: Remove --source-path option

Posted by Antonio Ospite 4 years, 12 months ago

On 19/03/19 09:41, Antonio Ospite wrote:
> On 18/03/19 14:40, Peter Maydell wrote:
>> Normally configure identifies the source path by looking
>> at the location where the configure script itself exists.
>> We also provide a --source-path option which lets the user
>> manually override this.
>>
>> There isn't really an obvious use case for the --source-path
>> option, and in commit 927128222b0a91f56c13a in 2017 we
>> accidentally added some logic that looks at $source_path
>> before the command line option that overrides it has been
>> processed.
>>
>> The fact that nobody complained suggests that there isn't
>> any use of this option and we aren't testing it either;
>> remove it. This allows us to move the "make $source_path
>> absolute" logic up so that there is no window in the script
>> where $source_path is set but not yet absolute.
>>
>> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
>> ---
>> Since this is a "noticed while reading code" issue rather than
>> one that's actually causing a problem, it's probably 4.1
>> material at this point.
>>
>> Cc'ing Antonio since they also have a patch to configure
>> which this will affect.
> 
> Thanks for CC-ing me, I will send a v2 of my patch after this one gets in.
> 
> A minor comment below.
> 
>> ---
>>   configure | 10 ++--------
>>   1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/configure b/configure
>> index 7071f525843..bc2953e6114 100755
>> --- a/configure
>> +++ b/configure
>> @@ -278,6 +278,8 @@ ld_has() {
>>   # default parameters
>>   source_path=$(dirname "$0")
>> +# make source path absolute
>> +source_path=$(cd "$source_path"; pwd)
> 
> While we are at it, can't source_path be set only once?
> 
> And probably $(dirname -- "$0") is a little more robust, it covers the 
> case of directories starting with a dash, so maybe:
> 
> # default parameters
> # make source path absolute
> source_path=$(cd "$(dirname -- "$0")"; pwd)
> ...
>

Ping.

Now that 4.0 has been released, maybe we can move on with this minor change.

I will send a fix for https://bugs.launchpad.net/qemu/+bug/1817345 after 
this patch lands.

Thank you,
    Antonio

>>   cpu=""
>>   iasl="iasl"
>>   interp_prefix="/usr/gnemul/qemu-%M"
>> @@ -519,8 +521,6 @@ for opt do
>>     ;;
>>     --cxx=*) CXX="$optarg"
>>     ;;
>> -  --source-path=*) source_path="$optarg"
>> -  ;;
>>     --cpu=*) cpu="$optarg"
>>     ;;
>>     --extra-cflags=*) QEMU_CFLAGS="$QEMU_CFLAGS $optarg"
>> @@ -599,9 +599,6 @@ if test "$debug_info" = "yes"; then
>>       LDFLAGS="-g $LDFLAGS"
>>   fi
>> -# make source path absolute
>> -source_path=$(cd "$source_path"; pwd)
>> -
>>   # running configure in the source tree?
>>   # we know that's the case if configure is there.
>>   if test -f "./configure"; then
>> @@ -945,8 +942,6 @@ for opt do
>>     ;;
>>     --interp-prefix=*) interp_prefix="$optarg"
>>     ;;
>> -  --source-path=*)
>> -  ;;
>>     --cross-prefix=*)
>>     ;;
>>     --cc=*)
>> @@ -1624,7 +1619,6 @@ $(echo Available targets: $default_target_list | \
>>     fold -s -w 53 | sed -e 's/^/                           /')
>>   Advanced options (experts only):
>> -  --source-path=PATH       path of source code [$source_path]
>>     --cross-prefix=PREFIX    use PREFIX for compile tools [$cross_prefix]
>>     --cc=CC                  use C compiler CC [$cc]
>>     --iasl=IASL              use ACPI compiler IASL [$iasl]
>>
>

Re: [Qemu-devel] [PATCH] configure: Remove --source-path option

Posted by Peter Maydell 4 years, 11 months ago

On Thu, 25 Apr 2019 at 17:42, Antonio Ospite
<antonio.ospite@collabora.com> wrote:
> Now that 4.0 has been released, maybe we can move on with this minor change.
>
> I will send a fix for https://bugs.launchpad.net/qemu/+bug/1817345 after
> this patch lands.

This patch has just gone in to master, so that should be ok
for you to rebase your patch on now.

thanks
-- PMM