[pacman-dev] [PATCH 3/7] libmakepkg: lint disallowed variables in package()

[Morgan Adamiec]

On Sun, 24 Jun 2018 at 03:26, Eli Schwartz <eschwartz at archlinux.org> wrote:
> But Allan's point seems to be that we shouldn't try to stop people from
> using variables that makepkg does not utilize, just because they look
> vaguely like something makepkg might have used.

The thing is makepkg does use these variables to some extent.

Sure pkgname_i686 is ignored. But things like makedepends_i686 do get
picked up. So the section would stay, I would instead just loop over a
different array of non overridable and non architecture specific
fields.

I'm all for stricter linting, It doesn't make sense to write
pkgname_i686, banning these kind of things would help catch mistakes.

> At a bare minimum, if we are going to ban them, the error message and
> entire handling logic should be merged into your next patch.

Sure I'll squash those two commits if you like.

> This error message would have the benefit of being the actual, true
> reason we're aborting. (You could check inside the package functions
> too, and extend the error message to refer to where they come from.)

Better error messages is something I like too. I'll leave it for
either after v2 or a different patch set altogether though. I'd like
to get the current issues reviewed and out the way first.


On Sun, 24 Jun 2018 at 03:26, Eli Schwartz <eschwartz at archlinux.org> wrote:
>
> On 06/23/2018 10:01 PM, Morgan Adamiec wrote:
> > Silly mistakes aside.
> >
> >> These are not variables being overridden...   pkgname_i686 is just not a
> >> thing as far as makepkg is concerned.
> >
> > The point of this is specifically disallow things like 'pkgname_i686'
> > rather than just ignore them.
>
> But Allan's point seems to be that we shouldn't try to stop people from
> using variables that makepkg does not utilize, just because they look
> vaguely like something makepkg might have used.
>
> At a bare minimum, if we are going to ban them, the error message and
> entire handling logic should be merged into your next patch.
>
> This error message would have the benefit of being the actual, true
> reason we're aborting. (You could check inside the package functions
> too, and extend the error message to refer to where they come from.)
>
>
> --
> Eli Schwartz
> Bug Wrangler and Trusted User
>