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

[Allan McRae]

On 26/1/19 9:40 am, Morgan Adamiec wrote:
> 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'.
>

Those names are good.

BTW, I used 'variable' in my suggested naming because it is an array of
variable names.  So not referring to the array, but what is in the array.

Thanks,
Allan