[pacman-dev] [PATCH v5 4/4] libmakepkg: lint disallowed architecture specific variables

[Dave Reisner]

On Fri, Jan 25, 2019 at 03:47:14PM +0000, Morgan Adamiec wrote:
> On Fri, 25 Jan 2019 at 00:52, Allan McRae <allan at archlinux.org> wrote:
> >
> > On 24/1/19 11:34 pm, Dave Reisner wrote:
> > > On Thu, Jan 24, 2019 at 09:09:59AM +0000, Morgan Adamiec wrote:
> > >> On Thu, 24 Jan 2019 at 03:01, Allan McRae <allan at archlinux.org> wrote:
> > >>>
> > >>> On 22/1/19 10:05 am, morganamilo wrote:
> > >>>> Variables such as 'pkgdesc_x86_64' are invalid, instead of ignoring them
> > >>>> raise an error.
> > >>>>
> > >>>> This also disallows using 'any' as an architecture specific variable
> > >>>>
> > >>>> Signed-off-by: morganamilo <morganamilo at gmail.com>
> > >>>> ---
> > >>>>
> > >>>> v5:
> > >>>>       "libmakepkg: disallow using any as an architecture specific variable"
> > >>>>       was squashed into this commit.
> > >>>>
> > >>>>       Move this lint to its own file.
> > >>>
> > >>> Moving this to its own file is fine in principle, but it has duplicated
> > >>> a few arrays of field values.   After this patch there would be:
> > >>>
> > >>> scripts/makepkg.sh.in:
> > >>>         splitpkg_overrides=(...
> > >>>
> > >>> scripts/libmakepkg/lint_pkgbuild/variable.sh.in:
> > >>> scripts/libmakepkg/lint_pkgbuild/arch_specific.sh.in:
> > >>>         local array=(...
> > >>>         local arch_array=(...
> > >>>         local string=(...
> > >>>
> > >>> scripts/libmakepkg/lint_pkgbuild/package_function_variable.sh.in:
> > >>>         local no_package=(...
> > >>>
> > >>> This will be annoying to update for any new fields or other changes.
> > >>>
> > >>>
> > >>> The properties of each field we are trying to capture are:
> > >>> 1) is an array/string
> > >>> 2) can be architecture specific
> > >>> 3) overridable in package function
> > >>>
> > >>> Can we store this in one file in a readily extendable fashion somewhere?
> > >>>
> > >>> A
> > >>
> > >> Good point. There was already a TODO on the fields, maybe it's finally
> > >> time to act on it.
> > >> I'm not too familiar with bash to really know a better way to structure it.
> > >> Would just moving the arrays to a different file and sourcing be fine?
> > >> util/pkgbuild.sh perhaps?
> > >
> > > Yes, this can be moved to another file and just be declared as arrays. I
> > > would suggest making sure that the names are fairly unique (and
> > > readonly) to avoid accidentally clobbering.
> > >
> > > This feels like semantics that we're overlaying on top of shell, so my vote
> > > is for util/schema.sh.
> > >
> >
> > For names, I suggest:
> >
> > pkgbuild_array_variable
> > pkgbuild_string_variable
> > pkgbuild_arch_override_variable
> > pkgbuild_package_override_variable
> >
> > Kind of long, but won't get clobbered.
> >
> > Allan
> 
> Ending a variable name with 'variable' seems weird to me. Also I
> prefer arrays to be plural.
> 
> So I'd prefer 'pkgbuild_array_variables' or even better
> `pkgbuild_arrarys`. But then of course
> the shorter name is more risky.
> 
> Still I'd put my vote on:
> 
> pkgbuild_arrays
> pkgbuild_strings
> pkgbuild_arch_overrides
> pkgbuild_package_overrides
> 
> Also seems as these are pretty much constants should they be in all
> caps? Or is that not
> done in bash?

All caps vars are usually reserved for environment vars (e.g. SHELL,
PAGER, LESS). You might capitalize PKGBUILD (as the prefix), but
capitalizing the whole thing is unnecessary.

Again, I'm going to harp on the fact that this feels like schema, so
paint it blue:

pkgbuild_schema_arrays
pkgbuild_schema_strings
pkgbuild_schema_arch_overrides
pkgbuild_schema_package_overrides

dR