Re: [Qemu-devel] [PATCH] configure: Remove --source-path option
On Thu, 25 Apr 2019 at 17:42, Antonio Ospite 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
Re: [Qemu-devel] [PATCH] configure: Remove --source-path option
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
---
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
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
---
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=PREFIXuse 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
On Mon, Mar 18, 2019 at 01:40:19PM +, 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 > --- > 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é 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 :|
[Qemu-devel] [PATCH] configure: Remove --source-path option
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
---
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=PREFIXuse PREFIX for compile tools [$cross_prefix]
--cc=CC use C compiler CC [$cc]
--iasl=IASL use ACPI compiler IASL [$iasl]
--
2.20.1
