[pacman-dev] makepkg: respect --ignorearch when verifying source or printing packagelist
diff mbox

Message ID 20180403215029.20409-1-eschwartz@archlinux.org
State New
Headers show

Commit Message

Eli Schwartz April 3, 2018, 9:50 p.m. UTC
--verifysource is often used to validate the PKGBUILD before uploading
to the AUR, but currently there is no way to trivially check all sources.
By default we don't check sources we won't use, because it forces
downloading those sources, but in certain cases the user might need to
check them regardless...

--packagelist is usually used to derive the built package names for the
user's machine, but in some cases, e.g. repo building, we might want to
get the packagelist for all supported arches. This is primarily
motivated by the desire to use this in the dbscripts testsuite (which
tests for i686 and x86_64). While dbscripts could use `for a in
${ARCHES[@]}; do CARCH=$a makepkg --packagelist` it seemed easier to add
this directly in makepkg once I was already changing --verifysource.

This partially reverts d8591dd3418d55c5736022ef003891fc03b953e0 but
ensures the use is controllable.

Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
---
 doc/makepkg.8.txt                      | 13 ++++++++-----
 scripts/libmakepkg/util/pkgbuild.sh.in | 14 ++++++++++----
 scripts/makepkg.sh.in                  | 11 ++++++++---
 3 files changed, 26 insertions(+), 12 deletions(-)

Comments

Allan McRae April 29, 2018, 11:28 a.m. UTC | #1
On 04/04/18 07:50, Eli Schwartz wrote:
> --verifysource is often used to validate the PKGBUILD before uploading
> to the AUR, but currently there is no way to trivially check all sources.
> By default we don't check sources we won't use, because it forces
> downloading those sources, but in certain cases the user might need to
> check them regardless...
> 

We looked at this when architecture dependent sources were added and
many or even most packages with architecture independent sources have
sources with the same filename from a different source path, or change
the source to have the same filename to make the rest of the PKGBUILD
easier.  So that was not implemented.

> --packagelist is usually used to derive the built package names for the
> user's machine, but in some cases, e.g. repo building, we might want to
> get the packagelist for all supported arches. This is primarily
> motivated by the desire to use this in the dbscripts testsuite (which
> tests for i686 and x86_64). While dbscripts could use `for a in
> ${ARCHES[@]}; do CARCH=$a makepkg --packagelist` it seemed easier to add
> this directly in makepkg once I was already changing --verifysource.

I'm not adding a feature to support a testsuite and not an actual usage.
 Do the loop.

> This partially reverts d8591dd3418d55c5736022ef003891fc03b953e0 but
> ensures the use is controllable.
> 
> Signed-off-by: Eli Schwartz <eschwartz@archlinux.org>
> ---
>  doc/makepkg.8.txt                      | 13 ++++++++-----
>  scripts/libmakepkg/util/pkgbuild.sh.in | 14 ++++++++++----
>  scripts/makepkg.sh.in                  | 11 ++++++++---
>  3 files changed, 26 insertions(+), 12 deletions(-)
> 
> diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt
> index a065b795..42abceae 100644
> --- a/doc/makepkg.8.txt
> +++ b/doc/makepkg.8.txt
> @@ -37,9 +37,10 @@ your logs and output are not localized.
>  Options
>  -------
>  *-A, \--ignorearch*::
> -	Ignore a missing or incomplete arch field in the build script. This is
> -	for rebuilding packages from source when the PKGBUILD may be slightly
> -	outdated and not updated with an `arch=('yourarch')` field.
> +	Ignore a missing or incomplete arch field in the build script, when
> +	building packages. This is for rebuilding packages from source when
> +	the PKGBUILD may be slightly outdated and not updated with an `arch=('yourarch')`
> +	field. Act on all available arches, when performing other actions.
>  
>  *-c, \--clean*::
>  	Clean up leftover work files and directories after a successful build.
> @@ -66,7 +67,8 @@ Options
>  	if required and perform the integrity checks. No extraction or build is
>  	performed. Dependencies specified in the PKGBUILD will not be handled
>  	unless `--syncdeps` is used. Useful for performing subsequent offline
> -	builds.
> +	builds. When `--ignorearch` is used, verify all split arch sources instead
> +	of just the sources for the current arch.
>  
>  *-f, \--force*::
>  	makepkg will not build a package if a built package already exists in
> @@ -201,7 +203,8 @@ Options
>  
>  *\--packagelist*::
>  	List the package filenames that would be produced without building. Listed
> -	package filenames include PKGDEST and PKGEXT.
> +	package filenames include PKGDEST and PKGEXT. When `--ignorearch` is used,
> +	list filenames for all supported arches.
>  
>  *\--printsrcinfo*::
>  	Generate and print the SRCINFO file to stdout.
> diff --git a/scripts/libmakepkg/util/pkgbuild.sh.in b/scripts/libmakepkg/util/pkgbuild.sh.in
> index 2db46f1f..e23208c3 100644
> --- a/scripts/libmakepkg/util/pkgbuild.sh.in
> +++ b/scripts/libmakepkg/util/pkgbuild.sh.in
> @@ -182,11 +182,17 @@ print_all_package_names() {
>  	local version=$(get_full_version)
>  	local architecture pkg opts a
>  	for pkg in ${pkgname[@]}; do
> -		architecture=$(get_pkg_arch $pkg)
> -		printf "%s/%s-%s-%s%s\n" "$PKGDEST" "$pkg" "$version" "$architecture" "$PKGEXT"
> -		if check_option "debug" "y" && check_option "strip" "y"; then
> -			printf "%s/%s-%s-%s-%s%s\n" "$PKGDEST" "$pkg" "@DEBUGSUFFIX@" "$version" "$architecture" "$PKGEXT"
> +		if (( IGNOREARCH )); then
> +			get_pkgbuild_attribute "$pkg" 'arch' 1 architecture
> +		else
> +			architecture=$(get_pkg_arch $pkg)
>  		fi
> +		for a in "${architecture[@]}"; do
> +			printf "%s/%s-%s-%s%s\n" "$PKGDEST" "$pkg" "$version" "$a" "$PKGEXT"
> +			if check_option "debug" "y" && check_option "strip" "y"; then
> +				printf "%s/%s-%s-%s-%s%s\n" "$PKGDEST" "$pkg" "@DEBUGSUFFIX@" "$version" "$a" "$PKGEXT"
> +			fi
> +		done
>  	done
>  }
>  
> diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
> index bc2d0061..584c10a8 100644
> --- a/scripts/makepkg.sh.in
> +++ b/scripts/makepkg.sh.in
> @@ -1262,7 +1262,7 @@ while true; do
>  		--nosign)         SIGNPKG='n' ;;
>  		-o|--nobuild)     NOBUILD=1 ;;
>  		-p)               shift; BUILDFILE=$1 ;;
> -		--packagelist)    PACKAGELIST=1 IGNOREARCH=1;;
> +		--packagelist)    PACKAGELIST=1 ;;
>  		--printsrcinfo)   PRINTSRCINFO=1 IGNOREARCH=1;;
>  		-r|--rmdeps)      RMDEPS=1 ;;
>  		-R|--repackage)   REPKG=1 ;;
> @@ -1631,8 +1631,13 @@ if (( !REPKG )); then
>  	if (( NOEXTRACT && ! VERIFYSOURCE )); then
>  		warning "$(gettext "Using existing %s tree")" "\$srcdir/"
>  	else
> -		download_sources
> -		check_source_integrity
> +		if (( VERIFYSOURCE && IGNOREARCH )); then
> +			download_sources allarch
> +			check_source_integrity all
> +		else
> +			download_sources
> +			check_source_integrity
> +		fi
>  		(( VERIFYSOURCE )) && exit $E_OK
>  
>  		if (( CLEANBUILD )); then
>
Eli Schwartz April 29, 2018, 2:45 p.m. UTC | #2
On 04/29/2018 06:28 AM, Allan McRae wrote:
> On 04/04/18 07:50, Eli Schwartz wrote:
>> --verifysource is often used to validate the PKGBUILD before uploading
>> to the AUR, but currently there is no way to trivially check all sources.
>> By default we don't check sources we won't use, because it forces
>> downloading those sources, but in certain cases the user might need to
>> check them regardless...
>>
> 
> We looked at this when architecture dependent sources were added and
> many or even most packages with architecture independent sources have
> sources with the same filename from a different source path, or change
> the source to have the same filename to make the rest of the PKGBUILD
> easier.  So that was not implemented.

As mentioned on IRC, this means --allsource is broken too, as we use
download_sources allarch followed by check_source_integrity all.

In fact, now I remember part of why I wanted this patch implemented, is
because the alternative which *works today* is to use --allsource, then
discard the source package. We at least need to be consistent here.

My $0.02: people who change the filename to use the same filename on all
arches, should use $CARCH in their build/package functions instead, and
name the files something CARCH-dependent if it isn't that way by
default. Extracted archives don't even need to worry about this, as they
will presumably use the extracted paths which are identical in $srcdir
regardless of the tarball filename in $SRCDEST!
Eli Schwartz May 13, 2018, 3:19 a.m. UTC | #3
On 04/29/2018 07:28 AM, Allan McRae wrote:
> On 04/04/18 07:50, Eli Schwartz wrote:
>> --verifysource is often used to validate the PKGBUILD before uploading
>> to the AUR, but currently there is no way to trivially check all sources.
>> By default we don't check sources we won't use, because it forces
>> downloading those sources, but in certain cases the user might need to
>> check them regardless...
>>
> 
> We looked at this when architecture dependent sources were added and
> many or even most packages with architecture independent sources have
> sources with the same filename from a different source path, or change
> the source to have the same filename to make the rest of the PKGBUILD
> easier.  So that was not implemented.

Since this does not currently work anyway due to --allsource not
supporting it, I don't think it makes sense to reject features over this.

I'd argue we shouldn't support this anyway:

CARCH-specific *tarballs* won't clash anyway as only the one for the
currently building architecture will be extracted, so either they both
use the same internal filelist but won't clash, or they use
CARCH-specific filelists in which case the maintainer must switch
between them anyway. In either case it's out of our hands.

For files, it's trivial to rename them to foo-x86_64 then use foo-$CARCH
in the build/package functions. IMHO it makes more sense, because this
results in more obvious behavior in addition to supporting my patch...
though I could be biased.

I guess an argument could be made that since we functionally don't
support it, we should do an explicit linting check, but that's a
separate matter.

>> --packagelist is usually used to derive the built package names for the
>> user's machine, but in some cases, e.g. repo building, we might want to
>> get the packagelist for all supported arches. This is primarily
>> motivated by the desire to use this in the dbscripts testsuite (which
>> tests for i686 and x86_64). While dbscripts could use `for a in
>> ${ARCHES[@]}; do CARCH=$a makepkg --packagelist` it seemed easier to add
>> this directly in makepkg once I was already changing --verifysource.
> 
> I'm not adding a feature to support a testsuite and not an actual usage.
>  Do the loop.

Would it help to convince you if I said I also want to use this in
devtools instead of some custom function to find cached packages? It's
less important there because unlike dbscripts, I don't need to fix
makepkg 5.1 compatibility *anyway*, but IMHO devtools could still use
it. Even if we only currently support one arch in devtools, it's still
"more proper" behavior.

I'm not sure how many other thirdparty uses could be helped by this.
Mostly because I don't really follow thirdparty infrastructure built
around makepkg.

Patch
diff mbox

diff --git a/doc/makepkg.8.txt b/doc/makepkg.8.txt
index a065b795..42abceae 100644
--- a/doc/makepkg.8.txt
+++ b/doc/makepkg.8.txt
@@ -37,9 +37,10 @@  your logs and output are not localized.
 Options
 -------
 *-A, \--ignorearch*::
-	Ignore a missing or incomplete arch field in the build script. This is
-	for rebuilding packages from source when the PKGBUILD may be slightly
-	outdated and not updated with an `arch=('yourarch')` field.
+	Ignore a missing or incomplete arch field in the build script, when
+	building packages. This is for rebuilding packages from source when
+	the PKGBUILD may be slightly outdated and not updated with an `arch=('yourarch')`
+	field. Act on all available arches, when performing other actions.
 
 *-c, \--clean*::
 	Clean up leftover work files and directories after a successful build.
@@ -66,7 +67,8 @@  Options
 	if required and perform the integrity checks. No extraction or build is
 	performed. Dependencies specified in the PKGBUILD will not be handled
 	unless `--syncdeps` is used. Useful for performing subsequent offline
-	builds.
+	builds. When `--ignorearch` is used, verify all split arch sources instead
+	of just the sources for the current arch.
 
 *-f, \--force*::
 	makepkg will not build a package if a built package already exists in
@@ -201,7 +203,8 @@  Options
 
 *\--packagelist*::
 	List the package filenames that would be produced without building. Listed
-	package filenames include PKGDEST and PKGEXT.
+	package filenames include PKGDEST and PKGEXT. When `--ignorearch` is used,
+	list filenames for all supported arches.
 
 *\--printsrcinfo*::
 	Generate and print the SRCINFO file to stdout.
diff --git a/scripts/libmakepkg/util/pkgbuild.sh.in b/scripts/libmakepkg/util/pkgbuild.sh.in
index 2db46f1f..e23208c3 100644
--- a/scripts/libmakepkg/util/pkgbuild.sh.in
+++ b/scripts/libmakepkg/util/pkgbuild.sh.in
@@ -182,11 +182,17 @@  print_all_package_names() {
 	local version=$(get_full_version)
 	local architecture pkg opts a
 	for pkg in ${pkgname[@]}; do
-		architecture=$(get_pkg_arch $pkg)
-		printf "%s/%s-%s-%s%s\n" "$PKGDEST" "$pkg" "$version" "$architecture" "$PKGEXT"
-		if check_option "debug" "y" && check_option "strip" "y"; then
-			printf "%s/%s-%s-%s-%s%s\n" "$PKGDEST" "$pkg" "@DEBUGSUFFIX@" "$version" "$architecture" "$PKGEXT"
+		if (( IGNOREARCH )); then
+			get_pkgbuild_attribute "$pkg" 'arch' 1 architecture
+		else
+			architecture=$(get_pkg_arch $pkg)
 		fi
+		for a in "${architecture[@]}"; do
+			printf "%s/%s-%s-%s%s\n" "$PKGDEST" "$pkg" "$version" "$a" "$PKGEXT"
+			if check_option "debug" "y" && check_option "strip" "y"; then
+				printf "%s/%s-%s-%s-%s%s\n" "$PKGDEST" "$pkg" "@DEBUGSUFFIX@" "$version" "$a" "$PKGEXT"
+			fi
+		done
 	done
 }
 
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in
index bc2d0061..584c10a8 100644
--- a/scripts/makepkg.sh.in
+++ b/scripts/makepkg.sh.in
@@ -1262,7 +1262,7 @@  while true; do
 		--nosign)         SIGNPKG='n' ;;
 		-o|--nobuild)     NOBUILD=1 ;;
 		-p)               shift; BUILDFILE=$1 ;;
-		--packagelist)    PACKAGELIST=1 IGNOREARCH=1;;
+		--packagelist)    PACKAGELIST=1 ;;
 		--printsrcinfo)   PRINTSRCINFO=1 IGNOREARCH=1;;
 		-r|--rmdeps)      RMDEPS=1 ;;
 		-R|--repackage)   REPKG=1 ;;
@@ -1631,8 +1631,13 @@  if (( !REPKG )); then
 	if (( NOEXTRACT && ! VERIFYSOURCE )); then
 		warning "$(gettext "Using existing %s tree")" "\$srcdir/"
 	else
-		download_sources
-		check_source_integrity
+		if (( VERIFYSOURCE && IGNOREARCH )); then
+			download_sources allarch
+			check_source_integrity all
+		else
+			download_sources
+			check_source_integrity
+		fi
 		(( VERIFYSOURCE )) && exit $E_OK
 
 		if (( CLEANBUILD )); then