| Message ID | 20190318134019.23729-1-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | configure: Remove --source-path option | expand |
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 :|
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] >
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] >> >
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
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]
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(-) -- 2.20.1