Backports from Parabola #5

LukeShu · 2018-03-14T05:35:16Z

These are commits from Parabola's dbscripts that I think are
upstreamable. No real theme to them, but because some of them are
wide-sweeping (eg. clean up quoting), they are a good base layer to
apply before backporting other changes.

I've done my best to look over them, but at least one weird thing did
happen while rebasing them, so it's possible I missed something that
rebased weird.

Some of this duplicates (non-backported) changes in the last patchset
I submitted.

This is currently v2 of the patchset. v1 was submitted on the mailing list. I'm submitting v2 here to get a shiny little confirmation from Travis that I didn't break anything; I'm submitting it to the mailing list as well.

LukeShu · 2018-03-14T05:46:11Z

Huh, Travis ran slightly faster than my local make test. Eli's awk expression in the final commit. I've pushed a revised version. I'm going to bed. If in the morning Travis/make test came back saying we're good, I'll also send it to the mailing list.

eli-schwartz

As I said on the mailing list, I don't see the need to overquote variables everywhere. You want it for the sake of linting (this is a shellcheck thing I actually disagree with) and to reduce the diff against your fork (which is not our design goal).

I'm not sure what to do about it.

eli-schwartz · 2018-03-14T06:21:22Z

cron-jobs/sourceballs

 		# Should this package be skipped?
 		if grep -Fqx "${pkgbase}" "${dirname}/sourceballs.skip"; then
 			continue
 		fi
 		# Check if the license or .force file does not enforce creating a source package
-		if ! ([[ -z ${ALLOWED_LICENSES[@]} ]] || chk_license ${pkglicense[@]} || grep -Fqx "${pkgbase}" "${dirname}/sourceballs.force"); then
+		if ! ([[ ${#ALLOWED_LICENSES[@]} == 0 ]] || chk_license "${pkglicense[@]}" || grep -Fqx "${pkgbase}" "${dirname}/sourceballs.force"); then


Instead of string comparisons why not use shell arithmetic?

eli-schwartz · 2018-03-14T06:23:27Z

db-functions

@@ -282,7 +282,7 @@ getpkgfile() {

 getpkgfiles() {
 	local f files
-	if [[ ! -z "$(printf '%s\n' "${@%\.*}" | sort | uniq -D)" ]]; then
+	if ! printf '%s\n' "${@%\.*}" | awk 'a[$0]++>1{exit 1}'; then


Why not fix this all at once instead of changing the same line multiple times?

It's two logically distinct changes.

And so are several other changes you lumped into the first commit.

They're all "this array wasn't quoted, but we can do a better fix than simply quoting it"

eli-schwartz · 2018-03-14T06:25:45Z

db-functions

-	else
-		return 1
-	fi
+	[[ ${pkgfile##*/} == "${pkgname}-${pkgver}-${pkgarch}"* ]]


Don't use ==, use = instead. This is a weird bash extension that literally serves no purpose as it merely aliases =.

[[ is a weird bash extension.

No, it is a cool bash extension that serves a purpose. You've added the only two examples of using == in the main code, just now, and I dispute Chet Ramey's reasons for adding the == alias to the bash scripting language as it adds no utility to the language while adding compatibility to C as an anti-feature.

Glob/regex matching, but the rest of it is weird. [[ -r $filename ]] adds nothing to the language, and serves no purpose over [ -r "$filename" ]

At the end of the day, it's just a style choice. And that's fine. But "it's the codebase's style" is the winning argument there, not "it's a bad extension". I hadn't realized that the main codebase was all =; I'd though it was inconsistent; I hadn't realized that all of the [[ == ]] uses were in test/.

eli-schwartz · 2018-03-14T06:49:30Z

cron-jobs/check_archlinux/parse_pkgbuilds.sh

 		indirect="${var}_backup"
 		if [ -n "${!indirect}" ]; then
-			eval "${var}=(\${$indirect[@]})"
+			eval "${var}=(\"\${$indirect[@]}\")"


I would still like to fix this by avoiding the issue entirely and using declare as I mentioned on the mailing list.

From the ML:

Maybe I should fix the backups as well, but that is a slightly more complicated case there [in makepkg].

Is it? It looks like the exact same case there.

What about this?

backup_package_variables() { restore_package_variables=$(declare -p "${splitpkg_overrides[@]}" 2>/dev/null || true) } restore_package_variables() { unset "${splitpkg_overrides[@]}" eval "$restore_package_variables" }

eli-schwartz · 2018-03-14T07:10:38Z

test/cases/db-update.bats

@@ -222,7 +222,7 @@ load ../lib/common

 @test "package has to be aregular file" {
 	local p
-	local target=$(mktemp -d)
+	local target=$(mktemp -dt)


No TEMPLATE given, this change is extraneous.

Whoops, you are correct.

LukeShu · 2018-03-14T16:09:30Z

As I said on the mailing list, I don't see the need to overquote variables everywhere. You want it for the sake of linting (this is a shellcheck thing I actually disagree with) and to reduce the diff against your fork (which is not our design goal).

I'm not sure what to do about it.

I respect that. I'm not sure what I would do about it if I were in your place. If you want me to re-submit this patchset without the quoting commit, let me know.

LukeShu · 2018-03-14T21:26:01Z

I've just pushed v3 of the patchset:

v3:

Use [[ $v = true ]] for testing boolean variables
Split the initial quoting commit in to a "only add double-quotes"
commit and a commit with more involved changes
Combine the sort|uniq -> awk replacement commit with the more
involved quoting commit
db-functions:check_pkgfile: Use [[ = ]] instead of [[ == ]]
Don't bother with mktemp's -t flag if there isn't a template given
Use declare -p to simplify {backup,restore}_package_variables
Use (( )) instead of [[ ]] when checking if an array is empty

I believe it addresses all of Eli's concerns, though it still quotes variables (but not variable-used-as-a-command; we don't have those anymore). I would have never though that quoting variables would be a controversial change.

eli-schwartz · 2018-03-15T01:42:06Z

cron-jobs/check_archlinux/parse_pkgbuilds.sh

-		fi
-	done
+	unset "${splitpkg_overrides[@]}"
+	eval "$restore_package_variables"


FWIW: https://lists.archlinux.org/pipermail/pacman-dev/2018-March/022406.html

TBH this whole script seems somewhat convoluted. If anything I think I'd rather port check_packages.py to use pyalpm instead of ctypes, and makepkg --printsrcinfo + python-srcinfo rather than parse_pkgbuilds.sh

But then again, I don't think anyone still uses it...

Agree about it being convoluted.

Isn't your solution going to have declare -p spit things on stderr?

eli-schwartz · 2018-03-16T01:15:32Z

test/lib/common.bash

-					echo "$(bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O)" | grep -qv ${pkgname}
+					if bsdtar -xf "${FTP_BASE}/${repo}/os/${tarch}/${repo}${db%.tar.*}" -O | grep ${pkgname} &>/dev/null; then
+						return 1
+					fi


grep -v has been converted to if grep; then return 1

I understand why that has been done here, as AFAICT this used to always return successfully since there was always some line that did not contain ${pkgname} -- even if that line was %NAME% :D

Which means we weren't actually testing anything with checkRemovedPackageDB as we cannot actually get test failures.

That being said, this does need to be noted in a commit message.

I also think it would make more sense to check for a pkgname using bsdtar -tf whereas in the other change we need to use -xf in order to check a pkgfile. (I'm not positive why we need to check the pkgfile there either.)

eli-schwartz · 2018-03-16T01:29:34Z

testing2x

@@ -49,7 +49,7 @@ done
 for pkgarch in ${ARCHES[@]}; do
 	repo_unlock ${TESTING_REPO} ${pkgarch}
 done
-for repo in ${STABLE_REPOS[@]}; do
+for repo in  "${STABLE_REPOS[@]}"; do


Added extra whitespace here, commit message says you are removing whitespace.

Huh. I wonder how I screwed that up. But I did remove whitespace further up in the commit!

eli-schwartz · 2018-03-16T01:33:30Z

db-functions

 		chgrp $group "${filesfile}" || error "Could not change group of %s to %s" "$filesfile" "$group"
-		chmod g+w "${dbfile}" || error "Could not set write permission for group %s to %s" "$group" "$dbfile"
+		chmod g+w "${dbfile}"    || error "Could not set write permission for group %s to %s" "$group" "$dbfile"
 		chmod g+w "${filesfile}" || error "Could not set write permission for group %s to %s" "$group" "$filesfile"


You lined up one of these, but the other is still misaligned...

Whoops. That line is 2 characters to the left because $group is unquoted--it gets quoted in the next commit. I missed that line as being part of lining up when I split it in to 2 commits.

eli-schwartz · 2018-03-16T01:39:53Z

cron-jobs/sourceballs

-		pkgarch=${pkginfo[2]}
-		pkglicense=(${pkginfo[@]:3})
-
+	while read -r pkgbase pkgver pkgarch pkglicense; do


pkglicense is now a string, rather than an array. It would still work as both licenses are in the (unquoted) array element 0, but of course that is now quoted.

So this only works if there is exactly one license in play.

LukeShu · 2018-03-16T02:35:25Z

I've just pushed v4 of the patchset

v4:

db-functions: fix lining up (relied on a change in a later commit)
testing2x: fix whitespace being introduced
parse_pkgbuilds.sh: have backup_package_variables output an
expression; remove restore_package_variables
sourceballs: pkglicense should be an array
test/: have a test fixture have multiple entries in license=()
Note in the commit message that the grep -q commit fixes an issue
by avoiding grep -v.

I believe this addresses each of Eli's comments. I did not switch to using bsdtar tf--I agree that it probably makes sense, but that's new work for another day.

LukeShu · 2018-03-16T03:25:21Z

I've just pushed v5 of this patchset. It's just rebasing on to
master.

v5:

Rewrite printf-formatter commit message, since most of this commit
was subsumed by 33aae31.
Fix the ${pkgs[@]@Q} mistake in Misc touchup #6 as part of the
fixups-near-quoting commit.

LukeShu · 2018-03-16T03:33:20Z

(whoops, deleted the wrong branch)

LukeShu · 2018-03-16T04:31:55Z

v6:

rebase, shuffle things affecting arch_repo_modify between commits

git diff between v5 and v6 comes up empty.

LukeShu · 2018-03-16T20:56:55Z

v7:

I had missed $_lockblock being used as a boolean/command in db-functions

eli-schwartz · 2018-03-20T03:30:12Z

Was looking at replacing the whole parse_pkgbuild.sh mess with python-srcinfo and a custom srcinfo generator, but then got sidetracked and had to debug this: https://lists.archlinux.org/pipermail/pacman-dev/2018-March/022428.html

yay...

eli-schwartz · 2018-03-20T04:00:18Z

db-functions

 }

 getpkgfiles() {
 	local f files
-	if [[ ! -z "$(echo ${@%\.*} | sed "s/ /\n/g" | sort | uniq -D)" ]]; then
+	if ! printf '%s\n' "${@%\.*}" | awk 'a[$0]++>1{exit 1}'; then


My initial suggestion did not have >1 in it, and this version does not work.

Aww crap, you're right. I added that when fixing the Travis failure the first night I submitted it ^{([comment] [build-log])}. In my sleepy state I was thinking ++a[$0], and when Travis came back clean in the morning, I didn't think about it again.

LukeShu · 2018-03-20T05:15:51Z

v8:

db-functions: getpkgfiles: fix awk expression to detect duplicates
test/: add a test to verify that getpkgfiles() rejects duplicates

LukeShu · 2018-03-22T01:55:30Z

v9:

rebase

I think you should at least pick the test/ commit, but I wouldn't be upset if you wanted to reject the others.

eli-schwartz · 2018-04-02T17:04:16Z

(Apologies for my initial response to the quoting changes, I did sort of overreact when I saw it breaking things due to excessive quoting. But that appears to be solved now, and it doesn't look like any other regressions are hiding.)

The rest look okay, I think. But I'm slightly confused what the test/ commit does, it seems to overlap the test case right above it?

Also apologies for the lateness of the response, since I'm now on holiday and somewhat erratically available.

LukeShu · 2018-04-03T01:49:09Z

The test above it checks that we detect when a package in $STAGING duplicates a package already in the repos. The new test checks that we detect duplicates when both packages are in $STAGING.

v10:

clean rebase, no changes

…same transaction This is a leftover change from the patch that became 0432cff; v2 of that patch broke db-functions:getpkgfiles, but the testsuite didn't catch it. This patch adds a testcase that catches this type of breakage.

Search for unquoted variables using the command: grep -Prn --exclude-dir=.git '(?<!["=]|\[\[ |\[\[ -[zn] )\$(?!{?#|\(|\? )' and ignore a bunch of false positives. You may verify that the only differences between the lines removed and lines added are double-quotes: diff -u \ <(git show|sed -n 's/^-//p'|grep -v '^-- a/'|sed 's/"//g') \ <(git show|sed -n 's/^+//p'|grep -v '^++ b/'|sed 's/"//g')

This does correct handling of - executing a program by symlink - any weird characters in the full path - I'm sure there's another case I thought about when I originally did this.

eli-schwartz · 2018-04-09T01:32:47Z

v11: remove veverything comments from commit messages

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from bb337fd to 795b654 Compare March 14, 2018 05:45

eli-schwartz suggested changes Mar 14, 2018

View reviewed changes

eli-schwartz self-assigned this Mar 14, 2018

eli-schwartz suggested changes Mar 14, 2018

View reviewed changes

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from 795b654 to 5526570 Compare March 14, 2018 21:18

eli-schwartz reviewed Mar 15, 2018

View reviewed changes

eli-schwartz reviewed Mar 16, 2018

View reviewed changes

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from 5526570 to 62c3daa Compare March 16, 2018 02:34

LukeShu mentioned this pull request Mar 16, 2018

Misc touchup #6

Merged

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from 62c3daa to ca1c034 Compare March 16, 2018 03:24

LukeShu closed this Mar 16, 2018

LukeShu deleted the lukeshu/rebase/archlinux+cleanup branch March 16, 2018 03:32

LukeShu restored the lukeshu/rebase/archlinux+cleanup branch March 16, 2018 03:32

LukeShu reopened this Mar 16, 2018

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from ca1c034 to e683a64 Compare March 16, 2018 04:30

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from e683a64 to f5774e4 Compare March 16, 2018 20:55

eli-schwartz reviewed Mar 20, 2018

View reviewed changes

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from f5774e4 to b351f03 Compare March 20, 2018 05:14

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from b351f03 to 5a6c4dd Compare March 22, 2018 01:53

LukeShu force-pushed the lukeshu/rebase/archlinux+cleanup branch from 5a6c4dd to eb09efd Compare April 3, 2018 01:38

LukeShu added 4 commits April 8, 2018 16:16

Consistently use "$(dirname "$(readlink -e "$0")")"

0cf2ac4

This does correct handling of - executing a program by symlink - any weird characters in the full path - I'm sure there's another case I thought about when I originally did this.

devlist-mailer: Make LIST and FROM configurable

656fd6d

eli-schwartz force-pushed the lukeshu/rebase/archlinux+cleanup branch from eb09efd to 656fd6d Compare April 9, 2018 01:31

eli-schwartz merged commit 656fd6d into archlinux:master Apr 9, 2018

LukeShu deleted the lukeshu/rebase/archlinux+cleanup branch April 10, 2018 00:42

Backports from Parabola #5

Backports from Parabola #5

Conversation

LukeShu commented Mar 14, 2018

LukeShu commented Mar 14, 2018

eli-schwartz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-schwartz Mar 14, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeShu commented Mar 14, 2018 • edited

LukeShu commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeShu commented Mar 16, 2018 • edited

LukeShu commented Mar 16, 2018

LukeShu commented Mar 16, 2018

LukeShu commented Mar 16, 2018

LukeShu commented Mar 16, 2018

eli-schwartz commented Mar 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukeShu commented Mar 20, 2018

LukeShu commented Mar 22, 2018

eli-schwartz commented Apr 2, 2018

LukeShu commented Apr 3, 2018

eli-schwartz commented Apr 9, 2018

eli-schwartz Mar 14, 2018 •

edited

LukeShu commented Mar 14, 2018 •

edited

LukeShu commented Mar 16, 2018 •

edited