[pacman-dev] [PATCH 01/11] makepkg: fix variable checks when writing pkginfo

older
[pacman-dev] [PATCH] makepkg: try...

Andres P

17 Jun 2010 17 Jun '10

12:44 p.m.

Regression in c71fe7db checked for wrong variables when populating .PKGINFO. Signed-off-by: Andres P --- scripts/makepkg.sh.in | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 678359f..d0b8b4b 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -923,14 +923,14 @@ write_pkginfo() { echo "force = true" fi - [[ $license ]] && printf "license = %s\n" "${license[@]}" - [[ $replaces ]] && printf "replaces = %s\n" "${replaces[@]}" - [[ $groups ]] && printf "group = %s\n" "${groups[@]}" - [[ $depends ]] && printf "depend = %s\n" "${depends[@]}" - [[ $optdepend ]] && printf "optdepend = %s\n" "${optdepends[@]}" - [[ $conflict ]] && printf "conflict = %s\n" "${conflicts[@]}" - [[ $provides ]] && printf "provides = %s\n" "${provides[@]}" - [[ $backup ]] && printf "backup = %s\n" "${backup[@]}" + [[ $license ]] && printf "license = %s\n" "${license[@]}" + [[ $replaces ]] && printf "replaces = %s\n" "${replaces[@]}" + [[ $groups ]] && printf "group = %s\n" "${groups[@]}" + [[ $depends ]] && printf "depend = %s\n" "${depends[@]}" + [[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]}" + [[ $conflicts ]] && printf "conflict = %s\n" "${conflicts[@]}" + [[ $provides ]] && printf "provides = %s\n" "${provides[@]}" + [[ $backup ]] && printf "backup = %s\n" "${backup[@]}" for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" -- 1.7.1

Show replies by date

Andres P

17 Jun 17 Jun

12:44 p.m.

New subject: [pacman-dev] [PATCH 02/11] makepkg: use declare -f's return val when checking for functions

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index d0b8b4b..28c7879 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1260,7 +1260,7 @@ check_sanity() { if (( ${#pkgname[@]} > 1 )); then for pkg in ${pkgname[@]}; do - if [[ $(type -t package_${pkg}) != "function" ]]; then + if declare -f package_${pkg} >/dev/null; then error "$(gettext "missing package function for split package '%s'")" "$pkg" return 1 fi @@ -1769,12 +1769,12 @@ if (( ${#pkgname[@]} > 1 )); then fi # test for available PKGBUILD functions -if [[ $(type -t build) = "function" ]]; then +if declare -f build >/dev/null; then BUILDFUNC=1 fi -if [[ $(type -t package) = "function" ]]; then +if declare -f package >/dev/null; then PKGFUNC=1 -elif [[ $SPLITPKG -eq 0 && $(type -t package_${pkgname}) = "function" ]]; then +elif [[ $SPLITPKG -eq 0 ]] && declare -f package_${pkgname} >/dev/null; then SPLITPKG=1 fi -- 1.7.1

Dan McGee

2:24 p.m.

New subject: [pacman-dev] [PATCH 02/11] makepkg: use declare -f's return val when checking for functions

On Thu, Jun 17, 2010 at 7:44 AM, Andres P wrote:

...

Signed-off-by: Andres P ---

Looks fine to me.

Andres P

12:44 p.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 28c7879..0bfb607 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -865,7 +865,7 @@ tidy_install() { done fi - if [[ $(check_option strip) = y && -n ${STRIP_DIRS[*]} ]]; then + if [[ $(check_option strip) = "y" && $arch != "any" && $STRIP_DIRS ]]; then msg2 "$(gettext "Stripping unneeded symbols from binaries and libraries...")" # make sure library stripping variables are defined to prevent excess stripping [[ -z ${STRIP_SHARED+x} ]] && STRIP_SHARED="-S" -- 1.7.1

Dan McGee

2:26 p.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

On Thu, Jun 17, 2010 at 7:44 AM, Andres P wrote:

...

Signed-off-by: Andres P ---

This is one of those "seems like a good idea but why" patches. Yes, we'll save milliseconds in building a package, but what if someone has a legit reason for putting libraries or binaries in an 'any' package? I'm going to -1 this one. -Dan

Andres P

2:55 p.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

On Thu, Jun 17, 2010 at 9:56 AM, Dan McGee wrote:

...

This is one of those "seems like a good idea but why" patches. Yes, we'll save milliseconds in building a package, but what if someone has a legit reason for putting libraries or binaries in an 'any' package? I'm going to -1 this one.

But "any" means that there's no arch dependant code in the package? strip(1) can't take the debug symbols out of a .py last time I checked. Seems like there's no organization. :/ Even namcap should throw an error if it finds an elf in an "-any" package. To think I was going to propose /libexec Andres P

Dan McGee

3:26 p.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

On Thu, Jun 17, 2010 at 9:55 AM, Andres P wrote:

...

On Thu, Jun 17, 2010 at 9:56 AM, Dan McGee wrote:

...
This is one of those "seems like a good idea but why" patches. Yes, we'll save milliseconds in building a package, but what if someone has a legit reason for putting libraries or binaries in an 'any' package? I'm going to -1 this one.

But "any" means that there's no arch dependant code in the package?

No, it means the package is able to be installed on any architecture and work as intended. What if I had an "elf-demo" package that contained different ELF files from multiple architectures? Yes, contrived, but possible.

...

strip(1) can't take the debug symbols out of a .py last time I checked.

Seems like there's no organization. :/

WTF?

...

Even namcap should throw an error if it finds an elf in an "-any" package.

It does, and I wrote the damn rule. But that is not for makepkg to decide. http://projects.archlinux.org/namcap.git/commit/?id=a121258fc2be1df8f970e09c...

...

To think I was going to propose /libexec

Seriously, your attitude is going to get you nowhere on this list. We appreciate your patches, but you can't throw a fit every time we say something negative. I don't even know what points you are trying to make or what agenda you are trying to push on a completely different topic. Address the issue your patch brings up, address the comments people give, but don't start spewing everything else, please. -Dan

Andres P

3:51 p.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

On Thu, Jun 17, 2010 at 10:56 AM, Dan McGee wrote:

...

On Thu, Jun 17, 2010 at 9:55 AM, Andres P wrote:

...
On Thu, Jun 17, 2010 at 9:56 AM, Dan McGee wrote:

...
This is one of those "seems like a good idea but why" patches. Yes, we'll save milliseconds in building a package, but what if someone has a legit reason for putting libraries or binaries in an 'any' package? I'm going to -1 this one.

But "any" means that there's no arch dependant code in the package?

No, it means the package is able to be installed on any architecture and work as intended. What if I had an "elf-demo" package that contained different ELF files from multiple architectures? Yes, contrived, but possible.

And for that corner case, people that have split in the opts array have to run it against -any packages that, suffice to say, are probably not going to benefit from the exception. So you're creating a what if for the least possible scenario. And if namcap has such checks, why not makepkg? It just gets very tedious to mantain a makepkg-ng on the side. Andres P

Nezmer

8:59 p.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

On Thu, Jun 17, 2010 at 11:21:26AM -0430, Andres P wrote:

...

On Thu, Jun 17, 2010 at 10:56 AM, Dan McGee wrote:

...
On Thu, Jun 17, 2010 at 9:55 AM, Andres P wrote:

...
On Thu, Jun 17, 2010 at 9:56 AM, Dan McGee wrote:

...
This is one of those "seems like a good idea but why" patches. Yes, we'll save milliseconds in building a package, but what if someone has a legit reason for putting libraries or binaries in an 'any' package? I'm going to -1 this one.

But "any" means that there's no arch dependant code in the package?

No, it means the package is able to be installed on any architecture and work as intended. What if I had an "elf-demo" package that contained different ELF files from multiple architectures? Yes, contrived, but possible.

And for that corner case, people that have split in the opts array have to run it against -any packages that, suffice to say, are probably not going to benefit from the exception.

So you're creating a what if for the least possible scenario.

It's not a hard-to-imagine corner case. Remember that we are talking i[3-6]86 and x86_64 here. The latter is capable of running the former binaries (As far as the IS is concerned). Some OSes still use i386 low level tools (e.g for booting) in x86_64/AMD64 systems. That's just one example. Some projects use self-contained wine. That's another example. I can provide more examples if you wish.

Andres P

18 Jun 18 Jun

2:34 a.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

On Thu, Jun 17, 2010 at 4:29 PM, Nezmer wrote:

...

It's not a hard-to-imagine corner case. Remember that we are talking i[3-6]86 and x86_64 here. The latter is capable of running the former binaries (As far as the IS is concerned).

Some OSes still use i386 low level tools (e.g for booting) in x86_64/AMD64 systems. That's just one example.

Some projects use self-contained wine. That's another example.

I can provide more examples if you wish.

As great as the example sounds, the vast majority of packages do not fall under that category. Not Arch's, nor Frugal's, etc. And for the few that do, they'll have debug symbols vs. the time gained from not checking for them in the rest of the -any packages' makepkg run. Until a repo reflects this necessity, it's esoteric because the situation is far in between... Andres P

Allan McRae

17 Jun 17 Jun

10:34 p.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

On 18/06/10 01:51, Andres P wrote:

...

On Thu, Jun 17, 2010 at 10:56 AM, Dan McGee wrote:

...
On Thu, Jun 17, 2010 at 9:55 AM, Andres P wrote:

...
On Thu, Jun 17, 2010 at 9:56 AM, Dan McGee wrote:

...
This is one of those "seems like a good idea but why" patches. Yes, we'll save milliseconds in building a package, but what if someone has a legit reason for putting libraries or binaries in an 'any' package? I'm going to -1 this one.

But "any" means that there's no arch dependant code in the package?

No, it means the package is able to be installed on any architecture and work as intended. What if I had an "elf-demo" package that contained different ELF files from multiple architectures? Yes, contrived, but possible.

And for that corner case, people that have split in the opts array have to run it against -any packages that, suffice to say, are probably not going to benefit from the exception.

So you're creating a what if for the least possible scenario.

This patch enforces no stripping on a arch=any package. All it takes is one real work exception to the thinking that arch=any packages can not contain binaries and then we would need to revert that patch. Saying that, even the contrived examples present in this thread so far should not have the host system strip used on the binaries so you require options=(!strip) added to the PKGBUILD anyway. So I am leaning in favour of including that patch until someone presents an actual case where it is wrong.

...

And if namcap has such checks, why not makepkg?

makepkg makes packages, namcap checks packages. The distinction seems clear.

...

It just gets very tedious to mantain a makepkg-ng on the side.

Then you need to learn to use git better. I maintained a patch set for at least six months in the last release cycle before they got accepted into mainline.

Andres P

18 Jun 18 Jun

2:44 a.m.

New subject: [pacman-dev] [PATCH 03/11] makepkg: do not attempt to strip arch agnostic packages

On Thu, Jun 17, 2010 at 6:04 PM, Allan McRae wrote:

...

...
And if namcap has such checks, why not makepkg?

makepkg makes packages, namcap checks packages. The distinction seems clear.

No, it doesn't seem clear. You know as well as I do that makepkg has a great deal of logic pertaining to checking packages that go farther than an arch check before stripping debug symbols. It checks to see wether pkgver and the like have illegal characters, the same with other fields. With the exception of a package name starting with a '-' character, since that check has to do with operands and calling makepkg with --pkg foo. It's also very strange that you draw this destinction as black and white when there's been patches posted in the last ~48 hours related to makepkg 'sanity' checks that have to do with this very subject. And without bringing those up; you agree with the arch check yet that's where the whole namcap mention (and later, comparison) came in? Not making a lot of sense.

...

...
It just gets very tedious to mantain a makepkg-ng on the side.

Then you need to learn to use git better. I maintained a patch set for at least six months in the last release cycle before they got accepted into mainline.

That's well besides the point. ;) Learning git better won't magically teach me how to sit down and sweettalk people over the net. It seems ridiculous that I have to go out of my way quoting man sudoers and other stupid stuff for things that I know that are broken. It'd be much more productive if, say, people would check what they're saying first? Andres P

Andres P

17 Jun 17 Jun

12:44 p.m.

New subject: [pacman-dev] [PATCH 04/11] makepkg: less code duplication in create_package

Combine changelog and install file creation as in previous commits. Signed-off-by: Andres P --- scripts/makepkg.sh.in | 26 +++++++++++--------------- 1 files changed, 11 insertions(+), 15 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 0bfb607..37241bd 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -996,21 +996,17 @@ create_package() { local comp_files=".PKGINFO" - # check for an install script - if [[ -n $install ]]; then - msg2 "$(gettext "Adding install script...")" - cp "$startdir/$install" .INSTALL - chmod 644 .INSTALL - comp_files="$comp_files .INSTALL" - fi - - # do we have a changelog? - if [[ -n $changelog ]]; then - msg2 "$(gettext "Adding package changelog...")" - cp "$startdir/$changelog" .CHANGELOG - chmod 644 .CHANGELOG - comp_files="$comp_files .CHANGELOG" - fi + # check for changelog/install files + for i in 'changelog' 'install'; do + orig="${!i}" + dest=".${i^^}" + if [[ -n $orig ]]; then + msg2 "$(gettext "Adding %s file...")" "$i" + cp "$startdir/$orig" "$dest" + chmod 644 "$dest" + comp_files+=" $dest" + fi + done # tar it up msg2 "$(gettext "Compressing package...")" -- 1.7.1

Allan McRae

20 Jun 20 Jun

10:39 a.m.

New subject: [pacman-dev] [PATCH 04/11] makepkg: less code duplication in create_package

On 17/06/10 22:44, Andres P wrote:

...

Combine changelog and install file creation as in previous commits.

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 26 +++++++++++--------------- 1 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 0bfb607..37241bd 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -996,21 +996,17 @@ create_package() {

local comp_files=".PKGINFO"

- # check for an install script - if [[ -n $install ]]; then - msg2 "$(gettext "Adding install script...")" - cp "$startdir/$install" .INSTALL - chmod 644 .INSTALL - comp_files="$comp_files .INSTALL" - fi - - # do we have a changelog? - if [[ -n $changelog ]]; then - msg2 "$(gettext "Adding package changelog...")" - cp "$startdir/$changelog" .CHANGELOG - chmod 644 .CHANGELOG - comp_files="$comp_files .CHANGELOG" - fi + # check for changelog/install files + for i in 'changelog' 'install'; do + orig="${!i}" + dest=".${i^^}"

That is a bash4 command so will break cygwin compatibility (which currently has bash-3.2). Resubmit with something like "tr [:lower:] [:upper:]" usage. (I know the ${i^^} is used elsewhere, but that slipped passed my notice and will be being reverted.

...

+ if [[ -n $orig ]]; then + msg2 "$(gettext "Adding %s file...")" "$i" + cp "$startdir/$orig" "$dest" + chmod 644 "$dest" + comp_files+=" $dest" + fi + done

# tar it up msg2 "$(gettext "Compressing package...")"

Andres P

17 Jun 17 Jun

12:44 p.m.

New subject: [pacman-dev] [PATCH 05/11] makepkg: undeclared local variables

Tight variable scoping should avoid further regressions with new patches and variable overriding (see what ac5c2fd09 fixed). Signed-off-by: Andres P --- scripts/makepkg.sh.in | 30 ++++++++++++++++++++++-------- 1 files changed, 22 insertions(+), 8 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 37241bd..630d9c2 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -146,12 +146,14 @@ clean_up() { if (( PKGFUNC )); then rm -f "${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-package.log"* elif (( SPLITPKG )); then + local pkg for pkg in ${pkgname[@]}; do rm -f "${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-package_${pkg}.log"* done fi # clean up dangling symlinks to packages + local file for pkg in ${pkgname[@]}; do for file in ${pkg}-*-*-${CARCH}${PKGEXT}; do if [[ -h $file && ! -e $file ]]; then @@ -308,7 +310,7 @@ get_downloadclient() { for i in "${DLAGENTS[@]}"; do local handler="${i%%::*}" if [[ $proto = $handler ]]; then - agent="${i##*::}" + local agent="${i##*::}" break fi done @@ -384,6 +386,9 @@ run_pacman() { check_deps() { (( $# > 0 )) || return + # XXX: Due to a bash bug, pmout's subshell cannot be declared sensibly: + # local pmout=$(run_pacman -T "$@") + local pmout local ret=0 pmout=$(run_pacman -T "$@") ret=$? @@ -651,7 +656,7 @@ extract_sources() { msg "$(gettext "Extracting Sources...")" local netfile for netfile in "${source[@]}"; do - file=$(get_filename "$netfile") + local file=$(get_filename "$netfile") if in_array "$file" ${noextract[@]}; then #skip source files in the noextract=() array # these are marked explicitly to NOT be extracted @@ -727,7 +732,7 @@ run_function() { if [[ -z $1 ]]; then return 1 fi - pkgfunc="$1" + local pkgfunc="$1" # clear user-specified makeflags if requested if [[ $(check_option makeflags) = "n" ]]; then @@ -743,8 +748,9 @@ run_function() { local shellopts=$(shopt -p) local ret=0 + local restoretrap if (( LOGGING )); then - BUILDLOG="${startdir}/${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-$pkgfunc.log" + local BUILDLOG="${startdir}/${pkgbase}-${pkgver}-${pkgrel}-${CARCH}-$pkgfunc.log" if [[ -f $BUILDLOG ]]; then local i=1 while true; do @@ -799,6 +805,7 @@ run_build() { } run_package() { + local pkgfunc if [[ -z $1 ]]; then pkgfunc="package" else @@ -932,6 +939,7 @@ write_pkginfo() { [[ $provides ]] && printf "provides = %s\n" "${provides[@]}" [[ $backup ]] && printf "backup = %s\n" "${backup[@]}" + local it for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" if [[ $ret != "?" ]]; then @@ -980,6 +988,7 @@ create_package() { cd "$pkgdir" msg "$(gettext "Creating package...")" + local nameofpkg if [[ -z $1 ]]; then nameofpkg="$pkgname" else @@ -997,6 +1006,7 @@ create_package() { local comp_files=".PKGINFO" # check for changelog/install files + local i orig dest for i in 'changelog' 'install'; do orig="${!i}" dest=".${i^^}" @@ -1011,6 +1021,7 @@ create_package() { # tar it up msg2 "$(gettext "Compressing package...")" + local EXT case "$PKGEXT" in *tar.gz) EXT=${PKGEXT%.gz} ;; *tar.bz2) EXT=${PKGEXT%.bz2} ;; @@ -1135,7 +1146,7 @@ install_package() { msg "$(gettext "Installing %s package group with %s -U...")" "$pkgbase" "$PACMAN" fi - local pkglist + local pkg pkglist for pkg in ${pkgname[@]}; do if [[ -f $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT} ]]; then pkglist="${pkglist} $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT}" @@ -1215,7 +1226,7 @@ check_sanity() { local optdepend for optdepend in "${optdepends[@]}"; do - pkg=${optdepend%%:*} + local pkg=${optdepend%%:*} if [[ ! $pkg =~ ^[[:alnum:]\>\<\=\.\+\_\-]*$ ]]; then error "$(gettext "Invalid syntax for optdepend : '%s'")" "$optdepend" fi @@ -1360,15 +1371,17 @@ devel_update() { } backup_package_variables() { + local var for var in ${splitpkg_overrides[@]}; do - indirect="${var}_backup" + local indirect="${var}_backup" eval "${indirect}=(\"\${$var[@]}\")" done } restore_package_variables() { + local var for var in ${splitpkg_overrides[@]}; do - indirect="${var}_backup" + local indirect="${var}_backup" if [[ -n ${!indirect} ]]; then eval "${var}=(\"\${$indirect[@]}\")" else @@ -1383,6 +1396,7 @@ parse_options() { local long_options=$1; shift; local ret=0; local unused_options="" + local i while [[ -n $1 ]]; do if [[ ${1:0:2} = '--' ]]; then -- 1.7.1

Allan McRae

1:25 p.m.

New subject: [pacman-dev] [PATCH 05/11] makepkg: undeclared local variables

On 17/06/10 22:44, Andres P wrote:

...

Tight variable scoping should avoid further regressions with new patches and variable overriding (see what ac5c2fd09 fixed).

Variable scoping had nothing to do with what ac5c2fd09 fixed. Maybe you mean another commit?

Andres P

1:33 p.m.

New subject: [pacman-dev] [PATCH 05/11] makepkg: undeclared local variables

On Thu, Jun 17, 2010 at 8:55 AM, Allan McRae wrote:

...

On 17/06/10 22:44, Andres P wrote:

...
Tight variable scoping should avoid further regressions with new patches and variable overriding (see what ac5c2fd09 fixed).

Variable scoping had nothing to do with what ac5c2fd09 fixed. Maybe you mean another commit?

It had everything to do with it. Functions are big as it is, meaning the namespace is pretty cramped. By chance there was a variable named changelog and install in scope. Do remind that the original suggestion was to make that a seperate function, giving it its own set of local variables. Andres P

Allan McRae

1:45 p.m.

New subject: [pacman-dev] [PATCH 05/11] makepkg: undeclared local variables

On 17/06/10 23:33, Andres P wrote:

...

On Thu, Jun 17, 2010 at 8:55 AM, Allan McRae wrote:

...
On 17/06/10 22:44, Andres P wrote:

...
Tight variable scoping should avoid further regressions with new patches and variable overriding (see what ac5c2fd09 fixed).

Variable scoping had nothing to do with what ac5c2fd09 fixed. Maybe you mean another commit?

It had everything to do with it. Functions are big as it is, meaning the namespace is pretty cramped.

By chance there was a variable named changelog and install in scope.

Do remind that the original suggestion was to make that a seperate function, giving it its own set of local variables.

As I said, this had nothing to do with ac5c2fd09. Maybe you mean b8863622? Even then the issue was only in the case where multiple changelog or install files were specified in the PKGBUILD which the "$i=$(sed...)" syntax did not handle. Allan

Allan McRae

20 Jun 20 Jun

12:13 p.m.

New subject: [pacman-dev] [PATCH 05/11] makepkg: undeclared local variables

On 17/06/10 22:44, Andres P wrote:

...

Tight variable scoping should avoid further regressions with new patches and variable overriding (see what ac5c2fd09 fixed).

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 30 ++++++++++++++++++++++-------- 1 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 37241bd..630d9c2 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in

<snip>

...

@@ -384,6 +386,9 @@ run_pacman() { check_deps() { (( $#> 0 )) || return

+ # XXX: Due to a bash bug, pmout's subshell cannot be declared sensibly: + # local pmout=$(run_pacman -T "$@")

Can you explain that comment?

...

+ local pmout local ret=0 pmout=$(run_pacman -T "$@") ret=$? @@ -651,7 +656,7 @@ extract_sources() { msg "$(gettext "Extracting Sources...")" local netfile for netfile in "${source[@]}"; do - file=$(get_filename "$netfile") + local file=$(get_filename "$netfile")

And why it does not apply here...

...

if in_array "$file" ${noextract[@]}; then #skip source files in the noextract=() array # these are marked explicitly to NOT be extracted

These are the sort of things that are really good to explain in commit messages. Allan

Andres P

4:04 p.m.

New subject: [pacman-dev] [PATCH 05/11] makepkg: undeclared local variables

On Sun, Jun 20, 2010 at 7:43 AM, Allan McRae wrote:

...

On 17/06/10 22:44, Andres P wrote:

...
Tight variable scoping should avoid further regressions with new patches and variable overriding (see what ac5c2fd09 fixed).

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 30 ++++++++++++++++++++++-------- 1 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 37241bd..630d9c2 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in

<snip>

...
@@ -384,6 +386,9 @@ run_pacman() { check_deps() { (( $#> 0 )) || return

+ # XXX: Due to a bash bug, pmout's subshell cannot be declared sensibly: + # local pmout=$(run_pacman -T "$@")

Can you explain that comment?

...
+ local pmout local ret=0 pmout=$(run_pacman -T "$@") ret=$? @@ -651,7 +656,7 @@ extract_sources() { msg "$(gettext "Extracting Sources...")" local netfile for netfile in "${source[@]}"; do - file=$(get_filename "$netfile") + local file=$(get_filename "$netfile")

And why it does not apply here...

Because get_filename isn't being checked for a return value. I posted this on yesterday's patch's comments, but I guess it should be in a comment aswell: $ fn() { foo=$(false); echo $?; local bar=$(false); echo $?; } $ fn 1 0 This means that local bar=$(false), or local bar=$(exit 255) returns 0. It would be more organized to change all local subshells instead of spending time figuring out if it gets checked for a return value or not. But that belongs in a style guide type document or something... Just yesterday I saw someone submit a patch with [[ $(type -t "foo") == "function" ]], so I'm guessing there's going to be more of these flashbacks. Andres P

Andres P

17 Jun 17 Jun

12:44 p.m.

New subject: [pacman-dev] [PATCH 06/11] makepkg: less code repetition for emtpy variable checking

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 19 +++++++------------ 1 files changed, 7 insertions(+), 12 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 630d9c2..23e3b36 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1163,18 +1163,13 @@ install_package() { check_sanity() { # check for no-no's in the build script - if [[ -z $pkgname ]]; then - error "$(gettext "%s is not allowed to be empty.")" "pkgname" - return 1 - fi - if [[ -z $pkgver ]]; then - error "$(gettext "%s is not allowed to be empty.")" "pkgver" - return 1 - fi - if [[ -z $pkgrel ]]; then - error "$(gettext "%s is not allowed to be empty.")" "pkgrel" - return 1 - fi + local i + for i in 'pkgname' 'pkgrel' 'pkgver'; do + if [[ -z ${!i} ]]; then + error "$(gettext "%s is not allowed to be empty.")" "$i" + return 1 + fi + done local name for name in "${pkgname[@]}"; do -- 1.7.1

Dan McGee

2:29 p.m.

New subject: [pacman-dev] [PATCH 06/11] makepkg: less code repetition for emtpy variable checking

On Thu, Jun 17, 2010 at 7:44 AM, Andres P wrote:

...

Signed-off-by: Andres P ---

Looks good, outside of the spelling error in the title. emtpy -> empty

...

scripts/makepkg.sh.in | 19 +++++++------------ 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 630d9c2..23e3b36 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1163,18 +1163,13 @@ install_package() {

check_sanity() { # check for no-no's in the build script - if [[ -z $pkgname ]]; then - error "$(gettext "%s is not allowed to be empty.")" "pkgname" - return 1 - fi - if [[ -z $pkgver ]]; then - error "$(gettext "%s is not allowed to be empty.")" "pkgver" - return 1 - fi - if [[ -z $pkgrel ]]; then - error "$(gettext "%s is not allowed to be empty.")" "pkgrel" - return 1 - fi + local i + for i in 'pkgname' 'pkgrel' 'pkgver'; do + if [[ -z ${!i} ]]; then + error "$(gettext "%s is not allowed to be empty.")" "$i" + return 1 + fi + done

local name for name in "${pkgname[@]}"; do -- 1.7.1

Andres P

12:44 p.m.

New subject: [pacman-dev] [PATCH 07/11] makepkg: less code repetition when sanitizing variable contents

During check_sanity, use regex and abstract the series of variable checks into a list. Signed-off-by: Andres P --- scripts/makepkg.sh.in | 70 +++++++++++++++++++----------------------------- 1 files changed, 28 insertions(+), 42 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 23e3b36..991ad0f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1161,6 +1161,19 @@ install_package() { fi } +var_lint() { + local pattern="$1" + local directive="$2" + shift 2 + + local i + for i; do + [[ $i =~ $pattern ]] || continue + error "$(gettext "'%s' is an invalid value for %s")" "$i" "$directive" + return 1 + done +} + check_sanity() { # check for no-no's in the build script local i @@ -1170,28 +1183,25 @@ check_sanity() { return 1 fi done + + local i + for i in 'pkgver' 'pkgrel'; do + var_lint '-' $i "${!i}" || return + done - local name - for name in "${pkgname[@]}"; do - if [[ ${name:0:1} = "-" ]]; then - error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgname" - return 1 + var_lint '^-' pkgname "${pkgname[@]}" || return + var_lint '^-' pkgbase "${pkgbase}" || return + var_lint '[<>]' provides "${provides[@]}" || return + var_lint '^/' backup "${backup[@]}" || return + + local optdepend + for optdepend in "${optdepends[@]}"; do + local pkg=${optdepend%%:*} + if [[ ! $pkg =~ ^[[:alnum:]\>\<\=\.\+\_\-]*$ ]]; then + error "$(gettext "Invalid syntax for optdepend : '%s'")" "$optdepend" fi done - if [[ ${pkgbase:0:1} = "-" ]]; then - error "$(gettext "%s is not allowed to start with a hyphen.")" "pkgbase" - return 1 - fi - if [[ $pkgver != ${pkgver//-/} ]]; then - error "$(gettext "%s is not allowed to contain hyphens.")" "pkgver" - return 1 - fi - if [[ $pkgrel != ${pkgrel//-/} ]]; then - error "$(gettext "%s is not allowed to contain hyphens.")" "pkgrel" - return 1 - fi - if [[ $arch != 'any' ]]; then if ! in_array $CARCH ${arch[@]}; then if (( ! IGNOREARCH )); then @@ -1203,30 +1213,6 @@ check_sanity() { fi fi - local provide - for provide in ${provides[@]}; do - if [[ $provide != ${provide///} ]]; then - error "$(gettext "Provides array cannot contain comparison (< or >) operators.")" - return 1 - fi - done - - local file - for file in "${backup[@]}"; do - if [[ ${file:0:1} = "/" ]]; then - error "$(gettext "Invalid backup entry : %s")" "$file" - return 1 - fi - done - - local optdepend - for optdepend in "${optdepends[@]}"; do - local pkg=${optdepend%%:*} - if [[ ! $pkg =~ ^[[:alnum:]\>\<\=\.\+\_\-]*$ ]]; then - error "$(gettext "Invalid syntax for optdepend : '%s'")" "$optdepend" - fi - done - local i for i in 'changelog' 'install'; do local filelist=$(sed -n "s/^[[:space:]]*$i=//p" "$BUILDFILE") -- 1.7.1

Allan McRae

1:22 p.m.

New subject: [pacman-dev] [PATCH 07/11] makepkg: less code repetition when sanitizing variable contents

On 17/06/10 22:44, Andres P wrote:

...

During check_sanity, use regex and abstract the series of variable checks into a list.

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 70 +++++++++++++++++++----------------------------- 1 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 23e3b36..991ad0f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1161,6 +1161,19 @@ install_package() { fi }

+var_lint() { + local pattern="$1" + local directive="$2" + shift 2 + + local i + for i; do + [[ $i =~ $pattern ]] || continue + error "$(gettext "'%s' is an invalid value for %s")" "$i" "$directive" + return 1 + done +}

I am against this as the error messages are no longer informative. Allan

Andres P

1:53 p.m.

New subject: [pacman-dev] [PATCH 07/11] makepkg: less code repetition when sanitizing variable contents

On Thu, Jun 17, 2010 at 8:52 AM, Allan McRae wrote:

...

On 17/06/10 22:44, Andres P wrote:

...
During check_sanity, use regex and abstract the series of variable checks into a list.

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 70 +++++++++++++++++++----------------------------- 1 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 23e3b36..991ad0f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1161,6 +1161,19 @@ install_package() { fi }

+var_lint() { + local pattern="$1" + local directive="$2" + shift 2 + + local i + for i; do + [[ $i =~ $pattern ]] || continue + error "$(gettext "'%s' is an invalid value for %s")" "$i" "$directive" + return 1 + done +}

I am against this as the error messages are no longer informative.

Allan

Well, the error message would be the least of worries now that it's in one place instead of >= 7. What type of error message would be informative? "variable %s may not match regex %s" And if makepkg has code repetition because of documentation, then the man page out to be fixed? Not that the error message is less descriptive as it is anyhow. Andres P

Dan McGee

2:09 p.m.

New subject: [pacman-dev] [PATCH 07/11] makepkg: less code repetition when sanitizing variable contents

On Thu, Jun 17, 2010 at 8:53 AM, Andres P wrote:

...

On Thu, Jun 17, 2010 at 8:52 AM, Allan McRae wrote:

...
On 17/06/10 22:44, Andres P wrote:

...
During check_sanity, use regex and abstract the series of variable checks into a list.

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 70 +++++++++++++++++++----------------------------- 1 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 23e3b36..991ad0f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1161,6 +1161,19 @@ install_package() { fi }

+var_lint() { + local pattern="$1" + local directive="$2" + shift 2 + + local i + for i; do + [[ $i =~ $pattern ]] || continue + error "$(gettext "'%s' is an invalid value for %s")" "$i" "$directive" + return 1 + done +}

I am against this as the error messages are no longer informative.

Allan

Well, the error message would be the least of worries now that it's in one place instead of >= 7.

What type of error message would be informative?

"variable %s may not match regex %s"

Helpful error messages are what is there now- I understand the want to reduce the repetition but not at the expense of the user understanding what was wrong. I think this message is better but still not ideal as you then have to decipher the regex.

...

And if makepkg has code repetition because of documentation, then the man page out to be fixed? Not that the error message is less descriptive as it is anyhow.

I don't follow what you are saying here- I think we have a language barrier. What does the manpage have to do with anything? -Dan

Andres P

2:24 p.m.

New subject: [pacman-dev] [PATCH 07/11] makepkg: less code repetition when sanitizing variable contents

On Thu, Jun 17, 2010 at 9:39 AM, Dan McGee wrote:

...

Helpful error messages are what is there now- I understand the want to reduce the repetition but not at the expense of the user understanding what was wrong. I think this message is better but still not ideal as you then have to decipher the regex.

That's just a random idea. I could restore the messages and keep the rep down... but first there *has* to be an incentive to not copy paste code. If we agree that it needs less copy paste then we can work an error message out, which is the least difficult thing to do.

...

...
And if makepkg has code repetition because of documentation, then the man page out to be fixed? Not that the error message is less descriptive as it is anyhow.

I don't follow what you are saying here- I think we have a language barrier. What does the manpage have to do with anything?

I'm implying that these verbose descriptions would fit better in the manpage. The manpage would have a section on explaining what and what cannot go in a field. Whereas makepkg will just throw an error with the string that matched the pattern. Again, that is contradictory to what I posted in reply above... but it can be worked out *if* there's interest in less copy paste. I'm percieving that there is not. Andres P

Allan McRae

2:16 p.m.

New subject: [pacman-dev] [PATCH 07/11] makepkg: less code repetition when sanitizing variable contents

On 17/06/10 23:53, Andres P wrote:

...

On Thu, Jun 17, 2010 at 8:52 AM, Allan McRae wrote:

...
On 17/06/10 22:44, Andres P wrote:

...
During check_sanity, use regex and abstract the series of variable checks into a list.

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 70 +++++++++++++++++++----------------------------- 1 files changed, 28 insertions(+), 42 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 23e3b36..991ad0f 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1161,6 +1161,19 @@ install_package() { fi }

+var_lint() { + local pattern="$1" + local directive="$2" + shift 2 + + local i + for i; do + [[ $i =~ $pattern ]] || continue + error "$(gettext "'%s' is an invalid value for %s")" "$i" "$directive" + return 1 + done +}

I am against this as the error messages are no longer informative.

Allan

Well, the error message would be the least of worries now that it's in one place instead of>= 7.

What type of error message would be informative?

"variable %s may not match regex %s"

And if makepkg has code repetition because of documentation, then the man page out to be fixed? Not that the error message is less descriptive as it is anyhow.

The new error message is much less descriptive; "pkgname is not allowed to start with a hyphen" is very clear, while "'-foo' is an invalid value for pkgname" does not tell me what is wrong. Using the regex in the error message is just going to confuse the hell out of people too. Reading an error message should not require you know regexes. And your quest to reduce code duplication can go to the point of being nonsensical. e.g. in this patch:

...

+ + local i + for i in 'pkgver' 'pkgrel'; do + var_lint '-' $i "${!i}" || return + done

That would be two lines without the loop...

Andres P

2:18 p.m.

New subject: [pacman-dev] [PATCH 07/11] makepkg: less code repetition when sanitizing variable contents

On Thu, Jun 17, 2010 at 9:46 AM, Allan McRae wrote:

...

The new error message is much less descriptive; "pkgname is not allowed to start with a hyphen" is very clear, while "'-foo' is an invalid value for pkgname" does not tell me what is wrong.

Using the regex in the error message is just going to confuse the hell out of people too. Reading an error message should not require you know regexes.

It is a two character regex; how are people expected to use makepkg and not know what ^- means? Besides that, the message in place *isn't* a regex.

...

And your quest to reduce code duplication can go to the point of being nonsensical. e.g. in this patch:

...
+ + local i + for i in 'pkgver' 'pkgrel'; do + var_lint '-' $i "${!i}" || return + done

That would be two lines without the loop...

You're thinking in line count whereas code repetition is about duplicating logic. What if you add a new field, do you copy paste it or add it to the loop? Quoting a slice of the patch when the vast majority of the changes are happening elsewere isn't helping things either. You missed a quote from that diff that's right on the top: 1 files changed, 28 insertions(+), 42 deletions(-) In other words, there's less lines, Allan. Andres P

Andres P

12:44 p.m.

New subject: [pacman-dev] [PATCH 08/11] makepkg: add to variables without expanding them

Use foo+=bar instead of foo=${foo}bar Signed-off-by: Andres P --- scripts/makepkg.sh.in | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 991ad0f..cf23fdb 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1149,9 +1149,9 @@ install_package() { local pkg pkglist for pkg in ${pkgname[@]}; do if [[ -f $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT} ]]; then - pkglist="${pkglist} $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT}" + pkglist+=" $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT}" else - pkglist="${pkglist} $PKGDEST/${pkg}-${pkgver}-${pkgrel}-any${PKGEXT}" + pkglist+=" $PKGDEST/${pkg}-${pkgver}-${pkgrel}-any${PKGEXT}" fi done @@ -1521,11 +1521,11 @@ ARGLIST=("$@") # Parse Command Line Options. OPT_SHORT="AcCdefFghiLmop:rRsV" OPT_LONG="allsource,asroot,ignorearch,clean,cleancache,nodeps" -OPT_LONG="$OPT_LONG,noextract,force,forcever:,geninteg,help,holdver" -OPT_LONG="$OPT_LONG,install,log,nocolor,nobuild,pkg:,rmdeps,repackage,skipinteg" -OPT_LONG="$OPT_LONG,source,syncdeps,version,config:" +OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver" +OPT_LONG+=",install,log,nocolor,nobuild,pkg:,rmdeps,repackage,skipinteg" +OPT_LONG+=",source,syncdeps,version,config:" # Pacman Options -OPT_LONG="$OPT_LONG,noconfirm,noprogressbar" +OPT_LONG+=",noconfirm,noprogressbar" OPT_TEMP="$(parse_options $OPT_SHORT $OPT_LONG "$@" || echo 'PARSE_OPTIONS FAILED')" if [[ $OPT_TEMP = *'PARSE_OPTIONS FAILED'* ]]; then # This is a small hack to stop the script bailing with 'set -e' @@ -1537,8 +1537,8 @@ unset OPT_SHORT OPT_LONG OPT_TEMP while true; do case "$1" in # Pacman Options - --noconfirm) PACMAN_OPTS="$PACMAN_OPTS --noconfirm" ;; - --noprogressbar) PACMAN_OPTS="$PACMAN_OPTS --noprogressbar" ;; + --noconfirm) PACMAN_OPTS+=" --noconfirm" ;; + --noprogressbar) PACMAN_OPTS+=" --noprogressbar" ;; # Makepkg Options --allsource) SOURCEONLY=2 ;; -- 1.7.1

Dan McGee

2:32 p.m.

New subject: [pacman-dev] [PATCH 08/11] makepkg: add to variables without expanding them

On Thu, Jun 17, 2010 at 7:44 AM, Andres P wrote:

...

Use foo+=bar instead of foo=${foo}bar

Signed-off-by: Andres P ---

Signed-off-by: Dan McGee

...

scripts/makepkg.sh.in | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 991ad0f..cf23fdb 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1149,9 +1149,9 @@ install_package() { local pkg pkglist for pkg in ${pkgname[@]}; do if [[ -f $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT} ]]; then - pkglist="${pkglist} $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT}" + pkglist+=" $PKGDEST/${pkg}-${pkgver}-${pkgrel}-${CARCH}${PKGEXT}" else - pkglist="${pkglist} $PKGDEST/${pkg}-${pkgver}-${pkgrel}-any${PKGEXT}" + pkglist+=" $PKGDEST/${pkg}-${pkgver}-${pkgrel}-any${PKGEXT}" fi done

@@ -1521,11 +1521,11 @@ ARGLIST=("$@") # Parse Command Line Options. OPT_SHORT="AcCdefFghiLmop:rRsV" OPT_LONG="allsource,asroot,ignorearch,clean,cleancache,nodeps" -OPT_LONG="$OPT_LONG,noextract,force,forcever:,geninteg,help,holdver" -OPT_LONG="$OPT_LONG,install,log,nocolor,nobuild,pkg:,rmdeps,repackage,skipinteg" -OPT_LONG="$OPT_LONG,source,syncdeps,version,config:" +OPT_LONG+=",noextract,force,forcever:,geninteg,help,holdver" +OPT_LONG+=",install,log,nocolor,nobuild,pkg:,rmdeps,repackage,skipinteg" +OPT_LONG+=",source,syncdeps,version,config:" # Pacman Options -OPT_LONG="$OPT_LONG,noconfirm,noprogressbar" +OPT_LONG+=",noconfirm,noprogressbar" OPT_TEMP="$(parse_options $OPT_SHORT $OPT_LONG "$@" || echo 'PARSE_OPTIONS FAILED')" if [[ $OPT_TEMP = *'PARSE_OPTIONS FAILED'* ]]; then # This is a small hack to stop the script bailing with 'set -e' @@ -1537,8 +1537,8 @@ unset OPT_SHORT OPT_LONG OPT_TEMP while true; do case "$1" in # Pacman Options - --noconfirm) PACMAN_OPTS="$PACMAN_OPTS --noconfirm" ;; - --noprogressbar) PACMAN_OPTS="$PACMAN_OPTS --noprogressbar" ;; + --noconfirm) PACMAN_OPTS+=" --noconfirm" ;; + --noprogressbar) PACMAN_OPTS+=" --noprogressbar" ;;

# Makepkg Options --allsource) SOURCEONLY=2 ;; -- 1.7.1

Andres P

12:44 p.m.

New subject: [pacman-dev] [PATCH 09/11] makepkg: merge in-line splitpkg handling logic

Ease maintainace; the two parts that have been combined into a function were identical. Signed-off-by: Andres P --- scripts/makepkg.sh.in | 38 ++++++++++++++++---------------------- 1 files changed, 16 insertions(+), 22 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cf23fdb..6de6100 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1371,6 +1371,20 @@ restore_package_variables() { done } +handle_splitpkg() { + for pkg in ${pkgname[@]}; do + pkgdir="$pkgdir/$pkg" + mkdir -p "$pkgdir" + chmod a-s "$pkgdir" + backup_package_variables + run_package $pkg + tidy_install + create_package $pkg + restore_package_variables + pkgdir="${pkgdir%/*}" + done +} + # getopt like parser parse_options() { local short_options=$1; shift; @@ -1838,17 +1852,7 @@ if (( INFAKEROOT )); then fi create_package else - for pkg in ${pkgname[@]}; do - pkgdir="$pkgdir/$pkg" - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" - backup_package_variables - run_package $pkg - tidy_install - create_package $pkg - restore_package_variables - pkgdir="${pkgdir%/*}" - done + handle_splitpkg fi msg "$(gettext "Leaving fakeroot environment.")" @@ -1966,17 +1970,7 @@ else fi create_package else - for pkg in ${pkgname[@]}; do - pkgdir="$pkgdir/$pkg" - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" - backup_package_variables - run_package $pkg - tidy_install - create_package $pkg - restore_package_variables - pkgdir="${pkgdir%/*}" - done + handle_splitpkg fi else if (( ! REPKG && ( PKGFUNC || SPLITPKG ) )); then -- 1.7.1

Dan McGee

2:34 p.m.

New subject: [pacman-dev] [PATCH 09/11] makepkg: merge in-line splitpkg handling logic

On Thu, Jun 17, 2010 at 7:44 AM, Andres P wrote:

...

Ease maintainace; the two parts that have been combined into a function were identical.

Signed-off-by: Andres P ---

Seems reasonable. Signed-off-by: Dan McGee

...

scripts/makepkg.sh.in | 38 ++++++++++++++++---------------------- 1 files changed, 16 insertions(+), 22 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cf23fdb..6de6100 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1371,6 +1371,20 @@ restore_package_variables() { done }

+handle_splitpkg() { + for pkg in ${pkgname[@]}; do + pkgdir="$pkgdir/$pkg" + mkdir -p "$pkgdir" + chmod a-s "$pkgdir" + backup_package_variables + run_package $pkg + tidy_install + create_package $pkg + restore_package_variables + pkgdir="${pkgdir%/*}" + done +} + # getopt like parser parse_options() { local short_options=$1; shift; @@ -1838,17 +1852,7 @@ if (( INFAKEROOT )); then fi create_package else - for pkg in ${pkgname[@]}; do - pkgdir="$pkgdir/$pkg" - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" - backup_package_variables - run_package $pkg - tidy_install - create_package $pkg - restore_package_variables - pkgdir="${pkgdir%/*}" - done + handle_splitpkg fi

msg "$(gettext "Leaving fakeroot environment.")" @@ -1966,17 +1970,7 @@ else fi create_package else - for pkg in ${pkgname[@]}; do - pkgdir="$pkgdir/$pkg" - mkdir -p "$pkgdir" - chmod a-s "$pkgdir" - backup_package_variables - run_package $pkg - tidy_install - create_package $pkg - restore_package_variables - pkgdir="${pkgdir%/*}" - done + handle_splitpkg fi else if (( ! REPKG && ( PKGFUNC || SPLITPKG ) )); then -- 1.7.1

Andres P

12:44 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

Fixes a regression in 05ff276eefc with passwd_timeout=0 in sudoers. Passwords were being asked twice for *every* operation. Signed-off-by: Andres P --- makepkg shouldn't make assumptions about the site's security settings, specially something as innocuous as passwd_timeout. A cleaner way that also involves less forks is to process sudo's $?, if possible: sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? if [[ $? = 4 ]]; then error "$(gettext "You are not authorized to use sudo pacman.")" exit $E_AUTH fi Note that 4 is just an example ... scripts/makepkg.sh.in | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 6de6100..f03c358 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -372,7 +372,7 @@ download_file() { run_pacman() { local ret=0 if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]]; then - if type -p sudo >/dev/null && sudo -l $PACMAN &>/dev/null; then + if type -p sudo >/dev/null; then sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? else su -c "$PACMAN $PACMAN_OPTS $*" || ret=$? -- 1.7.1

Allan McRae

1:19 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 17/06/10 22:44, Andres P wrote:

...

Fixes a regression in 05ff276eefc with passwd_timeout=0 in sudoers.

Passwords were being asked twice for *every* operation.

Signed-off-by: Andres P ---

makepkg shouldn't make assumptions about the site's security settings, specially something as innocuous as passwd_timeout.

A cleaner way that also involves less forks is to process sudo's $?, if possible: sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? if [[ $? = 4 ]]; then error "$(gettext "You are not authorized to use sudo pacman.")" exit $E_AUTH fi Note that 4 is just an example

I do not understand you at all here... As far as I can tell, "sudo -l" never asks for a password. Also, passwd_timeout=0 sets sudo to only ever ask for a password once. I am completely lost at what you are trying to achieve with this! Allan

Andres P

1:28 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 8:49 AM, Allan McRae wrote:

...

On 17/06/10 22:44, Andres P wrote:

...
Fixes a regression in 05ff276eefc with passwd_timeout=0 in sudoers.

Passwords were being asked twice for *every* operation.

Signed-off-by: Andres P ---

makepkg shouldn't make assumptions about the site's security settings, specially something as innocuous as passwd_timeout.

A cleaner way that also involves less forks is to process sudo's $?, if possible: sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? if [[ $? = 4 ]]; then error "$(gettext "You are not authorized to use sudo pacman.")" exit $E_AUTH fi Note that 4 is just an example

I do not understand you at all here... As far as I can tell, "sudo -l" never asks for a password. Also, passwd_timeout=0 sets sudo to only ever ask for a password once. I am completely lost at what you are trying to achieve with this!

Allan

My bad, it's timestamp_timeout Run pacman 3.3's makepkg with timestamp_timeout=0 then 3.4... It will ask you twice Actually, just do this sudo -l /bin/true && sudo /bin/true *with* timestamp_timeout=0 Andres P

Allan McRae

1:34 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 17/06/10 23:28, Andres P wrote:

...

On Thu, Jun 17, 2010 at 8:49 AM, Allan McRae wrote:

...
On 17/06/10 22:44, Andres P wrote:

...
Fixes a regression in 05ff276eefc with passwd_timeout=0 in sudoers.

Passwords were being asked twice for *every* operation.

Signed-off-by: Andres P ---

makepkg shouldn't make assumptions about the site's security settings, specially something as innocuous as passwd_timeout.

A cleaner way that also involves less forks is to process sudo's $?, if possible: sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? if [[ $? = 4 ]]; then error "$(gettext "You are not authorized to use sudo pacman.")" exit $E_AUTH fi Note that 4 is just an example

I do not understand you at all here... As far as I can tell, "sudo -l" never asks for a password. Also, passwd_timeout=0 sets sudo to only ever ask for a password once. I am completely lost at what you are trying to achieve with this!

Allan

My bad, it's timestamp_timeout

Run pacman 3.3's makepkg with timestamp_timeout=0 then 3.4...

It will ask you twice

Actually, just do this

sudo -l /bin/true&& sudo /bin/true

*with* timestamp_timeout=0

Um... no it does not... sudo -l does not ask for a password even with timestamp_timeout=0. Allan

Andres P

1:35 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 9:04 AM, Allan McRae wrote:

...

Um... no it does not... sudo -l does not ask for a password even with timestamp_timeout=0.

Allan

Yes it does... man sudoers Defaults timestamp_timeout=0, passwd_timeout=0 sudo -l /bin/true && sudo /bin/true will ask you twice... come on now :/ Andres P -- Andres P

Allan McRae

1:47 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 17/06/10 23:35, Andres P wrote:

...

On Thu, Jun 17, 2010 at 9:04 AM, Allan McRae wrote:

...
Um... no it does not... sudo -l does not ask for a password even with timestamp_timeout=0.

Allan

Yes it does... man sudoers

Defaults timestamp_timeout=0, passwd_timeout=0

sudo -l /bin/true&& sudo /bin/true

will ask you twice... come on now :/

allan@mugen ~

...

sudo -l Matching Defaults entries for allan on this host: timestamp_timeout=0, passwd_timeout=0

User allan may run the following commands on this host: (ALL) ALL allan@mugen ~

...

sudo -l /bin/true && sudo /bin/true /bin/true Password:

allan@mugen ~

...

I count one password request...

Andres P

1:49 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 9:17 AM, Allan McRae wrote:

...

On 17/06/10 23:35, Andres P wrote:

...
On Thu, Jun 17, 2010 at 9:04 AM, Allan McRae wrote:

...
Um... no it does not... sudo -l does not ask for a password even with timestamp_timeout=0.

Allan

Yes it does... man sudoers

Defaults timestamp_timeout=0, passwd_timeout=0

sudo -l /bin/true&& sudo /bin/true

will ask you twice... come on now :/

allan@mugen ~

...
sudo -l Matching Defaults entries for allan on this host: timestamp_timeout=0, passwd_timeout=0

User allan may run the following commands on this host: (ALL) ALL

allan@mugen ~

...
sudo -l /bin/true && sudo /bin/true /bin/true Password:

allan@mugen ~

...
I count one password request...

I advice that you create a new user with a fresh leash. I'm using sudo 1.7.2p7-1 and could go through the trouble of naggging folks to post their sudo output just to get this fixed ;) My sudoers verbatim: # Defaults specification Defaults rootpw, timestamp_timeout=0, passwd_timeout=0 # User privilege specification root ALL=(ALL) ALL # Uncomment to allow people in group wheel to run all commands %wheel ALL=(ALL) ALL Nothing exotic... the only relevant setting is timestamp Andres P

Dan McGee

2:13 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 8:49 AM, Andres P wrote:

...

On Thu, Jun 17, 2010 at 9:17 AM, Allan McRae wrote:

...
On 17/06/10 23:35, Andres P wrote:

...
On Thu, Jun 17, 2010 at 9:04 AM, Allan McRae wrote:

...
Um... no it does not... sudo -l does not ask for a password even with timestamp_timeout=0.

Allan

Yes it does... man sudoers

Defaults timestamp_timeout=0, passwd_timeout=0

sudo -l /bin/true&& sudo /bin/true

will ask you twice... come on now :/

allan@mugen ~

...
sudo -l Matching Defaults entries for allan on this host: timestamp_timeout=0, passwd_timeout=0

User allan may run the following commands on this host: (ALL) ALL

allan@mugen ~

...
sudo -l /bin/true && sudo /bin/true /bin/true Password:

allan@mugen ~

...
I count one password request...

I advice that you create a new user with a fresh leash.

I'm using sudo 1.7.2p7-1 and could go through the trouble of naggging folks to post their sudo output just to get this fixed ;)

My sudoers verbatim: # Defaults specification Defaults rootpw, timestamp_timeout=0, passwd_timeout=0

# User privilege specification root ALL=(ALL) ALL

# Uncomment to allow people in group wheel to run all commands %wheel ALL=(ALL) ALL

Nothing exotic... the only relevant setting is timestamp

Dude, the ball is in your court to prove this, I can't get it to do anything resembling asking for my password twice. I added the two options to my sudoers file and look at hte following sequence. Note that the only time it asks for my password is on the actual execution of the command, not on the '-l' usage. dmcgee@galway ~/projects/pacman (master) $ sudo -l /bin/true /bin/true dmcgee@galway ~/projects/pacman (master) $ sudo /bin/true Password: dmcgee@galway ~/projects/pacman (master) $ sudo /bin/true Password: dmcgee@galway ~/projects/pacman (master) $ sudo -l /bin/true /bin/true

Allan McRae

2:30 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 18/06/10 00:13, Dan McGee wrote:

...

On Thu, Jun 17, 2010 at 8:49 AM, Andres P wrote:

...
On Thu, Jun 17, 2010 at 9:17 AM, Allan McRae wrote:

...
On 17/06/10 23:35, Andres P wrote:

...
On Thu, Jun 17, 2010 at 9:04 AM, Allan McRae wrote:

...
Um... no it does not... sudo -l does not ask for a password even with timestamp_timeout=0.

Allan

Yes it does... man sudoers

Defaults timestamp_timeout=0, passwd_timeout=0

sudo -l /bin/true&& sudo /bin/true

will ask you twice... come on now :/

allan@mugen ~

...
sudo -l Matching Defaults entries for allan on this host: timestamp_timeout=0, passwd_timeout=0

User allan may run the following commands on this host: (ALL) ALL

allan@mugen ~

...
sudo -l /bin/true&& sudo /bin/true /bin/true Password:

allan@mugen ~

...
I count one password request...

I advice that you create a new user with a fresh leash.

I'm using sudo 1.7.2p7-1 and could go through the trouble of naggging folks to post their sudo output just to get this fixed ;)

My sudoers verbatim: # Defaults specification Defaults rootpw, timestamp_timeout=0, passwd_timeout=0

# User privilege specification root ALL=(ALL) ALL

# Uncomment to allow people in group wheel to run all commands %wheel ALL=(ALL) ALL

Nothing exotic... the only relevant setting is timestamp

Dude, the ball is in your court to prove this, I can't get it to do anything resembling asking for my password twice. I added the two options to my sudoers file and look at hte following sequence. Note that the only time it asks for my password is on the actual execution of the command, not on the '-l' usage.

dmcgee@galway ~/projects/pacman (master) $ sudo -l /bin/true /bin/true

dmcgee@galway ~/projects/pacman (master) $ sudo /bin/true Password:

dmcgee@galway ~/projects/pacman (master) $ sudo /bin/true Password:

dmcgee@galway ~/projects/pacman (master) $ sudo -l /bin/true /bin/true

I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password. So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss. Allan

Andres P

2:37 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 10:00 AM, Allan McRae wrote:

...

I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Eureka! I was just about to mail the sudo maintainer. Anyhow, What if there's a check for sudo's retval like I posted in the comments? Andres P

Dan McGee

2:45 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 9:37 AM, Andres P wrote:

...

On Thu, Jun 17, 2010 at 10:00 AM, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Eureka! I was just about to mail the sudo maintainer.

I'm terribly confused still. $ sudo cat /etc/sudoers Password: Defaults editor = /usr/bin/vim:/usr/bin/vi root ALL=(ALL) ALL %wheel ALL=(ALL) ALL dmcgee ALL= NOPASSWD: /usr/sbin/vpnc, /usr/sbin/vpnc-disconnect dmcgee ALL= NOPASSWD: /usr/bin/openconnect I don't see any exemption for `sudo -l` in there, but it never prompts me for a passwd (even if adding those timeout defaults). Or is it just the presence of any NOPASSWD entry at all? If that is the case, that seems downright silly...

...

Anyhow,

What if there's a check for sudo's retval like I posted in the comments?

There is no way to tell the difference between the retval of sudo and the retval of the called program as far as I can tell, so this wouldn't quite work. -Dan

Andres P

3:01 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 10:15 AM, Dan McGee wrote:

...

I'm terribly confused still.

$ sudo cat /etc/sudoers Password:

Defaults editor = /usr/bin/vim:/usr/bin/vi

root ALL=(ALL) ALL %wheel ALL=(ALL) ALL dmcgee ALL= NOPASSWD: /usr/sbin/vpnc, /usr/sbin/vpnc-disconnect dmcgee ALL= NOPASSWD: /usr/bin/openconnect

I don't see any exemption for `sudo -l` in there, but it never prompts me for a passwd (even if adding those timeout defaults). Or is it just the presence of any NOPASSWD entry at all? If that is the case, that seems downright silly...

My config is pretty vanilla so try that instead? Since the misbehaviour happened there.

...

...
Anyhow,

What if there's a check for sudo's retval like I posted in the comments?

There is no way to tell the difference between the retval of sudo and the retval of the called program as far as I can tell, so this wouldn't quite work.

In the context of falling back to su if sudo pacman fails, it would not matter if the error is due to pacman or to sudo missing permissions for pacman. Notice that it was in direct response to "more password prompts" over "loss of functionality": On Thu, Jun 17, 2010 at 10:00 AM, Allan McRae wrote:

...

I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

In short, su would be the fallback if sudo fails for any reason. Andres P

Allan McRae

3:02 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 18/06/10 00:45, Dan McGee wrote:

...

On Thu, Jun 17, 2010 at 9:37 AM, Andres P wrote:

...
On Thu, Jun 17, 2010 at 10:00 AM, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Eureka! I was just about to mail the sudo maintainer.

I'm terribly confused still.

$ sudo cat /etc/sudoers Password:

Defaults editor = /usr/bin/vim:/usr/bin/vi

root ALL=(ALL) ALL %wheel ALL=(ALL) ALL dmcgee ALL= NOPASSWD: /usr/sbin/vpnc, /usr/sbin/vpnc-disconnect dmcgee ALL= NOPASSWD: /usr/bin/openconnect

I don't see any exemption for `sudo -l` in there, but it never prompts me for a passwd (even if adding those timeout defaults). Or is it just the presence of any NOPASSWD entry at all? If that is the case, that seems downright silly...

It is the presence of ANY entry... see the "listpw" section of "man sudoers" Allan

Loui Chang

3:09 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...

I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

Andres P

3:10 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 10:39 AM, Loui Chang wrote:

...

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

+1 ;) Andres P

Allan McRae

10:19 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 18/06/10 01:09, Loui Chang wrote:

...

On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

What do you mean? Remove automatic dependency installation or require the entire thin to be run as root? Allan

Loui Chang

11:12 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Fri 18 Jun 2010 08:19 +1000, Allan McRae wrote:

...

On 18/06/10 01:09, Loui Chang wrote:

...
On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

What do you mean? Remove automatic dependency installation or require the entire thin to be run as root?

Enable the entire thing to be run as any user. A user does not necessarily need to be called 'root' to have package manager privileges, nor do they need to be 'root' to have superuser privileges, so why do we need a special flag for when the user does happen to be 'root'? I think a user should arrange those himself, rather than having makepkg assume that he wants to become root via sudo. If the user hasn't previously arranged the privs, then makepkg dependency installation should fail. In my opinion any use of sudo, and any restrictions on root in makepkg should be removed. If you're keen to this idea I could provide some patches.

Allan McRae

11:37 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 18/06/10 09:12, Loui Chang wrote:

...

On Fri 18 Jun 2010 08:19 +1000, Allan McRae wrote:

...
On 18/06/10 01:09, Loui Chang wrote:

...
On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

What do you mean? Remove automatic dependency installation or require the entire thin to be run as root?

Enable the entire thing to be run as any user.

A user does not necessarily need to be called 'root' to have package manager privileges, nor do they need to be 'root' to have superuser privileges, so why do we need a special flag for when the user does happen to be 'root'?

I think a user should arrange those himself, rather than having makepkg assume that he wants to become root via sudo. If the user hasn't previously arranged the privs, then makepkg dependency installation should fail.

In my opinion any use of sudo, and any restrictions on root in makepkg should be removed. If you're keen to this idea I could provide some patches.

I still am not sure where you are going with this... 1) pacman requires you to be root to install packages (or at least UID=0 I think)

...

pacman -S pacman error: you cannot perform this operation unless you are root.

2) Doing the actual packaging as root is dangerous, especially if you have "make install" by accident in your PKGBUILD. Or, as does happen, the software has a shitty Makefile and ignores DESTDIR for part of the installation (for this reason --asroot is not being removed). So we have conflicting needs within makepkg. root to install, non-root to build. When makepkg needs to install dependency packages, it checks if sudo is an option and if not falls back to using "su -c", and if that fails it gives up. Are you proposing that it just gives up straight away and not attempt privilege escalation? Allan

Dan McGee

18 Jun 18 Jun

12:09 a.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 6:37 PM, Allan McRae wrote:

...

On 18/06/10 09:12, Loui Chang wrote:

...
On Fri 18 Jun 2010 08:19 +1000, Allan McRae wrote:

...
On 18/06/10 01:09, Loui Chang wrote:

...
On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

What do you mean? Remove automatic dependency installation or require the entire thin to be run as root?

Enable the entire thing to be run as any user.

A user does not necessarily need to be called 'root' to have package manager privileges, nor do they need to be 'root' to have superuser privileges, so why do we need a special flag for when the user does happen to be 'root'?

I think a user should arrange those himself, rather than having makepkg assume that he wants to become root via sudo. If the user hasn't previously arranged the privs, then makepkg dependency installation should fail.

In my opinion any use of sudo, and any restrictions on root in makepkg should be removed. If you're keen to this idea I could provide some patches.

I still am not sure where you are going with this...

1) pacman requires you to be root to install packages (or at least UID=0 I think)

...
pacman -S pacman error: you cannot perform this operation unless you are root.

Correct. There is really no way to avoid being root here; you are going to touch everything on the system *and* you need arbitrary command execution. /* geteuid undefined in CYGWIN */ uid_t myuid = geteuid(); ...... /* check if we have sufficient permission for the requested operation */ if(myuid > 0 && needs_root()) { pm_printf(PM_LOG_ERROR, _("you cannot perform this operation unless you are root.\n")); cleanup(EXIT_FAILURE); }

...

2) Doing the actual packaging as root is dangerous, especially if you have "make install" by accident in your PKGBUILD. Or, as does happen, the software has a shitty Makefile and ignores DESTDIR for part of the installation (for this reason --asroot is not being removed).

+1000, Try packaging munin sometime from a blank slate as root and let me know when you un-screw your system. I have spent a long time haggling with packages like that to make sure they are actually doing their work in $pkgdest rather than my live system.

...

So we have conflicting needs within makepkg. root to install, non-root to build. When makepkg needs to install dependency packages, it checks if sudo is an option and if not falls back to using "su -c", and if that fails it gives up. Are you proposing that it just gives up straight away and not attempt privilege escalation?

Couldn't have said it better myself, thank you Allan. -Dan

Loui Chang

10:58 a.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu 17 Jun 2010 19:09 -0500, Dan McGee wrote:

...

Correct. There is really no way to avoid being root here; you are going to touch everything on the system *and* you need arbitrary command execution.

/* geteuid undefined in CYGWIN */ uid_t myuid = geteuid(); ...... /* check if we have sufficient permission for the requested operation */ if(myuid > 0 && needs_root()) { pm_printf(PM_LOG_ERROR, _("you cannot perform this operation unless you are root.\n")); cleanup(EXIT_FAILURE); }

...
2) Doing the actual packaging as root is dangerous, especially if you have "make install" by accident in your PKGBUILD. Or, as does happen, the software has a shitty Makefile and ignores DESTDIR for part of the installation (for this reason --asroot is not being removed).

+1000, Try packaging munin sometime from a blank slate as root and let me know when you un-screw your system. I have spent a long time haggling with packages like that to make sure they are actually doing their work in $pkgdest rather than my live system.

Hmm. How does it exactly screw your system? I did notice that it doesn't have an uninstall make rule, but other than that it seems to perform sanely, and easy to clean up.

Loui Chang

1:18 a.m.

New subject: [pacman-dev] makepkg: privilege management utility? twice

On Fri 18 Jun 2010 09:37 +1000, Allan McRae wrote:

...

On 18/06/10 09:12, Loui Chang wrote:

...
On Fri 18 Jun 2010 08:19 +1000, Allan McRae wrote:

...
On 18/06/10 01:09, Loui Chang wrote:

...
Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

What do you mean? Remove automatic dependency installation or require the entire thin to be run as root?

Enable the entire thing to be run as any user.

A user does not necessarily need to be called 'root' to have package manager privileges, nor do they need to be 'root' to have superuser privileges, so why do we need a special flag for when the user does happen to be 'root'?

I think a user should arrange those himself, rather than having makepkg assume that he wants to become root via sudo. If the user hasn't previously arranged the privs, then makepkg dependency installation should fail.

In my opinion any use of sudo, and any restrictions on root in makepkg should be removed. If you're keen to this idea I could provide some patches.

I still am not sure where you are going with this...

What I'm essentially saying is that makepkg shouldn't be the one managing privileges. It should be the users.

...

1) pacman requires you to be root to install packages (or at least UID=0 I think)

...
pacman -S pacman error: you cannot perform this operation unless you are root.

2) Doing the actual packaging as root is dangerous, especially if you have "make install" by accident in your PKGBUILD. Or, as does happen, the software has a shitty Makefile and ignores DESTDIR for part of the installation (for this reason --asroot is not being removed).

So we have conflicting needs within makepkg. root to install, non-root to build. When makepkg needs to install dependency packages, it checks if sudo is an option and if not falls back to using "su -c", and if that fails it gives up. Are you proposing that it just gives up straight away and not attempt privilege escalation?

Yes, I am proposing that. You can always drop to more restricted permissions to build if you are a worried superuser. It's an admin's duty to understand these precautions isn't it?

Andres P

4:27 a.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 7:07 PM, Allan McRae wrote:

...

2) Doing the actual packaging as root is dangerous, especially if you have "make install" by accident in your PKGBUILD. Or, as does happen, the software has a shitty Makefile and ignores DESTDIR for part of the installation (for this reason --asroot is not being removed).

If at any point you encounter a "shitty" makefile, then you submit a patch upstream like anybody else. The last thing you do is put a "shitty" workaround in a bash script.

...

So we have conflicting needs within makepkg. root to install, non-root to build. When makepkg needs to install dependency packages, it checks if sudo is an option and if not falls back to using "su -c", and if that fails it gives up. Are you proposing that it just gives up straight away and not attempt privilege escalation?

All of this insight going nowhere, and the fact still stands that this behaviour is *new*. If sudo -l && sudo means 2 password prompts, then the logical route that makepkg can take is either assume that if sudo is in PATH (type -p sudo), then that means it's configured to run makepkg. If it's not in PATH, then su it is. The other route would be to revert to what you had in the repo before the commit that's been refered to took place, but that would make too much sense. Andres P

Andres P

4:33 a.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu, Jun 17, 2010 at 11:57 PM, Andres P wrote:

...

The other route would be to revert to what you had in the repo before the commit that's been refered to took place, but that would make too much sense.

Wait, I have an even wilder idea. Let the user configure wether sudo or su should be used, since it's been proven that makepkg *cannot* guess which one it is through bash hackery. Andres P

Andres P

19 Jun 19 Jun

12:47 a.m.

New subject: [pacman-dev] [PATCH 1/3] makepkg: simplify su/sudo calling logic

* Before, calls to sudo were not being word-split, whereas falling back to su, for some reason, meant applying IFS to the arguments. From now on calling either does not mangle any of the operands. * Instead of recreating the basic bash concept of $? with $ret, opt for stacking the command line into a variable that can later be checked *once* for a return code. Signed-off-by: Andres P --- scripts/makepkg.sh.in | 14 ++++++-------- 1 files changed, 6 insertions(+), 8 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 536da30..2258d2a 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -370,17 +370,15 @@ download_file() { } run_pacman() { - local ret=0 - if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]]; then - if type -p sudo >/dev/null && sudo -l $PACMAN &>/dev/null; then - sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? + local cmd=$(printf "%q " "$PACMAN" $PACMAN_OPTS "$@") + if (( ! ASROOT )) && [[ ! $1 =~ ^-(T|Qq)$ ]]; then + if type -p sudo >/dev/null && sudo -l "$PACMAN" &>/dev/null; then + cmd="sudo $cmd" else - su -c "$PACMAN $PACMAN_OPTS $*" || ret=$? + cmd="su -c '$cmd'" fi - else - $PACMAN $PACMAN_OPTS "$@" || ret=$? fi - return $ret + eval "$cmd" } check_deps() { -- 1.7.1

Andres P

12:50 a.m.

New subject: [pacman-dev] [PATCH 1/3] makepkg: simplify su/sudo calling logic

On Fri, Jun 18, 2010 at 8:17 PM, Andres P wrote:

...

* Before, calls to sudo were not being word-split, whereas falling back to su, for some reason, meant applying IFS to the arguments. From now on calling either does not mangle any of the operands.

* Instead of recreating the basic bash concept of $? with $ret, opt for stacking the command line into a variable that can later be checked *once* for a return code.

Note that this still has the "using su fucks up the terminal" bug present in 3.4. When makepkg uses su for any reason, fallback or not, the terminal will no longer display stdin and has to be reset(1). To reproduce: # mv /usr/bin/sudo{,~} Then use a makepkg operation that requires root; e.g. -i or -s. I can reproduce outside of makepkg: $ fakeroot bash -c "su -c 'pacman -Q foo'" But this is *just* an example. These calls do not involve fakeroot. Andres P

Allan McRae

6:29 a.m.

New subject: [pacman-dev] makepkg, su and output - Was: [PATCH 1/3] makepkg: simplify su/sudo calling logic

On 19/06/10 10:50, Andres P wrote:

...

Note that this still has the "using su fucks up the terminal" bug present in 3.4.

Still has? This is the first I am hearing of it... Is there a bug report about this?

...

When makepkg uses su for any reason, fallback or not, the terminal will no longer display stdin and has to be reset(1).

To reproduce: # mv /usr/bin/sudo{,~}

Then use a makepkg operation that requires root; e.g. -i or -s.

I just tested this and there is even worse things happening. I have a PKGBUILD which has depends=('pacman>4') in it for testing out another bug. When using su I get: --start output--

...

makepkg -sf ==> WARNING: Sudo can not be found. Will use su to acquire root privileges. ==> Making package: pacman-contrib 3.4.0-1 (Sat Jun 19 16:24:35 EST 2010) ==> Checking Runtime Dependencies... ==> Installing missing dependencies... Password: Proceed with installation? [Y/n] ==> Missing Dependencies: -> pacman>4 ==> Checking Buildtime Dependencies... ==> ERROR: Could not resolve all dependencies. --end output--

Note it processed trying to install pacman, even though >4 does not exist. Then if find a file called "4" in my $startdir with: --start 4-- warning: pacman-3.4.0-1 is up to date -- reinstalling resolving dependencies... looking for inter-conflicts... Targets (1): pacman-3.4.0-1 Total Download Size: 0.00 MB Total Installed Size: 1.98 MB checking package integrity... checking for file conflicts... upgrading pacman... --end 4-- It appears that the 'pacman>4' is not staying quoted and is causing the output to be redirected.

...

I can reproduce outside of makepkg: $ fakeroot bash -c "su -c 'pacman -Q foo'"

I can not see anything wrong when using that. Allan

Andres P

6:48 a.m.

New subject: [pacman-dev] makepkg, su and output - Was: [PATCH 1/3] makepkg: simplify su/sudo calling logic

On Sat, Jun 19, 2010 at 1:59 AM, Allan McRae wrote:

...

On 19/06/10 10:50, Andres P wrote:

...
Note that this still has the "using su fucks up the terminal" bug present in 3.4.

Still has? This is the first I am hearing of it... Is there a bug report about this?

Not afaik, I just experienced it with >=3.4 (I had not tested pacman-git until it was released in testing).

...

...
I can reproduce outside of makepkg: $ fakeroot bash -c "su -c 'pacman -Q foo'"

I can not see anything wrong when using that.

Hmm, but you do confirm that makepkg falling back to su screws up the terminal? Andres P

Allan McRae

7:54 a.m.

New subject: [pacman-dev] makepkg, su and output - Was: [PATCH 1/3] makepkg: simplify su/sudo calling logic

On 19/06/10 16:48, Andres P wrote:

...

On Sat, Jun 19, 2010 at 1:59 AM, Allan McRae wrote:

...
On 19/06/10 10:50, Andres P wrote:

...
Note that this still has the "using su fucks up the terminal" bug present in 3.4.

Still has? This is the first I am hearing of it... Is there a bug report about this?

Not afaik, I just experienced it with>=3.4 (I had not tested pacman-git until it was released in testing).

...
...
I can reproduce outside of makepkg: $ fakeroot bash -c "su -c 'pacman -Q foo'"

I can not see anything wrong when using that.

Hmm, but you do confirm that makepkg falling back to su screws up the terminal?

No... I do not know what you mean by this. Can you post some output or a screenshot? Allan

Andres P

8:33 a.m.

New subject: [pacman-dev] makepkg, su and output - Was: [PATCH 1/3] makepkg: simplify su/sudo calling logic

On Sat, Jun 19, 2010 at 3:24 AM, Allan McRae wrote:

...

No... I do not know what you mean by this. Can you post some output or a screenshot?

It'd be kind of difficult since the problem is mostly about what doesn't show up. It's like when you turn on flow control with ^S, type 'ls; echo foo' and press enter, then press ^Q. Except that I'm free to press enter and see the feedback on my scrollback buffer from the commands that I press, whithout seeing a "preview" of it to the right of my $PS1. I've now isolated the problem and it does not happen with every su operation, only when cancelling the su password prompt. Here's what I'm doing: 1. I uninstall asciidoc 2. curl http://aur.archlinux.org/packages/pacman-git/pacman-git.tar.gz | tar xz 3. cd pacman-git; makepkg -s 4. When it shows the su password prompt, I hit ^C What I type is now invisible until I `reset`. Does not happen with sudo nor from regular calls like `su -c ls` from my shell. btw, I cannot get master nor latest tag to pipe files named after versioned deps to $startdir; e.g. $startdir/4. makepkg falling back to su still calls pacman -T correctly. Andres P

Allan McRae

8:54 a.m.

New subject: [pacman-dev] makepkg, su and output - Was: [PATCH 1/3] makepkg: simplify su/sudo calling logic

On 19/06/10 18:33, Andres P wrote:

...

On Sat, Jun 19, 2010 at 3:24 AM, Allan McRae wrote:

...
No... I do not know what you mean by this. Can you post some output or a screenshot?

It'd be kind of difficult since the problem is mostly about what doesn't show up.

It's like when you turn on flow control with ^S, type 'ls; echo foo' and press enter, then press ^Q.

Except that I'm free to press enter and see the feedback on my scrollback buffer from the commands that I press, whithout seeing a "preview" of it to the right of my $PS1.

I've now isolated the problem and it does not happen with every su operation, only when cancelling the su password prompt.

Here's what I'm doing:

1. I uninstall asciidoc 2. curl http://aur.archlinux.org/packages/pacman-git/pacman-git.tar.gz | tar xz 3. cd pacman-git; makepkg -s 4. When it shows the su password prompt, I hit ^C

What I type is now invisible until I `reset`. Does not happen with sudo nor from regular calls like `su -c ls` from my shell.

ah... I can replicate. This is weird. Perhaps some sort of pipe is still capturing the output?

...

btw, I cannot get master nor latest tag to pipe files named after versioned deps to $startdir; e.g. $startdir/4. makepkg falling back to su still calls pacman -T correctly.

1. update ABS 2. cp /var/abs/community-testing/pacman-contrib/* . 3. adjust dependencies to be depends=('pacman>4') 4. makepkg -s, type in your password. It will reinstall pacman from the repos and then bail because on missing deps 5. notice file named "4" in you directory containing pacman output...

Andres P

9:48 a.m.

New subject: [pacman-dev] makepkg, su and output - Was: [PATCH 1/3] makepkg: simplify su/sudo calling logic

On Sat, Jun 19, 2010 at 4:24 AM, Allan McRae wrote:

...

1. update ABS 2. cp /var/abs/community-testing/pacman-contrib/* . 3. adjust dependencies to be depends=('pacman>4') 4. makepkg -s, type in your password. It will reinstall pacman from the repos and then bail because on missing deps 5. notice file named "4" in you directory containing pacman output...

Yep, now i see: pacman-contrib $ cat 4 warning: downgrading package pacman (3.4.0-1 => 3.3.3-5) resolving dependencies... looking for inter-conflicts... Targets (1): pacman-3.3.3-5 Total Download Size: 0.00 MB Total Installed Size: 2.09 MB checking package integrity... checking for file conflicts... upgrading pacman... However, this is no longer an issue after my patch and can confirm that there's no $startdir/4. $ printf %q\\n "pacman>4" pacman\>4 Andres P

Andres P

12:47 a.m.

New subject: [pacman-dev] [PATCH 2/3] makepkg: allow specifying means for privilege escalation

Given that certain sudoers settings cause issues with excessive password prompts, and that certain users don't want to use sudo to begin with, makepkg will now refrain from making assumptions that couldn't possibly be accurate.

...

From this point on, there are no artificial dependencies nor second hand guesses regarding su/sudo.

Signed-off-by: Andres P --- scripts/makepkg.sh.in | 18 ++++++------------ 1 files changed, 6 insertions(+), 12 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 2258d2a..29b5664 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -372,10 +372,10 @@ download_file() { run_pacman() { local cmd=$(printf "%q " "$PACMAN" $PACMAN_OPTS "$@") if (( ! ASROOT )) && [[ ! $1 =~ ^-(T|Qq)$ ]]; then - if type -p sudo >/dev/null && sudo -l "$PACMAN" &>/dev/null; then - cmd="sudo $cmd" + if [[ $PACMAN_AUTH =~ %p ]]; then + cmd="${PACMAN_AUTH//%p/$cmd}" else - cmd="su -c '$cmd'" + cmd="$PACMAN_AUTH $cmd" fi fi eval "$cmd" @@ -1610,8 +1610,9 @@ if [[ -r ~/.makepkg.conf ]]; then source ~/.makepkg.conf fi -# set pacman command if not already defined -PACMAN=${PACMAN:-pacman} +# set pacman commands if not already defined +PACMAN=${PACMAN:-"pacman"} +PACMAN_AUTH=${PACMAN_AUTH:-"sudo %p"} # check if messages are to be printed using color unset ALL_OFF BOLD BLUE GREEN RED YELLOW @@ -1714,13 +1715,6 @@ else fi fi -# check for sudo if we will need it during makepkg execution -if (( ! ( ASROOT || INFAKEROOT ) && ( DEP_BIN || RMDEPS || INSTALL ) )); then - if ! type -p sudo >/dev/null; then - warning "$(gettext "Sudo can not be found. Will use su to acquire root privileges.")" - fi -fi - unset pkgname pkgbase pkgver pkgrel pkgdesc url license groups provides unset md5sums replaces depends conflicts backup source install changelog build unset makedepends optdepends options noextract -- 1.7.1

Andres P

12:47 a.m.

New subject: [pacman-dev] [PATCH 3/3] makepkg: document PACMAN_AUTH in config and manual pages

Signed-off-by: Andres P --- doc/makepkg.conf.5.txt | 7 +++++++ etc/makepkg.conf.in | 11 +++++++++++ 2 files changed, 18 insertions(+), 0 deletions(-) diff --git a/doc/makepkg.conf.5.txt b/doc/makepkg.conf.5.txt index 753b179..b9a0c67 100644 --- a/doc/makepkg.conf.5.txt +++ b/doc/makepkg.conf.5.txt @@ -195,6 +195,13 @@ Options *PKGEXT*, *SRCEXT*:: Do not touch these unless you know what you are doing. +*PACMAN_AUTH*:: + If makepkg needs to run pacman as root it will use the command specified in + this value to call pacman when the user is not UID 0. + + + If present, `%p` will be replaced with the environmental variable PACMAN and + whatever arguments makepkg needs to pass to PACMAN. Otherwise, both PACMAN + and its arguments will be appended to the end of the command. See Also -------- diff --git a/etc/makepkg.conf.in b/etc/makepkg.conf.in index f0d1c44..b8252a7 100644 --- a/etc/makepkg.conf.in +++ b/etc/makepkg.conf.in @@ -112,4 +112,15 @@ PURGE_TARGETS=(usr/{,share}/info/dir .packlist *.pod) PKGEXT='@PKGEXT@' SRCEXT='@SRCEXT@' +######################################################################### +# PRIVILEGE ESCALATION OPTIONS +######################################################################### +# +# Default: call pacman using sudo(8) +# +#-- Authentication: specify a command that should enable running pacman as root +# when needed +PACMAN_AUTH="sudo %p" +#PACMAN_AUTH="su -c '%p'" + # vim: set ft=sh ts=2 sw=2 et: -- 1.7.1

Cedric Staniewski

30 Jun 30 Jun

12:49 p.m.

New subject: [pacman-dev] [PATCH 3/3] makepkg: document PACMAN_AUTH in config and manual pages

Any plans to allow overriding from the environment? You can currently set PACMAN, but that may require to adjust PACMAN_AUTH.

Cedric Staniewski

24 Jun 24 Jun

4:28 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 17.06.2010 17:09, Loui Chang wrote:

...

On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

I'd like to add that "sudo -l" was never meant as hand-holding. The intention was to support pacman-wrappers/replacements that aren't supposed to be run as root because they have their own logic to call pacman as root. The most prominent example would be yaourt, I guess. But since this is broken due to the 'su -c' patch, I'm fine with removing it again.

Loui Chang

4:58 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu 24 Jun 2010 18:28 +0200, Cedric Staniewski wrote:

...

On 17.06.2010 17:09, Loui Chang wrote:

...
On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

I'd like to add that "sudo -l" was never meant as hand-holding. The intention was to support pacman-wrappers/replacements that aren't supposed to be run as root because they have their own logic to call pacman as root. The most prominent example would be yaourt, I guess. But since this is broken due to the 'su -c' patch, I'm fine with removing it again.

Yeah it just kind of bothers me that makepkg is doing all these auxiliary functions like package installation, uninstallation, and permissions managment. It has lost its focus. I think those things are better placed in outside scripts (like yaourt). It almost seems like the only thing stopping it from becoming another yaourt is that we've dubbed the AUR as untrusted.

Allan McRae

11:18 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On 25/06/10 02:58, Loui Chang wrote:

...

On Thu 24 Jun 2010 18:28 +0200, Cedric Staniewski wrote:

...
On 17.06.2010 17:09, Loui Chang wrote:

...
On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

I'd like to add that "sudo -l" was never meant as hand-holding. The intention was to support pacman-wrappers/replacements that aren't supposed to be run as root because they have their own logic to call pacman as root. The most prominent example would be yaourt, I guess. But since this is broken due to the 'su -c' patch, I'm fine with removing it again.

Yeah it just kind of bothers me that makepkg is doing all these auxiliary functions like package installation, uninstallation, and permissions managment. It has lost its focus.

You know that dependency installation etc was a very, very early feature so how can makepkg have "lost its focus"?

...

I think those things are better placed in outside scripts (like yaourt). It almost seems like the only thing stopping it from becoming another yaourt is that we've dubbed the AUR as untrusted.

So you use makepkg to update your system? Seriously, if you are recommending that automatic dependency is removed from makepkg, you need to go away, do some more packaging and then reevaluate your opinion. Allan

Loui Chang

25 Jun 25 Jun

12:49 a.m.

New subject: [pacman-dev] Ideas welcome?

On Fri 25 Jun 2010 09:18 +1000, Allan McRae wrote:

...

On 25/06/10 02:58, Loui Chang wrote:

...
On Thu 24 Jun 2010 18:28 +0200, Cedric Staniewski wrote:

...
On 17.06.2010 17:09, Loui Chang wrote:

...
On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

I'd like to add that "sudo -l" was never meant as hand-holding. The intention was to support pacman-wrappers/replacements that aren't supposed to be run as root because they have their own logic to call pacman as root. The most prominent example would be yaourt, I guess. But since this is broken due to the 'su -c' patch, I'm fine with removing it again.

Yeah it just kind of bothers me that makepkg is doing all these auxiliary functions like package installation, uninstallation, and permissions managment. It has lost its focus.

You know that dependency installation etc was a very, very early feature so how can makepkg have "lost its focus"?

You're right. I should say that it lacks focus.

...

...
I think those things are better placed in outside scripts (like yaourt). It almost seems like the only thing stopping it from becoming another yaourt is that we've dubbed the AUR as untrusted.

So you use makepkg to update your system?

Seriously, if you are recommending that automatic dependency is removed from makepkg, you need to go away, do some more packaging and then reevaluate your opinion.

Yes I am recommending that it be -moved- from makepkg, but how does that mean I need to go away? I never said that it is unneccessary. I just believe the auxiliary functions should be moved into other scripts. I hardly need to do any packaging to see the flaws in the AUR, aurtools (defunct), and devtools. What makes makepkg the exception? I don't understand how my opinion on the design of the tools would be so dramatically changed whether I've made 10 packages, or 100. At least you could say "patches welcome". I guess that wouldn't be of much use though, because you've already completely dismissed my comments. Sorry. I didn't mean to offend your own opinions.

Andres P

1:01 a.m.

New subject: [pacman-dev] Ideas welcome?

On Thu, Jun 24, 2010 at 8:19 PM, Loui Chang wrote:

...

Yes I am recommending that it be -moved- from makepkg, but how does that mean I need to go away? I never said that it is unneccessary. I just believe the auxiliary functions should be moved into other scripts.

I agree. makepkg is too big. It needs to be librarized in every aspect, not just per package handling. It should look like this layout: http://www.openbsd.org/cgi-bin/cvsweb/ports/infrastructure/ If anyone would like to find an example in shell, simply install laptop-mode-tools, pm-utils or netcfg. Their apps are layed out in a sane hierarchy that's easy to modify and maintain. When I wanted to change the collation of packages, just by looking at the layout of alpm I could spot where are packages sorted. With makepkg no one has that luxury; it's one big script and it's messy to thread about. Other benefits include: * Since it'd be modularized, wrappers don't have to run the executable. They just source what they need. This isolates bugs like any good library would. * I don't have to write a friggen blog on the commit message. "Modified: core/auth strip/libs" would set the context of my changes, like in any sane project. Andres P

Allan McRae

2:09 a.m.

New subject: [pacman-dev] Ideas welcome?

On 25/06/10 10:49, Loui Chang wrote:

...

On Fri 25 Jun 2010 09:18 +1000, Allan McRae wrote:

...
On 25/06/10 02:58, Loui Chang wrote:

...
On Thu 24 Jun 2010 18:28 +0200, Cedric Staniewski wrote:

...
On 17.06.2010 17:09, Loui Chang wrote:

...
On Fri 18 Jun 2010 00:30 +1000, Allan McRae wrote:

...
I think I have found the issue here. We obviously have a NOPASSWD entry in our sudoers file so "sudo -l" does not require a password.

So the bug is confirmed. However the fix is not fully functional as if I have sudo installed but can not use it for pacman, then I can no longer fall back to using "su -c". I'd choose excess password typing over functionality loss.

Why not just take sudo and asroot out of the equation and treat makepkg as a real non-handholding executable?

I'd like to add that "sudo -l" was never meant as hand-holding. The intention was to support pacman-wrappers/replacements that aren't supposed to be run as root because they have their own logic to call pacman as root. The most prominent example would be yaourt, I guess. But since this is broken due to the 'su -c' patch, I'm fine with removing it again.

Yeah it just kind of bothers me that makepkg is doing all these auxiliary functions like package installation, uninstallation, and permissions managment. It has lost its focus.

You know that dependency installation etc was a very, very early feature so how can makepkg have "lost its focus"?

You're right. I should say that it lacks focus.

...
...
I think those things are better placed in outside scripts (like yaourt). It almost seems like the only thing stopping it from becoming another yaourt is that we've dubbed the AUR as untrusted.

So you use makepkg to update your system?

Seriously, if you are recommending that automatic dependency is removed from makepkg, you need to go away, do some more packaging and then reevaluate your opinion.

Yes I am recommending that it be -moved- from makepkg, but how does that mean I need to go away? I never said that it is unneccessary. I just believe the auxiliary functions should be moved into other scripts.

I said "go away, do some packaging" not just go away... Automatic dependency checking, installation (and removal) is an essential part of makepkg. I doubt any heavy makepkg user would disagree with me there. So what would moving it to another script achieve in practice? It would be a script likely only used by makepkg, essential to any real world use of makepkg and maintained alongside makepkg. And to be clear, you said "those things are better placed in outside scripts (like yaourt)" which implies you did not see them as necessary in the makepkg code base.

...

I hardly need to do any packaging to see the flaws in the AUR, aurtools (defunct), and devtools. What makes makepkg the exception?

makepkg does packaging... none of the other tools you mention do.

...

I don't understand how my opinion on the design of the tools would be so dramatically changed whether I've made 10 packages, or 100.

Making 10 packages means you refer to things like "package installation, uninstallation" as "auxiliary functions". Making 1000s of packages means you see them as essential and not feature bloat as you original email labeled them. Much like I only have 1 AUR package installed, so I see no need for AUR helpers while heary AUR users strong disagree.

...

At least you could say "patches welcome". I guess that wouldn't be of much use though, because you've already completely dismissed my comments. Sorry. I didn't mean to offend your own opinions.

I say patches welcome when I like the idea. You original email said "I think those things are better placed in outside scripts (like yaourt)". That tells me you do not even want this in the makepkg code base. I strongly disagreed so what would be the point of welcoming patches for it... Allan

Loui Chang

27 Jun 27 Jun

11:43 p.m.

New subject: [pacman-dev] Ideas welcome?

On Fri 25 Jun 2010 12:09 +1000, Allan McRae wrote:

...

I said "go away, do some packaging" not just go away... Automatic dependency checking, installation (and removal) is an essential part of makepkg. I doubt any heavy makepkg user would disagree with me there. So what would moving it to another script achieve in practice? It would be a script likely only used by makepkg, essential to any real world use of makepkg and maintained alongside makepkg. And to be clear, you said "those things are better placed in outside scripts (like yaourt)" which implies you did not see them as necessary in the makepkg code base.

Andres has mentioned some benefits. A more distinct separation of a PKGBUILD's definition and processing from extra conveniences interests me. I already perform some of these extras outside of makepkg.

...

...
I hardly need to do any packaging to see the flaws in the AUR, aurtools (defunct), and devtools. What makes makepkg the exception?

makepkg does packaging... none of the other tools you mention do.

These are related to packaging. My point really is that without using them much I can already see the problems. The same applies with makepkg.

...

...
I don't understand how my opinion on the design of the tools would be so dramatically changed whether I've made 10 packages, or 100.

Making 10 packages means you refer to things like "package installation, uninstallation" as "auxiliary functions". Making 1000s of packages means you see them as essential and not feature bloat as you original email labeled them. Much like I only have 1 AUR package installed, so I see no need for AUR helpers while heary AUR users strong disagree.

Right. Just because someone sees a part as essential doesn't really make it so. Meaning, that it isn't integral to the program's core purpose. Chroot packaging, and namcap might be essential to some, but they are not part of makepkg. You can have all those functions elsewhere and still get the benefits. It's just a difference of design.

...

...
At least you could say "patches welcome". I guess that wouldn't be of much use though, because you've already completely dismissed my comments. Sorry. I didn't mean to offend your own opinions.

I say patches welcome when I like the idea. You original email said "I think those things are better placed in outside scripts (like yaourt)". That tells me you do not even want this in the makepkg code base. I strongly disagreed so what would be the point of welcoming patches for it...

You're right. I don't think they should really be part of the makepkg codebase. If they existed externally, would that make them less effective? Probably not. Anyways, I'm not asking you to do anything other than accept that my ideas might have some validity. Thanks.

Cedric Staniewski

30 Jun 30 Jun

12:08 p.m.

New subject: [pacman-dev] Ideas welcome?

Dependency handling is a really useful feature, but I don't like the whole sudo/su thing. How about removing explicit support for sudo/su and leave it to the user to properly configure it for his needs? It could look similar to this: diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cbc344d..2599e3c 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -369,12 +369,8 @@ download_file() { run_pacman() { local ret=0 - if (( ! ASROOT )) && [[ $1 != "-T" && $1 != "-Qq" ]]; then - if type -p sudo >/dev/null && sudo -l $PACMAN &>/dev/null; then - sudo $PACMAN $PACMAN_OPTS "$@" || ret=$? - else - su -c "$PACMAN $PACMAN_OPTS $*" || ret=$? - fi + if [[ $1 = "-T" || $1 = "-Qq" ]]; then + $PACMANBIN $PACMAN_OPTS "$@" || ret=$? else $PACMAN $PACMAN_OPTS "$@" || ret=$? fi @@ -1631,7 +1627,8 @@ if [[ -r ~/.makepkg.conf ]]; then fi # set pacman command if not already defined -PACMAN=${PACMAN:-pacman} +PACMANBIN=${PACMANBIN:-pacman} +PACMAN=${PACMAN:-sudo $PACMANBIN} # check if messages are to be printed using color unset ALL_OFF BOLD BLUE GREEN RED YELLOW @@ -1734,13 +1731,6 @@ else fi fi -# check for sudo if we will need it during makepkg execution -if (( ! ( ASROOT || INFAKEROOT ) && ( DEP_BIN || RMDEPS || INSTALL ) )); then - if ! type -p sudo >/dev/null; then - warning "$(gettext "Sudo can not be found. Will use su to acquire root privileges.")" - fi -fi - unset pkgname pkgbase pkgver pkgrel pkgdesc url license groups provides unset md5sums replaces depends conflicts backup source install changelog build unset makedepends optdepends options noextract @@ -1896,7 +1886,7 @@ if (( NODEPS || ( (NOBUILD || REPKG) && !DEP_BIN ) )); then if (( NODEPS || ( REPKG && PKGFUNC ) )); then warning "$(gettext "Skipping dependency checks.")" fi -elif type -p "${PACMAN%% *}" >/dev/null; then +else if (( RMDEPS )); then original_pkglist=($(run_pacman -Qq)) # required by remove_dep fi @@ -1916,8 +1906,6 @@ elif type -p "${PACMAN%% *}" >/dev/null; then error "$(gettext "Could not resolve all dependencies.")" exit 1 fi -else - warning "$(gettext "%s was not found in PATH; skipping dependency checks.")" "${PACMAN%% *}" fi # ensure we have a sane umask set -- 1.7.1

Allan McRae

12:29 p.m.

New subject: [pacman-dev] Ideas welcome?

On 30/06/10 22:08, Cedric Staniewski wrote:

...

Dependency handling is a really useful feature, but I don't like the whole sudo/su thing. How about removing explicit support for sudo/su and leave it to the user to properly configure it for his needs? It could look similar to this:

There is also a couple of patches from Andres P on 2010-06-19 that implement a configuration option to allow the user to configure how to escalate privileges for package installation (specifically patches 2/3 and 3/3). I think that approach is better although I have not fully reviewed those patches yet. Allan

Cedric Staniewski

12:44 p.m.

New subject: [pacman-dev] Ideas welcome?

On 30.06.2010 14:29, Allan McRae wrote:

...

On 30/06/10 22:08, Cedric Staniewski wrote:

...
Dependency handling is a really useful feature, but I don't like the whole sudo/su thing. How about removing explicit support for sudo/su and leave it to the user to properly configure it for his needs? It could look similar to this:

There is also a couple of patches from Andres P on 2010-06-19 that implement a configuration option to allow the user to configure how to escalate privileges for package installation (specifically patches 2/3 and 3/3). I think that approach is better although I have not fully reviewed those patches yet.

Allan

You're right, seems I missed these. It's basically the same, but a bit more advanced. The only thing I don't like is that you cannot set PACMAN_AUTH from the environment.

Loui Chang

17 Jun 17 Jun

3:07 p.m.

New subject: [pacman-dev] [PATCH 10/11] makepkg: do not ask sudo password twice

On Thu 17 Jun 2010 09:19 -0430, Andres P wrote:

...

On Thu, Jun 17, 2010 at 9:17 AM, Allan McRae wrote:

...
On 17/06/10 23:35, Andres P wrote:

...
On Thu, Jun 17, 2010 at 9:04 AM, Allan McRae wrote:

...
Um... no it does not... sudo -l does not ask for a password even with timestamp_timeout=0.

Allan

Yes it does... man sudoers

Defaults timestamp_timeout=0, passwd_timeout=0

sudo -l /bin/true&& sudo /bin/true

will ask you twice... come on now :/

It asks for my password twice: [louipc@lynn ~]$ sudo -l /bin/true && sudo /bin/true Password: /bin/true Password:

Andres P

12:44 p.m.

New subject: [pacman-dev] [PATCH 11/11] makepkg: fix inconsistent output when checking dependencies

Instead of writing: ==> Checking Runtime Dependencies... ==> Checking Buildtime Dependencies... ==> Installing missing dependencies... Just make it homogeneous: ==> Checking runtime dependencies... ==> Checking buildtime dependencies... ==> Installing missing dependencies... Signed-off-by: Andres P --- scripts/makepkg.sh.in | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index f03c358..89a734a 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1884,10 +1884,10 @@ elif type -p "${PACMAN%% *}" >/dev/null; then fi deperr=0 - msg "$(gettext "Checking Runtime Dependencies...")" + msg "$(gettext "Checking runtime dependencies...")" resolve_deps ${depends[@]} || deperr=1 - msg "$(gettext "Checking Buildtime Dependencies...")" + msg "$(gettext "Checking buildtime dependencies...")" resolve_deps ${makedepends[@]} || deperr=1 if (( RMDEPS )); then -- 1.7.1

Dan McGee

2:33 p.m.

New subject: [pacman-dev] [PATCH 11/11] makepkg: fix inconsistent output when checking dependencies

On Thu, Jun 17, 2010 at 7:44 AM, Andres P wrote:

...

Instead of writing: ==> Checking Runtime Dependencies... ==> Checking Buildtime Dependencies... ==> Installing missing dependencies...

Just make it homogeneous: ==> Checking runtime dependencies... ==> Checking buildtime dependencies... ==> Installing missing dependencies...

Signed-off-by: Andres P ---

Signed-off-by: Dan McGee

Dan McGee

2:19 p.m.

On Thu, Jun 17, 2010 at 7:44 AM, Andres P wrote:

...

Regression in c71fe7db checked for wrong variables when populating .PKGINFO.

Signed-off-by: Andres P ---

I'm having a real hard time picking out what changed here. Mind explaining a bit more what changed? Actually, now i might see it, but your re-indenting the whole block made it non-obvious. Something as simple as this in the commit message would save us all 3 minutes of staring: The 'optdepend' and 'conflict' variables were singular instead of plural. Looks good to me otherwise, as long as this note ends up in the commit message.

...

scripts/makepkg.sh.in | 16 ++++++++-------- 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index 678359f..d0b8b4b 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -923,14 +923,14 @@ write_pkginfo() { echo "force = true" fi

- [[ $license ]] && printf "license = %s\n" "${license[@]}" - [[ $replaces ]] && printf "replaces = %s\n" "${replaces[@]}" - [[ $groups ]] && printf "group = %s\n" "${groups[@]}" - [[ $depends ]] && printf "depend = %s\n" "${depends[@]}" - [[ $optdepend ]] && printf "optdepend = %s\n" "${optdepends[@]}" - [[ $conflict ]] && printf "conflict = %s\n" "${conflicts[@]}" - [[ $provides ]] && printf "provides = %s\n" "${provides[@]}" - [[ $backup ]] && printf "backup = %s\n" "${backup[@]}" + [[ $license ]] && printf "license = %s\n" "${license[@]}" + [[ $replaces ]] && printf "replaces = %s\n" "${replaces[@]}" + [[ $groups ]] && printf "group = %s\n" "${groups[@]}" + [[ $depends ]] && printf "depend = %s\n" "${depends[@]}" + [[ $optdepends ]] && printf "optdepend = %s\n" "${optdepends[@]}" + [[ $conflicts ]] && printf "conflict = %s\n" "${conflicts[@]}" + [[ $provides ]] && printf "provides = %s\n" "${provides[@]}" + [[ $backup ]] && printf "backup = %s\n" "${backup[@]}"

for it in "${packaging_options[@]}"; do local ret="$(check_option $it)" -- 1.7.1

5047

Age (days ago)

5060

Last active (days ago)

List overview

Download

80 comments

6 participants

participants (6)

Allan McRae
Andres P
Cedric Staniewski
Dan McGee
Loui Chang
Nezmer