[pacman-dev,1/7] libmakepkg: disallow empty arch

Message ID 20180608181859.20724-2-morganamilo@gmail.com
State Rejected, archived
Headers show
Series Improve linting for makepkg | expand

Commit Message

morganamilo June 8, 2018, 6:18 p.m. UTC
We already ensure arch is an array but if arch is never defined
this error never triggers. Add an explicit check for a missing arch.

Signed-off-by: morganamilo <morganamilo@gmail.com>
---
 scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 5 +++++
 1 file changed, 5 insertions(+)

Comments

Eli Schwartz June 8, 2018, 7:33 p.m. UTC | #1
On 06/08/2018 02:18 PM, morganamilo wrote:
> We already ensure arch is an array but if arch is never defined
> this error never triggers. Add an explicit check for a missing arch.

Good catch! We'd usually abort with
==> ERROR: foo is not available for the 'x86_64' architecture.

but not if we're doing some IGNOREARCH operation that doesn't check
this, e.g. --source or --printsrcinfo or --packagelist

Or, makepkg --ignorearch.
Allan McRae June 19, 2018, 12:14 p.m. UTC | #2
On 09/06/18 04:18, morganamilo wrote:
> We already ensure arch is an array but if arch is never defined
> this error never triggers. Add an explicit check for a missing arch.
> 
> Signed-off-by: morganamilo <morganamilo@gmail.com>
> ---

OK.

>  scripts/libmakepkg/lint_pkgbuild/arch.sh.in | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> index ef1aac46..f2c80c73 100644
> --- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> +++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
> @@ -33,6 +33,11 @@ lint_pkgbuild_functions+=('lint_arch')
>  lint_arch() {
>  	local a name list ret=0
>  
> +	if [[ -z $arch ]]; then
> +		error "$(gettext "%s is not allowed to be empty.")" "arch"
> +		return 1
> +	fi
> +
>  	if [[ $arch == 'any' ]]; then
>  		return 0
>  	fi
>
Allan McRae June 19, 2018, 12:28 p.m. UTC | #3
On 09/06/18 05:33, Eli Schwartz wrote:
> On 06/08/2018 02:18 PM, morganamilo wrote:
>> We already ensure arch is an array but if arch is never defined
>> this error never triggers. Add an explicit check for a missing arch.
> 
> Good catch! We'd usually abort with
> ==> ERROR: foo is not available for the 'x86_64' architecture.
> 
> but not if we're doing some IGNOREARCH operation that doesn't check
> this, e.g. --source or --printsrcinfo or --packagelist
> 
> Or, makepkg --ignorearch.
> 

Hrm...  I just saw this after replying the patch was OK.

With this patch:

$ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch
==> ERROR: arch is not allowed to be empty.


I think our current check is OK here and this additional check is too
stringent.

Allan
morganamilo June 19, 2018, 7:26 p.m. UTC | #4
On Tue, 19 Jun 2018 at 13:28, Allan McRae <allan@archlinux.org> wrote:
>
> On 09/06/18 05:33, Eli Schwartz wrote:
> > On 06/08/2018 02:18 PM, morganamilo wrote:
> >> We already ensure arch is an array but if arch is never defined
> >> this error never triggers. Add an explicit check for a missing arch.
> >
> > Good catch! We'd usually abort with
> > ==> ERROR: foo is not available for the 'x86_64' architecture.
> >
> > but not if we're doing some IGNOREARCH operation that doesn't check
> > this, e.g. --source or --printsrcinfo or --packagelist
> >
> > Or, makepkg --ignorearch.
> >
>
> Hrm...  I just saw this after replying the patch was OK.
>
> With this patch:
>
> $ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch
> ==> ERROR: arch is not allowed to be empty.
>
>
> I think our current check is OK here and this additional check is too
> stringent.
>
> Allan

I would have to disagree. The man page (which I used as a reference to
most of these patches) explicitly states arch as a required field
along side pkgver, pkgrel and pkgname.

Given the existence of 'any', having no arch field makes no sense
under any circumstances.

Not having this check has lead to a number packages missing this field. See:

https://aur.archlinux.org/packages/highmoon/
https://aur.archlinux.org/packages/iview/
https://aur.archlinux.org/packages/mipsel-linux-gcc3/
https://aur.archlinux.org/packages/mipsel-linux-gcc3-initial/
https://aur.archlinux.org/packages/mipsel-linux-libstdc++5/
Eli Schwartz June 19, 2018, 7:54 p.m. UTC | #5
On 06/19/2018 08:28 AM, Allan McRae wrote:
> On 09/06/18 05:33, Eli Schwartz wrote:
>> On 06/08/2018 02:18 PM, morganamilo wrote:
>>> We already ensure arch is an array but if arch is never defined
>>> this error never triggers. Add an explicit check for a missing arch.
>>
>> Good catch! We'd usually abort with
>> ==> ERROR: foo is not available for the 'x86_64' architecture.
>>
>> but not if we're doing some IGNOREARCH operation that doesn't check
>> this, e.g. --source or --printsrcinfo or --packagelist
>>
>> Or, makepkg --ignorearch.
>>
> 
> Hrm...  I just saw this after replying the patch was OK.
> 
> With this patch:
> 
> $ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch
> ==> ERROR: arch is not allowed to be empty.
> 
> 
> I think our current check is OK here and this additional check is too
> stringent.
> 
> Allan

I'll agree with morganamilo here, if someone uses arch=(x86_64) and I
--ignorearch it to use it on armv7h, that's one thing, but if they don't
define it at all then --printsrcinfo succeeds but makepkg on its own fails.

IMHO that makes the patch worthwhile for linting --printsrcinfo, and in
the process if it results in --ignorearch failing for the same issue,
then I think it's a good thing on the grounds of technical correctness.

The PKGBUILD is fundamentally wrong to be missing required variables, it
shouldn't be "okay as long as you call makepkg with certain switches to
infer the arch", it doesn't work in the common case. Therefore it should
be fixed, and we should ensure that --printsrcinfo fails early.

Yes, anyone who tries to build the package will notice the issue. But
unfortunately many people who try writing PKGBUILDs will only ever run
--printsrcinfo and not try building it -- this patch prevents that from
occurring.
Allan McRae June 19, 2018, 11:52 p.m. UTC | #6
On 20/06/18 05:54, Eli Schwartz wrote:
> On 06/19/2018 08:28 AM, Allan McRae wrote:
>> On 09/06/18 05:33, Eli Schwartz wrote:
>>> On 06/08/2018 02:18 PM, morganamilo wrote:
>>>> We already ensure arch is an array but if arch is never defined
>>>> this error never triggers. Add an explicit check for a missing arch.
>>>
>>> Good catch! We'd usually abort with
>>> ==> ERROR: foo is not available for the 'x86_64' architecture.
>>>
>>> but not if we're doing some IGNOREARCH operation that doesn't check
>>> this, e.g. --source or --printsrcinfo or --packagelist
>>>
>>> Or, makepkg --ignorearch.
>>>
>>
>> Hrm...  I just saw this after replying the patch was OK.
>>
>> With this patch:
>>
>> $ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch
>> ==> ERROR: arch is not allowed to be empty.
>>
>>
>> I think our current check is OK here and this additional check is too
>> stringent.
>>
>> Allan
> 
> I'll agree with morganamilo here, if someone uses arch=(x86_64) and I
> --ignorearch it to use it on armv7h, that's one thing, but if they don't
> define it at all then --printsrcinfo succeeds but makepkg on its own fails.
> 
> IMHO that makes the patch worthwhile for linting --printsrcinfo, and in
> the process if it results in --ignorearch failing for the same issue,
> then I think it's a good thing on the grounds of technical correctness.
> 
> The PKGBUILD is fundamentally wrong to be missing required variables, it
> shouldn't be "okay as long as you call makepkg with certain switches to
> infer the arch", it doesn't work in the common case. Therefore it should
> be fixed, and we should ensure that --printsrcinfo fails early.
> 
> Yes, anyone who tries to build the package will notice the issue. But
> unfortunately many people who try writing PKGBUILDs will only ever run
> --printsrcinfo and not try building it -- this patch prevents that from
> occurring.
> 

man makepkg:

-A, --ignorearch
Ignore a missing or incomplete arch field in the build script.

The documentation defines the behaviour of --ignorearch as currently
specified.

Allan
morganamilo June 24, 2018, 12:41 a.m. UTC | #7
On Wed, 20 Jun 2018 at 00:52, Allan McRae <allan@archlinux.org> wrote:
>
> On 20/06/18 05:54, Eli Schwartz wrote:
> > On 06/19/2018 08:28 AM, Allan McRae wrote:
> >> On 09/06/18 05:33, Eli Schwartz wrote:
> >>> On 06/08/2018 02:18 PM, morganamilo wrote:
> >>>> We already ensure arch is an array but if arch is never defined
> >>>> this error never triggers. Add an explicit check for a missing arch.
> >>>
> >>> Good catch! We'd usually abort with
> >>> ==> ERROR: foo is not available for the 'x86_64' architecture.
> >>>
> >>> but not if we're doing some IGNOREARCH operation that doesn't check
> >>> this, e.g. --source or --printsrcinfo or --packagelist
> >>>
> >>> Or, makepkg --ignorearch.
> >>>
> >>
> >> Hrm...  I just saw this after replying the patch was OK.
> >>
> >> With this patch:
> >>
> >> $ /home/allan/arch/code/pacman/scripts/makepkg --ignorearch
> >> ==> ERROR: arch is not allowed to be empty.
> >>
> >>
> >> I think our current check is OK here and this additional check is too
> >> stringent.
> >>
> >> Allan
> >
> > I'll agree with morganamilo here, if someone uses arch=(x86_64) and I
> > --ignorearch it to use it on armv7h, that's one thing, but if they don't
> > define it at all then --printsrcinfo succeeds but makepkg on its own fails.
> >
> > IMHO that makes the patch worthwhile for linting --printsrcinfo, and in
> > the process if it results in --ignorearch failing for the same issue,
> > then I think it's a good thing on the grounds of technical correctness.
> >
> > The PKGBUILD is fundamentally wrong to be missing required variables, it
> > shouldn't be "okay as long as you call makepkg with certain switches to
> > infer the arch", it doesn't work in the common case. Therefore it should
> > be fixed, and we should ensure that --printsrcinfo fails early.
> >
> > Yes, anyone who tries to build the package will notice the issue. But
> > unfortunately many people who try writing PKGBUILDs will only ever run
> > --printsrcinfo and not try building it -- this patch prevents that from
> > occurring.
> >
>
> man makepkg:
>
> -A, --ignorearch
> Ignore a missing or incomplete arch field in the build script.
>
> The documentation defines the behaviour of --ignorearch as currently
> specified.
>
> Allan

While --ignorearch is documented correctly, flags such as
--printsrcinfo will not complain about a missing arch, which is
defined as a required field. I also believe --ignorearch should be
changed to not ignore a missing arch either.

Anyway, tonight I will start working on v2 of this patch set. I'll
remove this commit unless you change your mind.

Patch

diff --git a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
index ef1aac46..f2c80c73 100644
--- a/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
+++ b/scripts/libmakepkg/lint_pkgbuild/arch.sh.in
@@ -33,6 +33,11 @@  lint_pkgbuild_functions+=('lint_arch')
 lint_arch() {
 	local a name list ret=0
 
+	if [[ -z $arch ]]; then
+		error "$(gettext "%s is not allowed to be empty.")" "arch"
+		return 1
+	fi
+
 	if [[ $arch == 'any' ]]; then
 		return 0
 	fi