The call to git-submodule.sh done in configure may happen without a
previous checkout of the roms/SLOF submodule, or even without a
previous run of the script.

So, handle creating a .git-submodule-status file even in validate
mode.  If git is absent, ensure that all passed directories exists
(because you should be in a fresh untar and will not have stale
arguments to git-submodule.sh) but do no other checks.  If git
is present, ensure that .git-submodule-status contains an entry
for all submodules passed on the command line.

With this change, "ignore" mode is not needed anymore.

Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
Fixes: b11f9bd96f4 ("configure: move SLOF submodule handling to pc-bios/s390-ccw", 2023-06-06)
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 configure                |  2 +-
 scripts/git-submodule.sh | 72 ++++++++++++++++++++++------------------
 2 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/configure b/configure
index 86363a7e508..2b41c49c0d1 100755
--- a/configure
+++ b/configure
@@ -758,7 +758,7 @@ done
 
 if ! test -e "$source_path/.git"
 then
-    git_submodules_action="ignore"
+    git_submodules_action="validate"
 fi
 
 # test for any invalid configuration combinations
diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
index 11fad2137cd..c33d8fe4cac 100755
--- a/scripts/git-submodule.sh
+++ b/scripts/git-submodule.sh
@@ -9,13 +9,22 @@ command=$1
 shift
 maybe_modules="$@"
 
-# if not running in a git checkout, do nothing
-test "$command" = "ignore" && exit 0
-
+test -z "$maybe_modules" && exit 0
 test -z "$GIT" && GIT=$(command -v git)
 
 cd "$(dirname "$0")/.."
 
+no_git_error=
+if test -n "$maybe_modules" && ! test -e ".git"; then
+    no_git_error='no git checkout exists'
+elif test -n "$maybe_modules" && test -z "$GIT"; then
+    no_git_error='git binary not found'
+fi
+
+is_git() {
+    test -z "$no_git_error"
+}
+
 update_error() {
     echo "$0: $*"
     echo
@@ -34,7 +43,7 @@ update_error() {
 }
 
 validate_error() {
-    if test "$1" = "validate"; then
+    if is_git && test "$1" = "validate"; then
         echo "GIT submodules checkout is out of date, and submodules"
         echo "configured for validate only. Please run"
         echo "  scripts/git-submodule.sh update $maybe_modules"
@@ -51,42 +60,41 @@ check_updated() {
     test "$CURSTATUS" = "$OLDSTATUS"
 }
 
-if test -n "$maybe_modules" && ! test -e ".git"
-then
-    echo "$0: unexpectedly called with submodules but no git checkout exists"
-    exit 1
+if is_git; then
+    test -e $substat || touch $substat
+    modules=""
+    for m in $maybe_modules
+    do
+        $GIT submodule status $m 1> /dev/null 2>&1
+        if test $? = 0
+        then
+            modules="$modules $m"
+            grep $m $substat > /dev/null 2>&1 || $GIT submodule status $module >> $substat
+        else
+            echo "warn: ignoring non-existent submodule $m"
+        fi
+    done
+else
+    modules=$maybe_modules
 fi
 
-if test -n "$maybe_modules" && test -z "$GIT"
-then
-    echo "$0: unexpectedly called with submodules but git binary not found"
-    exit 1
-fi
-
-modules=""
-for m in $maybe_modules
-do
-    $GIT submodule status $m 1> /dev/null 2>&1
-    if test $? = 0
-    then
-        modules="$modules $m"
-    else
-        echo "warn: ignoring non-existent submodule $m"
-    fi
-done
-
 case "$command" in
 status|validate)
-    test -f "$substat" || validate_error "$command"
-    test -z "$maybe_modules" && exit 0
     for module in $modules; do
-        check_updated $module || validate_error "$command"
+        if is_git; then
+            check_updated $module || validate_error "$command"
+        elif ! test -d $module; then
+            echo "$0: sources not available for $module and $no_git_error"
+            validate_error "$command"
+        fi
     done
-    exit 0
     ;;
+
 update)
-    test -e $substat || touch $substat
-    test -z "$maybe_modules" && exit 0
+    is_git || {
+        echo "$0: unexpectedly called with submodules but $no_git_error"
+        exit 1
+    }
 
     $GIT submodule update --init $modules 1>/dev/null
     test $? -ne 0 && update_error "failed to update modules"
-- 
2.40.1

On Sun, 2023-06-18 at 23:20 +0200, Paolo Bonzini wrote:
> The call to git-submodule.sh done in configure may happen without a
> previous checkout of the roms/SLOF submodule, or even without a
> previous run of the script.
> 
> So, handle creating a .git-submodule-status file even in validate
> mode.  If git is absent, ensure that all passed directories exists
> (because you should be in a fresh untar and will not have stale
> arguments to git-submodule.sh) but do no other checks.  If git
> is present, ensure that .git-submodule-status contains an entry
> for all submodules passed on the command line.
> 
> With this change, "ignore" mode is not needed anymore.
> 
> Reported-by: Nina Schoetterl-Glausch <nsg@linux.ibm.com>
> Fixes: b11f9bd96f4 ("configure: move SLOF submodule handling to pc-bios/s390-ccw", 2023-06-06)
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  configure                |  2 +-
>  scripts/git-submodule.sh | 72 ++++++++++++++++++++++------------------
>  2 files changed, 41 insertions(+), 33 deletions(-)
> 
> diff --git a/configure b/configure
> index 86363a7e508..2b41c49c0d1 100755
> --- a/configure
> +++ b/configure
> @@ -758,7 +758,7 @@ done
>  
>  if ! test -e "$source_path/.git"
>  then
> -    git_submodules_action="ignore"
> +    git_submodules_action="validate"
>  fi
>  
>  # test for any invalid configuration combinations
> diff --git a/scripts/git-submodule.sh b/scripts/git-submodule.sh
> index 11fad2137cd..c33d8fe4cac 100755
> --- a/scripts/git-submodule.sh
> +++ b/scripts/git-submodule.sh
> @@ -9,13 +9,22 @@ command=$1
>  shift
>  maybe_modules="$@"
>  
> -# if not running in a git checkout, do nothing
> -test "$command" = "ignore" && exit 0
> -
> +test -z "$maybe_modules" && exit 0
>  test -z "$GIT" && GIT=$(command -v git)
>  
>  cd "$(dirname "$0")/.."
>  
> +no_git_error=
> +if test -n "$maybe_modules" && ! test -e ".git"; then
> +    no_git_error='no git checkout exists'
> +elif test -n "$maybe_modules" && test -z "$GIT"; then
> +    no_git_error='git binary not found'
> +fi

No need to test -n "$maybe_modules" if you exit early above.

> +
> +is_git() {
> +    test -z "$no_git_error"
> +}
> +
>  update_error() {
>      echo "$0: $*"
>      echo
> @@ -34,7 +43,7 @@ update_error() {
>  }
>  
>  validate_error() {
> -    if test "$1" = "validate"; then
> +    if is_git && test "$1" = "validate"; then
>          echo "GIT submodules checkout is out of date, and submodules"
>          echo "configured for validate only. Please run"
>          echo "  scripts/git-submodule.sh update $maybe_modules"
> @@ -51,42 +60,41 @@ check_updated() {
>      test "$CURSTATUS" = "$OLDSTATUS"
>  }
>  
> -if test -n "$maybe_modules" && ! test -e ".git"
> -then
> -    echo "$0: unexpectedly called with submodules but no git checkout exists"
> -    exit 1
> +if is_git; then
> +    test -e $substat || touch $substat
> +    modules=""
> +    for m in $maybe_modules
> +    do
> +        $GIT submodule status $m 1> /dev/null 2>&1
> +        if test $? = 0
> +        then
> +            modules="$modules $m"
> +            grep $m $substat > /dev/null 2>&1 || $GIT submodule status $module >> $substat
> +        else
> +            echo "warn: ignoring non-existent submodule $m"

What is the rational for ignoring non-existing submodules, i.e. how do the arguments to
the script go stale as you say in the patch description?
I'm asking because the fedora spec file initializes a new git repo in order to apply
patches so the script exits with 0.
Nothing that cannot be worked around ofc.

> +        fi
> +    done
> +else
> +    modules=$maybe_modules
>  fi
>  
> -if test -n "$maybe_modules" && test -z "$GIT"
> -then
> -    echo "$0: unexpectedly called with submodules but git binary not found"
> -    exit 1
> -fi
> -
> -modules=""
> -for m in $maybe_modules
> -do
> -    $GIT submodule status $m 1> /dev/null 2>&1
> -    if test $? = 0
> -    then
> -        modules="$modules $m"
> -    else
> -        echo "warn: ignoring non-existent submodule $m"
> -    fi
> -done
> -
>  case "$command" in
>  status|validate)
> -    test -f "$substat" || validate_error "$command"
> -    test -z "$maybe_modules" && exit 0
>      for module in $modules; do
> -        check_updated $module || validate_error "$command"
> +        if is_git; then
> +            check_updated $module || validate_error "$command"
> +        elif ! test -d $module; then

archive-source.sh creates an empty directory for e.g. roms/SLOF,
so this check succeeds even if the submodule sources are unavailable.
Something like

        elif ! test -d $module || test -z "$(ls -A "$module")"; then

works.

> +            echo "$0: sources not available for $module and $no_git_error"
> +            validate_error "$command"
> +        fi
>      done
> -    exit 0
>      ;;
> +
>  update)
> -    test -e $substat || touch $substat
> -    test -z "$maybe_modules" && exit 0
> +    is_git || {
> +        echo "$0: unexpectedly called with submodules but $no_git_error"
> +        exit 1
> +    }
>  
>      $GIT submodule update --init $modules 1>/dev/null
>      test $? -ne 0 && update_error "failed to update modules"

Il mar 20 giu 2023, 19:35 Nina Schoetterl-Glausch <nsg@linux.ibm.com> ha
scritto:

> > +            modules="$modules $m"
> > +            grep $m $substat > /dev/null 2>&1 || $GIT submodule status
> $module >> $substat
> > +        else
> > +            echo "warn: ignoring non-existent submodule $m"
>
> What is the rational for ignoring non-existing submodules, i.e. how do the
> arguments to
> the script go stale as you say in the patch description?
>

For example when a Makefile calls the script before the Makefile itself is
rebuilt.

I'm asking because the fedora spec file initializes a new git repo in order
> to apply
> patches so the script exits with 0.


You mean it succeeds even if roms/SLOF is empty?

Nothing that cannot be worked around ofc.
>
> > +        fi
> > +    done
> > +else
> > +    modules=$maybe_modules
> >  fi
> >
> > -if test -n "$maybe_modules" && test -z "$GIT"
> > -then
> > -    echo "$0: unexpectedly called with submodules but git binary not
> found"
> > -    exit 1
> > -fi
> > -
> > -modules=""
> > -for m in $maybe_modules
> > -do
> > -    $GIT submodule status $m 1> /dev/null 2>&1
> > -    if test $? = 0
> > -    then
> > -        modules="$modules $m"
> > -    else
> > -        echo "warn: ignoring non-existent submodule $m"
> > -    fi
> > -done
> > -
> >  case "$command" in
> >  status|validate)
> > -    test -f "$substat" || validate_error "$command"
> > -    test -z "$maybe_modules" && exit 0
> >      for module in $modules; do
> > -        check_updated $module || validate_error "$command"
> > +        if is_git; then
> > +            check_updated $module || validate_error "$command"
> > +        elif ! test -d $module; then
>
> archive-source.sh creates an empty directory for e.g. roms/SLOF,
> so this check succeeds even if the submodule sources are unavailable.

Something like
>
>         elif ! test -d $module || test -z "$(ls -A "$module")"; then
>

Or (set "$module"/* && test -e "$1").

Paolo

works.
>
> > +            echo "$0: sources not available for $module and
> $no_git_error"
> > +            validate_error "$command"
> > +        fi
> >      done
> > -    exit 0
> >      ;;
> > +
> >  update)
> > -    test -e $substat || touch $substat
> > -    test -z "$maybe_modules" && exit 0
> > +    is_git || {
> > +        echo "$0: unexpectedly called with submodules but $no_git_error"
> > +        exit 1
> > +    }
> >
> >      $GIT submodule update --init $modules 1>/dev/null
> >      test $? -ne 0 && update_error "failed to update modules"
>
>

On Tue, 2023-06-20 at 22:44 +0200, Paolo Bonzini wrote:        
> Il mar 20 giu 2023, 19:35 Nina Schoetterl-Glausch <nsg@linux.ibm.com> ha scritto:
> > > +            modules="$modules $m"
> > > +            grep $m $substat > /dev/null 2>&1 || $GIT submodule status $module >> $substat
> > > +        else
> > > +            echo "warn: ignoring non-existent submodule $m"
> > 
> > What is the rational for ignoring non-existing submodules, i.e. how do the arguments to
> > the script go stale as you say in the patch description?
> 
> For example when a Makefile calls the script before the Makefile itself is rebuilt.

Ah thanks. Can this still happen, roms/SLOF being the only remaining build time user of submodules,
as per 1f468152fb ("build: remove git submodule handling from main makefile")?
pc-bios/s390-ccw/Makefile explicitly names roms/SLOF, so if that were removed, the Makefile would
need to be fixed anyway.

> 
> > I'm asking because the fedora spec file initializes a new git repo in order to apply
> > patches so the script exits with 0.
> 
> You mean it succeeds even if roms/SLOF is empty?

Yeah, it does:

%prep
%autosetup -n qemu-%{version}%{?rcstr} -S git_am

Which I does a git init, git add ., git commit, so no submodules exist and git-submodule.sh ignores
every maybe_module.

Not a problem with qemu, I'm just wondering if this behavior is still the most sensible for git-submodule.sh


[...]

On Wed, 2023-06-21 at 16:07 +0200, Nina Schoetterl-Glausch wrote:
> On Tue, 2023-06-20 at 22:44 +0200, Paolo Bonzini wrote:        
> > Il mar 20 giu 2023, 19:35 Nina Schoetterl-Glausch <nsg@linux.ibm.com> ha scritto:
> > > > +            modules="$modules $m"
> > > > +            grep $m $substat > /dev/null 2>&1 || $GIT submodule status $module >> $substat
> > > > +        else
> > > > +            echo "warn: ignoring non-existent submodule $m"
> > > 
> > > What is the rational for ignoring non-existing submodules, i.e. how do the arguments to
> > > the script go stale as you say in the patch description?
> > 
> > For example when a Makefile calls the script before the Makefile itself is rebuilt.
> 
> Ah thanks. Can this still happen, roms/SLOF being the only remaining build time user of submodules,
> as per 1f468152fb ("build: remove git submodule handling from main makefile")?
> pc-bios/s390-ccw/Makefile explicitly names roms/SLOF, so if that were removed, the Makefile would
> need to be fixed anyway.
> 
> > 
> > > I'm asking because the fedora spec file initializes a new git repo in order to apply
> > > patches so the script exits with 0.
> > 
> > You mean it succeeds even if roms/SLOF is empty?
> 
> Yeah, it does:
> 
> %prep
> %autosetup -n qemu-%{version}%{?rcstr} -S git_am
> 
> Which I does a git init, git add ., git commit, so no submodules exist and git-submodule.sh ignores
> every maybe_module.
> 
> Not a problem with qemu, I'm just wondering if this behavior is still the most sensible for git-submodule.sh

Also the official source tar does include roms/SLOF, so they won't run into problems.
I used the spec file with a tar generated by archive-source.sh which doesn't package roms/SLOF.
How is the official tar generated?

> 
> 
> [...]

On 6/21/23 16:20, Nina Schoetterl-Glausch wrote:
> On Wed, 2023-06-21 at 16:07 +0200, Nina Schoetterl-Glausch wrote:
>> On Tue, 2023-06-20 at 22:44 +0200, Paolo Bonzini wrote:
>>> Il mar 20 giu 2023, 19:35 Nina Schoetterl-Glausch <nsg@linux.ibm.com> ha scritto:
>>>>> +            modules="$modules $m"
>>>>> +            grep $m $substat > /dev/null 2>&1 || $GIT submodule status $module >> $substat
>>>>> +        else
>>>>> +            echo "warn: ignoring non-existent submodule $m"
>>>>
>>>> What is the rational for ignoring non-existing submodules, i.e. how do the arguments to
>>>> the script go stale as you say in the patch description?
>>>
>>> For example when a Makefile calls the script before the Makefile itself is rebuilt.
>>
>> Ah thanks. Can this still happen, roms/SLOF being the only
>> remaining build time user of submodules,
>> as per 1f468152fb ("build: remove git submodule handling from main
>> makefile")? pc-bios/s390-ccw/Makefile explicitly names roms/SLOF,
>> so if that were removed, the Makefile would
>> need to be fixed anyway.

Yeah it cannot happen but I'm thinking it's sensible behavior in 
principle.  It also matches "git submodule update", which doesn't return 
an error if the required path is not a submodule.

>>> You mean it succeeds even if roms/SLOF is empty?
>>
>> Yeah, it does:
>>
>> %prep
>> %autosetup -n qemu-%{version}%{?rcstr} -S git_am
>>
>> Which I does a git init, git add ., git commit, so no submodules exist
>> and git-submodule.sh ignores every maybe_module.
> 
> Also the official source tar does include roms/SLOF, so they won't run into problems.
> I used the spec file with a tar generated by archive-source.sh which doesn't package roms/SLOF.
> How is the official tar generated?

It's generated with scripts/make-release.

Paolo