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

[Morgan Adamiec]

On Fri, 25 Jan 2019 at 16:05, Dave Reisner <d at falconindy.com> wrote:
>
> 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

I still like mine more, but I think `pkgbuild_schema_arrays` is fine.
I prefer it to 'variable'.