| Message ID | 20180403215029.20409-1-eschwartz@archlinux.org |
|---|---|
| State | Under Review |
| Headers | show |
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 >
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!
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.
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
--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(-)