| Message ID | 20180219233156.4016-1-lukeshu@lukeshu.com |
|---|---|
| State | Superseded |
| Headers | show |
On 02/19/2018 06:31 PM, Luke Shumaker wrote: > From: Luke Shumaker <lukeshu@parabola.nu> > > This has the test change PKGEXT the second time it tries to release the > package. Currently, this causes the tests to fail. That's a good thing; > it's checking for the regression where db-functions:check_pkgrepos isn't > treating PKGEXT as a glob. > > Without this, that regression didn't cause test failure because the checks > right after it were tripping anyway. > > https://lists.archlinux.org/pipermail/arch-projects/2018-February/004742.html > > v2: Follow Eli's suggestion to simplify it using the check in __buildPackage > v3: Simplify further by assuming __buildPackage checks PKGEXT, not PKGEXTS > --- > This is written againt Eli's v2 patchset (my concerns there don't > affect this). You can verify--applying this patch first makes the > tests fail, then applying Eli's patches make the tests pass again. > > Dave's objections to the __isGlobfile name and comment apply to this > patch as well. As far as the testsuite is concerned, you can just use "Fix overloading PKGEXT to mean two things." as a base. This means that all you need to do is check that if you releasePackage the same package twice using a new $PKGEXT it is still rejected. ... We're not testing whether or not globs work, we're testing whether or not check_pkgrepos properly detects pre-existing packages (which it does via globs). Using __isGlobfile() here will no longer be useful information once $PKGEXT is only ever something from makepkg. So it doesn't make sense to add code that will be almost immediately removed.
On 02/19/2018 08:47 PM, Eli Schwartz wrote: > On 02/19/2018 06:31 PM, Luke Shumaker wrote: >> From: Luke Shumaker <lukeshu@parabola.nu> >> >> This has the test change PKGEXT the second time it tries to release the >> package. Currently, this causes the tests to fail. That's a good thing; >> it's checking for the regression where db-functions:check_pkgrepos isn't >> treating PKGEXT as a glob. >> >> Without this, that regression didn't cause test failure because the checks >> right after it were tripping anyway. >> >> https://lists.archlinux.org/pipermail/arch-projects/2018-February/004742.html >> >> v2: Follow Eli's suggestion to simplify it using the check in __buildPackage >> v3: Simplify further by assuming __buildPackage checks PKGEXT, not PKGEXTS >> --- >> This is written againt Eli's v2 patchset (my concerns there don't >> affect this). You can verify--applying this patch first makes the >> tests fail, then applying Eli's patches make the tests pass again. >> >> Dave's objections to the __isGlobfile name and comment apply to this >> patch as well. > As far as the testsuite is concerned, you can just use "Fix overloading > PKGEXT to mean two things." as a base. This means that all you need to > do is check that if you releasePackage the same package twice using a > new $PKGEXT it is still rejected. > > ... > > We're not testing whether or not globs work, we're testing whether or > not check_pkgrepos properly detects pre-existing packages (which it does > via globs). Using __isGlobfile() here will no longer be useful > information once $PKGEXT is only ever something from makepkg. So it > doesn't make sense to add code that will be almost immediately removed. Actually, it would tend to help if we had the actual candidate filenames here. Hmm...
On 02/19/2018 09:12 PM, Eli Schwartz wrote: > On 02/19/2018 08:47 PM, Eli Schwartz wrote: >> On 02/19/2018 06:31 PM, Luke Shumaker wrote: >>> From: Luke Shumaker <lukeshu@parabola.nu> >>> >>> This has the test change PKGEXT the second time it tries to release the >>> package. Currently, this causes the tests to fail. That's a good thing; >>> it's checking for the regression where db-functions:check_pkgrepos isn't >>> treating PKGEXT as a glob. >>> >>> Without this, that regression didn't cause test failure because the checks >>> right after it were tripping anyway. >>> >>> https://lists.archlinux.org/pipermail/arch-projects/2018-February/004742.html >>> >>> v2: Follow Eli's suggestion to simplify it using the check in __buildPackage >>> v3: Simplify further by assuming __buildPackage checks PKGEXT, not PKGEXTS >>> --- >>> This is written againt Eli's v2 patchset (my concerns there don't >>> affect this). You can verify--applying this patch first makes the >>> tests fail, then applying Eli's patches make the tests pass again. >>> >>> Dave's objections to the __isGlobfile name and comment apply to this >>> patch as well. >> As far as the testsuite is concerned, you can just use "Fix overloading >> PKGEXT to mean two things." as a base. This means that all you need to >> do is check that if you releasePackage the same package twice using a >> new $PKGEXT it is still rejected. >> >> ... >> >> We're not testing whether or not globs work, we're testing whether or >> not check_pkgrepos properly detects pre-existing packages (which it does >> via globs). Using __isGlobfile() here will no longer be useful >> information once $PKGEXT is only ever something from makepkg. So it >> doesn't make sense to add code that will be almost immediately removed. > > Actually, it would tend to help if we had the actual candidate filenames > here. Hmm... Maybe: > if [[ -n ${BUILDDIR} ]]; then > cache=${BUILDDIR}/$(__getCheckSum PKGBUILD) > - if [[ -d ${cache} ]]; then > + if __isGlobfile "${cache}"/*"${PKGEXT}"; then > cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} > return 0 > else could use if cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} 2>/dev/null; then return 0 else This would avoid the need for __isGlobfile function altogether. I'd also like a more descriptive commit message. Don't tell me what you changed, tell me why you changed it. :p
Eli Schwartz wrote: > > > if [[ -n ${BUILDDIR} ]]; then > > cache=${BUILDDIR}/$(__getCheckSum PKGBUILD) > > - if [[ -d ${cache} ]]; then > > + if __isGlobfile "${cache}"/*"${PKGEXT}"; then > > cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} > > return 0 > > else > > We're not testing whether or not globs work, we're testing whether or > not check_pkgrepos properly detects pre-existing packages (which it does > via globs). Using __isGlobfile() here will no longer be useful > information once $PKGEXT is only ever something from makepkg. So it > doesn't make sense to add code that will be almost immediately removed. The glob I was using it for wasn't PKGEXT(s), it was the '*' that's right there in the argument! > Maybe: ... > if cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} 2>/dev/null; then > return 0 > else > > This would avoid the need for __isGlobfile function altogether. I like that! Good idea! > I'd also like a more descriptive commit message. Don't tell me what you > changed, tell me why you changed it. :p Ok, I'll go in to that more.
On 02/19/2018 10:24 PM, Luke Shumaker wrote: > The glob I was using it for wasn't PKGEXT(s), it was the '*' that's > right there in the argument! Right, that's why I replied to myself with "Actually, it would tend to help if we had the actual candidate filenames here. Hmm..." Context: I wrote a makepkg patch whose sole purpose is to ensure that people can extract a newline-separated list of absolute filepaths for each package that a PKGBUILD would/did create. No globs needed. :) I'm hoping this will simplify some rather hackish and fragile logic in *many* projects that interface with makepkg. >> Maybe: > ... >> if cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} 2>/dev/null; then >> return 0 >> else >> >> This would avoid the need for __isGlobfile function altogether. > > I like that! Good idea! > >> I'd also like a more descriptive commit message. Don't tell me what you >> changed, tell me why you changed it. :p > > Ok, I'll go in to that more. The changes look great, thanks.
diff --git a/test/cases/db-update.bats b/test/cases/db-update.bats index 1da7eef..2395438 100644 --- a/test/cases/db-update.bats +++ b/test/cases/db-update.bats @@ -92,7 +92,7 @@ load ../lib/common db-update checkPackage extra pkg-any-a - releasePackage extra pkg-any-a + PKGEXT=.pkg.tar.gz releasePackage extra pkg-any-a run db-update [ "$status" -ne 0 ] } diff --git a/test/lib/common.bash b/test/lib/common.bash index 540e403..d251259 100644 --- a/test/lib/common.bash +++ b/test/lib/common.bash @@ -13,6 +13,11 @@ __getCheckSum() { echo ${result[0]} } +# Check if a file exists, even if the file uses wildcards +__isGlobfile() { + [[ -f $1 ]] +} + __buildPackage() { local pkgdest=${1:-.} local p @@ -23,7 +28,7 @@ __buildPackage() { if [[ -n ${BUILDDIR} ]]; then cache=${BUILDDIR}/$(__getCheckSum PKGBUILD) - if [[ -d ${cache} ]]; then + if __isGlobfile "${cache}"/*"${PKGEXT}"; then cp -Lv ${cache}/*${PKGEXT}{,.sig} ${pkgdest} return 0 else
From: Luke Shumaker <lukeshu@parabola.nu> This has the test change PKGEXT the second time it tries to release the package. Currently, this causes the tests to fail. That's a good thing; it's checking for the regression where db-functions:check_pkgrepos isn't treating PKGEXT as a glob. Without this, that regression didn't cause test failure because the checks right after it were tripping anyway. https://lists.archlinux.org/pipermail/arch-projects/2018-February/004742.html v2: Follow Eli's suggestion to simplify it using the check in __buildPackage v3: Simplify further by assuming __buildPackage checks PKGEXT, not PKGEXTS --- This is written againt Eli's v2 patchset (my concerns there don't affect this). You can verify--applying this patch first makes the tests fail, then applying Eli's patches make the tests pass again. Dave's objections to the __isGlobfile name and comment apply to this patch as well. test/cases/db-update.bats | 2 +- test/lib/common.bash | 7 ++++++- 2 files changed, 7 insertions(+), 2 deletions(-)