| Message ID | 20200729195829.1335082-1-ddstreet@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | configure: actually disable 'git_update' mode with --disable-git-update | expand |
On Wed, Jul 29, 2020 at 03:58:29PM -0400, Dan Streetman wrote: > The --disable-git-update configure param sets git_update=no, but > some later checks only look for the .git dir. This changes the > --enable-git-update to set git_update=yes but also fail if it > does not find a .git dir. Then all the later checks for the .git > dir can just be changed to a check for $git_update = "yes". > > Also update the Makefile to skip the 'git_update' checks if it has > been disabled. > > This is needed because downstream packagers, e.g. Debian, Ubuntu, etc, > also keep the source code in git, but do not want to enable the > 'git_update' mode; with the current code, that's not possible even > if the downstream package specifies --disable-git-update. Lets recap the original intended behaviour 1. User building from master qemu.git or a fork a) git_update=yes (default) - Always sync submodules to required commit b) git_update=no (--disable-git-update) - Never sync submodules, user is responsible for sync - Validate submodules are at correct commit and fail if not. 2. User building from tarball - Never do anything git related at all Your change is removing the validation from 1.b). This is not desirable in general, because if a user has done a git pull and failed to sync the submodules, they are liable to get obscure, hard to diagnose errors later in the build process. This puts a burden on the user and maintainers who have to waste time diagnosing such problems. > Signed-off-by: Dan Streetman <ddstreet@canonical.com> > --- > Note this is a rebased resend of a previous email to qemu-trivial: > https://lists.nongnu.org/archive/html/qemu-trivial/2020-07/msg00180.html NB, I'm removing qemu-trivial, because I don't think this patch qualifies as trivial. > > Makefile | 15 +++++++++------ > configure | 21 +++++++++++++-------- > 2 files changed, 22 insertions(+), 14 deletions(-) > > diff --git a/Makefile b/Makefile > index c2120d8d48..42550ae086 100644 > --- a/Makefile > +++ b/Makefile > @@ -25,6 +25,8 @@ git-submodule-update: > > .PHONY: git-submodule-update > > +# If --disable-git-update specified, skip these git checks > +ifneq (no,$(GIT_UPDATE)) > git_module_status := $(shell \ > cd '$(SRC_PATH)' && \ > GIT="$(GIT)" ./scripts/git-submodule.sh status $(GIT_SUBMODULES); \ > @@ -32,7 +34,12 @@ git_module_status := $(shell \ > ) > > ifeq (1,$(git_module_status)) > -ifeq (no,$(GIT_UPDATE)) > +ifeq (yes,$(GIT_UPDATE)) > +git-submodule-update: > + $(call quiet-command, \ > + (cd $(SRC_PATH) && GIT="$(GIT)" ./scripts/git-submodule.sh update $(GIT_SUBMODULES)), \ > + "GIT","$(GIT_SUBMODULES)") > +else > git-submodule-update: > $(call quiet-command, \ > echo && \ > @@ -41,11 +48,7 @@ git-submodule-update: > echo "from the source directory checkout $(SRC_PATH)" && \ > echo && \ > exit 1) > -else > -git-submodule-update: > - $(call quiet-command, \ > - (cd $(SRC_PATH) && GIT="$(GIT)" ./scripts/git-submodule.sh update $(GIT_SUBMODULES)), \ > - "GIT","$(GIT_SUBMODULES)") > +endif > endif > endif > > diff --git a/configure b/configure > index 2acc4d1465..e7a241e971 100755 > --- a/configure > +++ b/configure > @@ -318,7 +318,7 @@ then > git_submodules="$git_submodules tests/fp/berkeley-testfloat-3" > git_submodules="$git_submodules tests/fp/berkeley-softfloat-3" > else > - git_update=no > + git_update="" > git_submodules="" > > if ! test -f "$source_path/ui/keycodemapdb/README" > @@ -1598,7 +1598,12 @@ for opt do > ;; > --with-git=*) git="$optarg" > ;; > - --enable-git-update) git_update=yes > + --enable-git-update) > + git_update=yes > + if test ! -e "$source_path/.git"; then > + echo "ERROR: cannot --enable-git-update without .git" > + exit 1 > + fi > ;; > --disable-git-update) git_update=no > ;; > @@ -2011,7 +2016,7 @@ fi > # Consult white-list to determine whether to enable werror > # by default. Only enable by default for git builds > if test -z "$werror" ; then > - if test -e "$source_path/.git" && \ > + if test "$git_update" = "yes" && \ > { test "$linux" = "yes" || test "$mingw32" = "yes"; }; then > werror="yes" > else > @@ -4412,10 +4417,10 @@ EOF > fdt=system > else > # have GIT checkout, so activate dtc submodule > - if test -e "${source_path}/.git" ; then > + if test "$git_update" = "yes" ; then > git_submodules="${git_submodules} dtc" > fi > - if test -d "${source_path}/dtc/libfdt" || test -e "${source_path}/.git" ; then > + if test -d "${source_path}/dtc/libfdt" || test "$git_update" = "yes" ; then > fdt=git > mkdir -p dtc > if [ "$pwd_is_source_path" != "y" ] ; then > @@ -5385,7 +5390,7 @@ case "$capstone" in > "" | yes) > if $pkg_config capstone; then > capstone=system > - elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then > + elif test "$git_update" = "yes" ; then > capstone=git > elif test -e "${source_path}/capstone/Makefile" ; then > capstone=internal > @@ -6414,7 +6419,7 @@ case "$slirp" in > "" | yes) > if $pkg_config slirp; then > slirp=system > - elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then > + elif test "$git_update" = "yes" ; then > slirp=git > elif test -e "${source_path}/slirp/Makefile" ; then > slirp=internal > @@ -6776,7 +6781,7 @@ if test "$cpu" = "s390x" ; then > roms="$roms s390-ccw" > # SLOF is required for building the s390-ccw firmware on s390x, > # since it is using the libnet code from SLOF for network booting. > - if test -e "${source_path}/.git" ; then > + if test "$git_update" = "yes" ; then > git_submodules="${git_submodules} roms/SLOF" > fi > fi > -- > 2.25.1 > > Regards, Daniel
On Tue, Sep 22, 2020 at 12:34 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Jul 29, 2020 at 03:58:29PM -0400, Dan Streetman wrote: > > The --disable-git-update configure param sets git_update=no, but > > some later checks only look for the .git dir. This changes the > > --enable-git-update to set git_update=yes but also fail if it > > does not find a .git dir. Then all the later checks for the .git > > dir can just be changed to a check for $git_update = "yes". > > > > Also update the Makefile to skip the 'git_update' checks if it has > > been disabled. > > > > This is needed because downstream packagers, e.g. Debian, Ubuntu, etc, > > also keep the source code in git, but do not want to enable the > > 'git_update' mode; with the current code, that's not possible even > > if the downstream package specifies --disable-git-update. > > Lets recap the original intended behaviour > > 1. User building from master qemu.git or a fork > a) git_update=yes (default) > - Always sync submodules to required commit > > b) git_update=no (--disable-git-update) > - Never sync submodules, user is responsible for sync > - Validate submodules are at correct commit and fail if not. > > 2. User building from tarball > - Never do anything git related at all > > > Your change is removing the validation from 1.b). Would you accept adding a --disable-git-submodules-check so downstream packagers can keep the source in git, but avoid the submodule validation? Or do you have another suggestion for handling this? Michael, Christian, do you have any suggestions or preference for how this should be handled in Debian and Ubuntu? > This is not desirable > in general, because if a user has done a git pull and failed to sync the > submodules, they are liable to get obscure, hard to diagnose errors later > in the build process. This puts a burden on the user and maintainers who > have to waste time diagnosing such problems. Yep, this problem causes the same obscure, hard to diagnose errors later in the build with downstream packages, but only if the .git dir is present, because the build process is subtly changed in that case. > > > > > Signed-off-by: Dan Streetman <ddstreet@canonical.com> > > --- > > Note this is a rebased resend of a previous email to qemu-trivial: > > https://lists.nongnu.org/archive/html/qemu-trivial/2020-07/msg00180.html > > NB, I'm removing qemu-trivial, because I don't think this patch > qualifies as trivial. > > > > > > Makefile | 15 +++++++++------ > > configure | 21 +++++++++++++-------- > > 2 files changed, 22 insertions(+), 14 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index c2120d8d48..42550ae086 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -25,6 +25,8 @@ git-submodule-update: > > > > .PHONY: git-submodule-update > > > > +# If --disable-git-update specified, skip these git checks > > +ifneq (no,$(GIT_UPDATE)) > > git_module_status := $(shell \ > > cd '$(SRC_PATH)' && \ > > GIT="$(GIT)" ./scripts/git-submodule.sh status $(GIT_SUBMODULES); \ > > @@ -32,7 +34,12 @@ git_module_status := $(shell \ > > ) > > > > ifeq (1,$(git_module_status)) > > -ifeq (no,$(GIT_UPDATE)) > > +ifeq (yes,$(GIT_UPDATE)) > > +git-submodule-update: > > + $(call quiet-command, \ > > + (cd $(SRC_PATH) && GIT="$(GIT)" ./scripts/git-submodule.sh update $(GIT_SUBMODULES)), \ > > + "GIT","$(GIT_SUBMODULES)") > > +else > > git-submodule-update: > > $(call quiet-command, \ > > echo && \ > > @@ -41,11 +48,7 @@ git-submodule-update: > > echo "from the source directory checkout $(SRC_PATH)" && \ > > echo && \ > > exit 1) > > -else > > -git-submodule-update: > > - $(call quiet-command, \ > > - (cd $(SRC_PATH) && GIT="$(GIT)" ./scripts/git-submodule.sh update $(GIT_SUBMODULES)), \ > > - "GIT","$(GIT_SUBMODULES)") > > +endif > > endif > > endif > > > > diff --git a/configure b/configure > > index 2acc4d1465..e7a241e971 100755 > > --- a/configure > > +++ b/configure > > @@ -318,7 +318,7 @@ then > > git_submodules="$git_submodules tests/fp/berkeley-testfloat-3" > > git_submodules="$git_submodules tests/fp/berkeley-softfloat-3" > > else > > - git_update=no > > + git_update="" > > git_submodules="" > > > > if ! test -f "$source_path/ui/keycodemapdb/README" > > @@ -1598,7 +1598,12 @@ for opt do > > ;; > > --with-git=*) git="$optarg" > > ;; > > - --enable-git-update) git_update=yes > > + --enable-git-update) > > + git_update=yes > > + if test ! -e "$source_path/.git"; then > > + echo "ERROR: cannot --enable-git-update without .git" > > + exit 1 > > + fi > > ;; > > --disable-git-update) git_update=no > > ;; > > @@ -2011,7 +2016,7 @@ fi > > # Consult white-list to determine whether to enable werror > > # by default. Only enable by default for git builds > > if test -z "$werror" ; then > > - if test -e "$source_path/.git" && \ > > + if test "$git_update" = "yes" && \ > > { test "$linux" = "yes" || test "$mingw32" = "yes"; }; then > > werror="yes" > > else > > @@ -4412,10 +4417,10 @@ EOF > > fdt=system > > else > > # have GIT checkout, so activate dtc submodule > > - if test -e "${source_path}/.git" ; then > > + if test "$git_update" = "yes" ; then > > git_submodules="${git_submodules} dtc" > > fi > > - if test -d "${source_path}/dtc/libfdt" || test -e "${source_path}/.git" ; then > > + if test -d "${source_path}/dtc/libfdt" || test "$git_update" = "yes" ; then > > fdt=git > > mkdir -p dtc > > if [ "$pwd_is_source_path" != "y" ] ; then > > @@ -5385,7 +5390,7 @@ case "$capstone" in > > "" | yes) > > if $pkg_config capstone; then > > capstone=system > > - elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then > > + elif test "$git_update" = "yes" ; then > > capstone=git > > elif test -e "${source_path}/capstone/Makefile" ; then > > capstone=internal > > @@ -6414,7 +6419,7 @@ case "$slirp" in > > "" | yes) > > if $pkg_config slirp; then > > slirp=system > > - elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then > > + elif test "$git_update" = "yes" ; then > > slirp=git > > elif test -e "${source_path}/slirp/Makefile" ; then > > slirp=internal > > @@ -6776,7 +6781,7 @@ if test "$cpu" = "s390x" ; then > > roms="$roms s390-ccw" > > # SLOF is required for building the s390-ccw firmware on s390x, > > # since it is using the libnet code from SLOF for network booting. > > - if test -e "${source_path}/.git" ; then > > + if test "$git_update" = "yes" ; then > > git_submodules="${git_submodules} roms/SLOF" > > fi > > fi > > -- > > 2.25.1 > > > > > > 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 Wed, Sep 30, 2020 at 09:28:54PM -0400, Dan Streetman wrote: > On Tue, Sep 22, 2020 at 12:34 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > On Wed, Jul 29, 2020 at 03:58:29PM -0400, Dan Streetman wrote: > > > The --disable-git-update configure param sets git_update=no, but > > > some later checks only look for the .git dir. This changes the > > > --enable-git-update to set git_update=yes but also fail if it > > > does not find a .git dir. Then all the later checks for the .git > > > dir can just be changed to a check for $git_update = "yes". > > > > > > Also update the Makefile to skip the 'git_update' checks if it has > > > been disabled. > > > > > > This is needed because downstream packagers, e.g. Debian, Ubuntu, etc, > > > also keep the source code in git, but do not want to enable the > > > 'git_update' mode; with the current code, that's not possible even > > > if the downstream package specifies --disable-git-update. > > > > Lets recap the original intended behaviour > > > > 1. User building from master qemu.git or a fork > > a) git_update=yes (default) > > - Always sync submodules to required commit > > > > b) git_update=no (--disable-git-update) > > - Never sync submodules, user is responsible for sync > > - Validate submodules are at correct commit and fail if not. > > > > 2. User building from tarball > > - Never do anything git related at all > > > > > > Your change is removing the validation from 1.b). > > Would you accept adding a --disable-git-submodules-check so downstream > packagers can keep the source in git, but avoid the submodule > validation? It feels like the current option shouldn't be a boolean, rather a tri-state --with-git-submodules=[update|validate|ignore] > Or do you have another suggestion for handling this? Assuming you're just using git for conveniently applying local downstream patches, you don't need the git repo to exist once getting to the build stage. IOW just delete the .git dir after applying patches before running a build. Regards, Daniel
On Fri, 2 Oct 2020 at 14:13, Daniel P. Berrangé <berrange@redhat.com> wrote: > Assuming you're just using git for conveniently applying local > downstream patches, you don't need the git repo to exist once > getting to the build stage. IOW just delete the .git dir after > applying patches before running a build. ...then what do you do if the build fails and you want to edit/update the patch before retrying? "Blow away your .git tree every time you build and reconstitute it somehow later" doesn't seem like a very friendly thing to require... thanks -- PMM
>> Assuming you're just using git for conveniently applying local >> downstream patches, you don't need the git repo to exist once >> getting to the build stage. IOW just delete the .git dir after >> applying patches before running a build. > >...then what do you do if the build fails and you want to >edit/update the patch before retrying? "Blow away your .git >tree every time you build and reconstitute it somehow later" >doesn't seem like a very friendly thing to require... +1. This option is disconnected with sustaining engineering reality IMHO: tons of interactive rebases, adding and dropping patches, re-orderings - so previous existing patches can allow the new ones (or even existing ones) to become clean cherry-picks - in between patch sets being worked on, bisections before continuing all this, etc.
On Fri, Oct 2, 2020 at 9:11 AM Daniel P. Berrangé <berrange@redhat.com> wrote: > > On Wed, Sep 30, 2020 at 09:28:54PM -0400, Dan Streetman wrote: > > On Tue, Sep 22, 2020 at 12:34 PM Daniel P. Berrangé <berrange@redhat.com> wrote: > > > > > > On Wed, Jul 29, 2020 at 03:58:29PM -0400, Dan Streetman wrote: > > > > The --disable-git-update configure param sets git_update=no, but > > > > some later checks only look for the .git dir. This changes the > > > > --enable-git-update to set git_update=yes but also fail if it > > > > does not find a .git dir. Then all the later checks for the .git > > > > dir can just be changed to a check for $git_update = "yes". > > > > > > > > Also update the Makefile to skip the 'git_update' checks if it has > > > > been disabled. > > > > > > > > This is needed because downstream packagers, e.g. Debian, Ubuntu, etc, > > > > also keep the source code in git, but do not want to enable the > > > > 'git_update' mode; with the current code, that's not possible even > > > > if the downstream package specifies --disable-git-update. > > > > > > Lets recap the original intended behaviour > > > > > > 1. User building from master qemu.git or a fork > > > a) git_update=yes (default) > > > - Always sync submodules to required commit > > > > > > b) git_update=no (--disable-git-update) > > > - Never sync submodules, user is responsible for sync > > > - Validate submodules are at correct commit and fail if not. > > > > > > 2. User building from tarball > > > - Never do anything git related at all > > > > > > > > > Your change is removing the validation from 1.b). > > > > Would you accept adding a --disable-git-submodules-check so downstream > > packagers can keep the source in git, but avoid the submodule > > validation? > > It feels like the current option shouldn't be a boolean, rather > a tri-state --with-git-submodules=[update|validate|ignore] > Ok, I updated the patch to do that: https://lists.nongnu.org/archive/html/qemu-devel/2020-10/msg04799.html > > Or do you have another suggestion for handling this? > > Assuming you're just using git for conveniently applying local > downstream patches, you don't need the git repo to exist once > getting to the build stage. IOW just delete the .git dir after > applying patches before running a build. > > > 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 :| >
diff --git a/Makefile b/Makefile index c2120d8d48..42550ae086 100644 --- a/Makefile +++ b/Makefile @@ -25,6 +25,8 @@ git-submodule-update: .PHONY: git-submodule-update +# If --disable-git-update specified, skip these git checks +ifneq (no,$(GIT_UPDATE)) git_module_status := $(shell \ cd '$(SRC_PATH)' && \ GIT="$(GIT)" ./scripts/git-submodule.sh status $(GIT_SUBMODULES); \ @@ -32,7 +34,12 @@ git_module_status := $(shell \ ) ifeq (1,$(git_module_status)) -ifeq (no,$(GIT_UPDATE)) +ifeq (yes,$(GIT_UPDATE)) +git-submodule-update: + $(call quiet-command, \ + (cd $(SRC_PATH) && GIT="$(GIT)" ./scripts/git-submodule.sh update $(GIT_SUBMODULES)), \ + "GIT","$(GIT_SUBMODULES)") +else git-submodule-update: $(call quiet-command, \ echo && \ @@ -41,11 +48,7 @@ git-submodule-update: echo "from the source directory checkout $(SRC_PATH)" && \ echo && \ exit 1) -else -git-submodule-update: - $(call quiet-command, \ - (cd $(SRC_PATH) && GIT="$(GIT)" ./scripts/git-submodule.sh update $(GIT_SUBMODULES)), \ - "GIT","$(GIT_SUBMODULES)") +endif endif endif diff --git a/configure b/configure index 2acc4d1465..e7a241e971 100755 --- a/configure +++ b/configure @@ -318,7 +318,7 @@ then git_submodules="$git_submodules tests/fp/berkeley-testfloat-3" git_submodules="$git_submodules tests/fp/berkeley-softfloat-3" else - git_update=no + git_update="" git_submodules="" if ! test -f "$source_path/ui/keycodemapdb/README" @@ -1598,7 +1598,12 @@ for opt do ;; --with-git=*) git="$optarg" ;; - --enable-git-update) git_update=yes + --enable-git-update) + git_update=yes + if test ! -e "$source_path/.git"; then + echo "ERROR: cannot --enable-git-update without .git" + exit 1 + fi ;; --disable-git-update) git_update=no ;; @@ -2011,7 +2016,7 @@ fi # Consult white-list to determine whether to enable werror # by default. Only enable by default for git builds if test -z "$werror" ; then - if test -e "$source_path/.git" && \ + if test "$git_update" = "yes" && \ { test "$linux" = "yes" || test "$mingw32" = "yes"; }; then werror="yes" else @@ -4412,10 +4417,10 @@ EOF fdt=system else # have GIT checkout, so activate dtc submodule - if test -e "${source_path}/.git" ; then + if test "$git_update" = "yes" ; then git_submodules="${git_submodules} dtc" fi - if test -d "${source_path}/dtc/libfdt" || test -e "${source_path}/.git" ; then + if test -d "${source_path}/dtc/libfdt" || test "$git_update" = "yes" ; then fdt=git mkdir -p dtc if [ "$pwd_is_source_path" != "y" ] ; then @@ -5385,7 +5390,7 @@ case "$capstone" in "" | yes) if $pkg_config capstone; then capstone=system - elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then + elif test "$git_update" = "yes" ; then capstone=git elif test -e "${source_path}/capstone/Makefile" ; then capstone=internal @@ -6414,7 +6419,7 @@ case "$slirp" in "" | yes) if $pkg_config slirp; then slirp=system - elif test -e "${source_path}/.git" && test $git_update = 'yes' ; then + elif test "$git_update" = "yes" ; then slirp=git elif test -e "${source_path}/slirp/Makefile" ; then slirp=internal @@ -6776,7 +6781,7 @@ if test "$cpu" = "s390x" ; then roms="$roms s390-ccw" # SLOF is required for building the s390-ccw firmware on s390x, # since it is using the libnet code from SLOF for network booting. - if test -e "${source_path}/.git" ; then + if test "$git_update" = "yes" ; then git_submodules="${git_submodules} roms/SLOF" fi fi
The --disable-git-update configure param sets git_update=no, but some later checks only look for the .git dir. This changes the --enable-git-update to set git_update=yes but also fail if it does not find a .git dir. Then all the later checks for the .git dir can just be changed to a check for $git_update = "yes". Also update the Makefile to skip the 'git_update' checks if it has been disabled. This is needed because downstream packagers, e.g. Debian, Ubuntu, etc, also keep the source code in git, but do not want to enable the 'git_update' mode; with the current code, that's not possible even if the downstream package specifies --disable-git-update. Signed-off-by: Dan Streetman <ddstreet@canonical.com> --- Note this is a rebased resend of a previous email to qemu-trivial: https://lists.nongnu.org/archive/html/qemu-trivial/2020-07/msg00180.html Makefile | 15 +++++++++------ configure | 21 +++++++++++++-------- 2 files changed, 22 insertions(+), 14 deletions(-)