Re: [pacman-dev] [PATCH v5 4/4] libmakepkg: lint disallowed architecture specific variables
On Thu, Jan 24, 2019 at 09:09:59AM +, Morgan Adamiec wrote: > On Thu, 24 Jan 2019 at 03:01, Allan McRae 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 > > > --- > > > > > > 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. dR
Re: [pacman-dev] [PATCH] Check for all return values of _alpm_key_in_keychain
On 04/21/17 at 04:07pm, David Phillips wrote:
> This fixes a bug I encountered with a GPG keyring where the
> key id used to locate a key in the keyring was ambiguous within
> my keychain.
>
> This commit ensures that all valid return values are checked to
> catch this and related error cases rather than incorrectly taking
> an error case to mean the key was found, since this is rarely going
> to be the case.
> ---
> lib/libalpm/be_package.c | 17 ++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/lib/libalpm/be_package.c b/lib/libalpm/be_package.c
> index 7e8b7920..1891fa5a 100644
> --- a/lib/libalpm/be_package.c
> +++ b/lib/libalpm/be_package.c
> @@ -754,10 +754,21 @@ int SYMEXPORT alpm_pkg_load(alpm_handle_t *handle,
> const char *filename, int ful
> alpm_list_t *k;
> for(k = keys; k; k = k->next) {
> char *key = k->data;
> - if(_alpm_key_in_keychain(handle, key)
> == 0) {
> - if(_alpm_key_import(handle,
> key) == -1) {
> + switch(_alpm_key_in_keychain(handle,
> key)) {
> + case 1:
> + /* key is known;
> proceed */
> + break;
> + case 0:
> + /* key is unknown;
> attempt to import */
> +
> if(_alpm_key_import(handle, key) == -1) {
> + fail = 1;
> + }
> + break;
> + case -1:
> + /* error finding key in
> keychain */
> + default:
> fail = 1;
Just below this, an error message is printed if fail is true, saying
that the key is missing. If we got a generic code, that may not be
the case, and is the exact opposite of the problem that prompted
patch.
I'm wondering if this is necessary at all, though. This bit of code
is specifically just for importing missing keys. Any other
significant errors should be caught during the actual validation
attempt, where we already provide the actual gpg error message. If
the gpgme segmentation fault is no longer an issue, do we gain
anything other than bailing out slightly sooner by adding the extra
check and complexity for printing an appropriate error message here?
> - }
> + break;
> }
> }
> FREELIST(keys);
> --
> 2.12.2
Re: [pacman-dev] [PATCH v5 4/4] libmakepkg: lint disallowed architecture specific variables
On Thu, 24 Jan 2019 at 03:01, Allan McRae 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 > > --- > > > > 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?
