On Thu, 10 Oct 2024 at 16:07, Paolo Bonzini <pbonz...@redhat.com> wrote:
>
> Rust subprojects have the semantic version (followed by -rs) in the subproject
> name, but the full version (without -rs) is used by crates.io for the root
> directory of the tarball.  Teach scripts/archive-source.sh to look for the
> root directory name in wrap files.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
>  scripts/archive-source.sh | 22 +++++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/archive-source.sh b/scripts/archive-source.sh
> index 65af8063e4b..7c7727eab58 100755
> --- a/scripts/archive-source.sh
> +++ b/scripts/archive-source.sh
> @@ -48,13 +48,33 @@ function tree_ish() {
>      echo "$retval"
>  }
>
> +function subproject_dir() {

"function" here is unnecessary and a bashism, but I guess that's
the way the rest of the script is written.

> +    if test -f subprojects/$1.wrap; then

We should quote $1

> +      # Print the directory key of the wrap file, defaulting to the 
> subproject name

Probably worth commenting here what the format of the wrap
file is (i.e. that it is a toml file and we are looking for
the "directory" key in a [wrap-something] section).

> +      local dir=$(sed \

https://www.shellcheck.net/wiki/SC2155
says we should write this
         local dir
         dir = "$(sed .... \
                ... \
                subprojects/"$1".wrap)"

to avoid losing the exit status of sed.

> +        -ne '/^\[wrap-[a-z][a-z]*\]$/!b' \
> +        -e ':label' \
> +        -e 'n' \
> +        -e 's/^directory *= *//p' \
> +        -e 'tquit' \
> +        -e '/^\[$/!blabel' \
> +        -e ':quit' \
> +        -e 'q' \
> +        subprojects/$1.wrap)

You could also write this

      local dir="$(sed -ne \
        '/^\[wrap-[a-z][a-z]*\]$/,/^\[/s/^directory *= *//p' \
        subprojects/"$1".wrap)"

i.e. use a sed address-range to say "operate only on
lines between [wrap-foo] and the next line starting '[',
and within that range print out the value of the directory
key". Definitely shorter. Is it more readable? IDK.
Behaviour in corner cases (multiple [wrap-foo] sections,
multiple 'directory' key lines) is different, but none of
our wrap files have those weirdnesses and I don't think
they're valid wrapfile syntax anyway.

> +      echo "${dir-$1}"

I think you want "${dir:-$1}" here, so that we get the
argument both if 'dir' is unset and if it is the
empty string. Otherwise (at least in my local testing)
you get the wrong thing for passing it 'keycodemapdb' or
other wrapfiles with no directory= key.

> +    else
> +      echo "error: scripts/archive-source.sh should only process wrap 
> subprojects" 2>&1
> +      exit 1
> +    fi

Personally I think
     if test ! -f subprojects/"$1".wrap; then
        echo "error: ..."
        exit
     fi
     # normal handling here

is a bit more readable than deferring the error-exit to
the end of the function.

> +}
> +
>  git archive --format tar "$(tree_ish)" > "$tar_file"
>  test $? -ne 0 && error "failed to archive qemu"
>
>  for sp in $subprojects; do
>      meson subprojects download $sp
>      test $? -ne 0 && error "failed to download subproject $sp"
> -    tar --append --file "$tar_file" --exclude=.git subprojects/$sp
> +    tar --append --file "$tar_file" --exclude=.git 
> subprojects/$(subproject_dir $sp)

We should quote: subprojects/"$(subproject_dir "$sp")"

>      test $? -ne 0 && error "failed to append subproject $sp to $tar_file"
>  done
>  exit 0
> --
> 2.46.2

thanks
-- PMM

Reply via email to