Re: [pacman-dev] [PATCH] Move signature payload creation to download engine

2020-06-10 Thread Allan McRae
On 14/5/20 8:15 am, Anatol Pomozov wrote:
> Until now callee of ALPM download functionality has been in charge of
> payload creation both for the main file (e.g. *.pkg) and for the accompanied
> *.sig file. One advantage of such solution is that all payloads are
> independent and can be fetched in parallel thus exploiting the maximum
> level of download parallelism.
> 
> To build *.sig file url we've been using a simple string concatenation:
> $requested_url + ".sig". Unfortunately there are cases when it does not
> work. For example an archlinux.org "Download From Mirror" link looks like
> this https://www.archlinux.org/packages/core/x86_64/bash/download/ and
> it gets redirected to some mirror. But if we append ".sig" to the end of
> the link url and try to download it then archlinux.org returns 404 error.
> 
> To overcome this issue we need to follow redirects for the main payload
> first, find the final url and only then append '.sig' suffix.
> This implies 2 things:
>  - the signature payload initialization need to be moved to dload.c
>  as it is the place where we have access to the resolved url
>  - *.sig is downloaded serially with the main payload and this reduces
>  level of parallelism
> 
> Move *.sig payload creation to dload.c. Once the main payload is fetched
> successfully we check if the callee asked to download the accompanied
> signature. If yes - create a new payload and add it to mcurl.
> 
> *.sig payload does not use server list of the main payload and thus does
> not support mirror failover. *.sig file comes from the same server as
> the main payload.
> 
> Refactor event loop in curl_multi_download_internal() a bit. Instead of
> relying on curl_multi_check_finished_download() to return number of new
> payloads we simply rerun the loop iteration one more time to check if
> there are any active downloads left.
> ---
>  lib/libalpm/be_sync.c | 34 +++-
>  lib/libalpm/dload.c   | 91 ++-
>  lib/libalpm/dload.h   |  4 +-
>  3 files changed, 65 insertions(+), 64 deletions(-)
> 
> diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c
> index 82018e15..41675d21 100644
> --- a/lib/libalpm/be_sync.c
> +++ b/lib/libalpm/be_sync.c
> @@ -180,12 +180,10 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle, 
> alpm_list_t *dbs, int force)
>   dbforce = 1;
>   }
>  
> - CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, 
> ALPM_ERR_MEMORY, cleanup));
> + siglevel = alpm_db_get_siglevel(db);
>  
> - /* set hard upper limit of 128 MiB */
> - payload->max_size = 128 * 1024 * 1024;
> + CALLOC(payload, 1, sizeof(*payload), GOTO_ERR(handle, 
> ALPM_ERR_MEMORY, cleanup));
>   payload->servers = db->servers;
> -
>   /* print server + filename into a buffer */
>   len = strlen(db->treename) + strlen(dbext) + 1;
>   MALLOC(payload->filepath, len,
> @@ -194,31 +192,11 @@ int SYMEXPORT alpm_db_update(alpm_handle_t *handle, 
> alpm_list_t *dbs, int force)
>   payload->handle = handle;
>   payload->force = dbforce;
>   payload->unlink_on_fail = 1;
> -
> + payload->download_signature = (siglevel & ALPM_SIG_DATABASE);
> + payload->signature_optional = (siglevel & 
> ALPM_SIG_DATABASE_OPTIONAL);
> + /* set hard upper limit of 128 MiB */
> + payload->max_size = 128 * 1024 * 1024;
>   payloads = alpm_list_add(payloads, payload);

OK.

> -
> - siglevel = alpm_db_get_siglevel(db);
> - if(siglevel & ALPM_SIG_DATABASE) {
> - struct dload_payload *sig_payload;
> - CALLOC(sig_payload, 1, sizeof(*sig_payload), 
> GOTO_ERR(handle, ALPM_ERR_MEMORY, cleanup));
> - sig_payload->signature = 1;
> -
> - /* print filename into a buffer (leave space for 
> separator and .sig) */
> - len = strlen(db->treename) + strlen(dbext) + 5;
> - MALLOC(sig_payload->filepath, len,
> - FREE(sig_payload); GOTO_ERR(handle, 
> ALPM_ERR_MEMORY, cleanup));
> - snprintf(sig_payload->filepath, len, "%s%s.sig", 
> db->treename, dbext);
> -
> - sig_payload->handle = handle;
> - sig_payload->force = dbforce;
> - sig_payload->errors_ok = (siglevel & 
> ALPM_SIG_DATABASE_OPTIONAL);
> -
> - /* set hard upper limit of 16 KiB */
> - sig_payload->max_size = 16 * 1024;
> - sig_payload->servers = db->servers;
> -
> - payloads = alpm_list_add(payloads, sig_payload);
> - }
>   }
>  
>   event.type = ALPM_EVENT_DB_RETRIEVE_START;

OK.

> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 4dbb011f..b68dcf6d 100644
> --- 

Re: [pacman-dev] [PATCH] Cleanup the old sequential download code

2020-06-10 Thread Allan McRae
On 12/5/20 3:16 am, Anatol Pomozov wrote:
> All users of _alpm_download() have been refactored to the new API.
> It is time to remove the old _alpm_download() functionality now.
> 
> This change also removes obsolete SIGPIPE signal handler functionality
> (this is a leftover from libfetch days).
> 
> Signed-off-by: Anatol Pomozov 
> ---
>  lib/libalpm/dload.c | 323 +---
>  lib/libalpm/dload.h |   4 -
>  2 files changed, 3 insertions(+), 324 deletions(-)
> 
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 43fe9847..4dbb011f 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -78,10 +78,6 @@ enum {
>  };
>  
>  static int dload_interrupted;
> -static void inthandler(int UNUSED signum)
> -{
> - dload_interrupted = ABORT_SIGINT;
> -}
>  
>  static int dload_progress_cb(void *file, curl_off_t dltotal, curl_off_t 
> dlnow,
>   curl_off_t UNUSED ultotal, curl_off_t UNUSED ulnow)
> @@ -236,8 +232,7 @@ static size_t dload_parseheader_cb(void *ptr, size_t 
> size, size_t nmemb, void *u
>   return realsize;
>  }
>  
> -static void curl_set_handle_opts(struct dload_payload *payload,
> - CURL *curl, char *error_buffer)
> +static void curl_set_handle_opts(CURL *curl, struct dload_payload *payload)
>  {

This change seems beyond the scope for this patch.  Can you split that
and the other related changes below into a separate patch.

Otherwise fine.

A


Re: [pacman-dev] [PATCH] Convert '-U pkg1 pkg2' codepath to parallel download

2020-06-10 Thread Allan McRae
On 11/5/20 6:50 pm, Anatol Pomozov wrote:
> Installing remote packages using its URL is an interesting case for ALPM
> API. Unlike package sync ('pacman -S pkg1 pkg2') '-U' does not deal with
> server mirror list. Thus _alpm_multi_download() should be able to
> handle file download for payloads that either have 'fileurl' field
> or pair of fields ('servers' and 'filepath') set.
> 
> Signature for alpm_fetch_pkgurl() has changed and it accepts an
> output list that is populated with filepaths to fetched packages.
> 
> Signed-off-by: Anatol Pomozov 

Thanks. Very minor comments belown.

> ---
>  lib/libalpm/alpm.h   |  11 ++-
>  lib/libalpm/dload.c  | 204 +--
>  lib/libalpm/dload.h  |   7 +-
>  src/pacman/upgrade.c | 102 ++
>  4 files changed, 181 insertions(+), 143 deletions(-)
> 
> diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h
> index 3ea66ccc..c6a13273 100644
> --- a/lib/libalpm/alpm.h
> +++ b/lib/libalpm/alpm.h
> @@ -755,12 +755,15 @@ typedef void (*alpm_cb_totaldl)(off_t total);
>  typedef int (*alpm_cb_fetch)(const char *url, const char *localpath,
>   int force);
>  
> -/** Fetch a remote pkg.
> +/** Fetch a list of remote packages.
>   * @param handle the context handle
> - * @param url URL of the package to download
> - * @return the downloaded filepath on success, NULL on error
> + * @param urls list of package URLs to download
> + * @param fetched list of filepaths to the fetched packages, each item
> + *corresponds to one in `urls` list

Can you make it clear that this is filled by the function, and not
something that should be passed.

> + * @return 0 on success or -1 on failure
>   */
> -char *alpm_fetch_pkgurl(alpm_handle_t *handle, const char *url);
> +int alpm_fetch_pkgurl(alpm_handle_t *handle, const alpm_list_t *urls,
> +   alpm_list_t **fetched);
>  
>  /** @addtogroup alpm_api_options Options
>   * Libalpm option getters and setters
> diff --git a/lib/libalpm/dload.c b/lib/libalpm/dload.c
> index 13aa4086..43fe9847 100644
> --- a/lib/libalpm/dload.c
> +++ b/lib/libalpm/dload.c
> @@ -613,7 +613,9 @@ static int curl_multi_retry_next_server(CURLM *curlm, 
> CURL *curl, struct dload_p
>   size_t len;
>   alpm_handle_t *handle = payload->handle;
>  
> - payload->servers = payload->servers->next;
> + if(payload->servers) {
> + payload->servers = payload->servers->next;
> + }

OK.

>   if(!payload->servers) {
>   _alpm_log(payload->handle, ALPM_LOG_DEBUG,
>   "%s: no more servers to retry\n", 
> payload->remote_name);
> @@ -622,7 +624,7 @@ static int curl_multi_retry_next_server(CURLM *curlm, 
> CURL *curl, struct dload_p
>   server = payload->servers->data;
>  
>   /* regenerate a new fileurl */
> - free(payload->fileurl);
> + FREE(payload->fileurl);

Change unrelated to this patch, but OK.

>   len = strlen(server) + strlen(payload->filepath) + 2;
>   MALLOC(payload->fileurl, len, RET_ERR(handle, ALPM_ERR_MEMORY, -1));
>   snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> @@ -849,20 +851,28 @@ static int curl_multi_add_payload(alpm_handle_t 
> *handle, CURLM *curlm,
>   struct dload_payload *payload, const char *localpath)
>  {
>   size_t len;
> - const char *server;
>   CURL *curl = NULL;
>   char hostname[HOSTNAME_SIZE];
>   int ret = -1;
>  
> - ASSERT(payload->servers, RET_ERR(handle, ALPM_ERR_SERVER_NONE, -1));
> - server = payload->servers->data;
> -
>   curl = curl_easy_init();
>   payload->curl = curl;
>  
> - len = strlen(server) + strlen(payload->filepath) + 2;
> - MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY, 
> cleanup));
> - snprintf(payload->fileurl, len, "%s/%s", server, payload->filepath);
> + if(payload->fileurl) {
> + ASSERT(!payload->servers, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, 
> cleanup));
> + ASSERT(!payload->filepath, GOTO_ERR(handle, 
> ALPM_ERR_WRONG_ARGS, cleanup));

OK.

> + } else {
> + const char *server;
> +
> + ASSERT(payload->servers, GOTO_ERR(handle, ALPM_ERR_SERVER_NONE, 
> cleanup));
> + ASSERT(payload->filepath, GOTO_ERR(handle, ALPM_ERR_WRONG_ARGS, 
> cleanup));
> +
> + server = payload->servers->data;
> +
> + len = strlen(server) + strlen(payload->filepath) + 2;
> + MALLOC(payload->fileurl, len, GOTO_ERR(handle, ALPM_ERR_MEMORY, 
> cleanup));
> + snprintf(payload->fileurl, len, "%s/%s", server, 
> payload->filepath);
> + }
>  

OK.

>   payload->tempfile_openmode = "wb";
>   if(!payload->remote_name) {
> @@ -1046,22 +1056,29 @@ int _alpm_multi_download(alpm_handle_t *handle,
>   alpm_list_t *s;
>   int success = 0;
>  
> - for(s = payload->servers; s; s = s->next) {
> -   

[pacman-dev] [GIT] The official pacman repository branch, master, updated. v5.2.1-95-g02ae97b0

2020-06-10 Thread Allan McRae
This is an automated email from the git hooks/post-receive script. It was
generated because a ref change was pushed to the repository containing
the project "The official pacman repository".

The branch, master has been updated
   via  02ae97b0da220d9079c6c2c1ac3e3ab0d12c1ac7 (commit)
   via  899d39b635d46f9e2daff1aada75ea07f08fef64 (commit)
   via  bf458cced7c0845f7b6fabb887d3878ae4cd51b2 (commit)
   via  381e11375569fa7588b1297e0e744749bdafe8f5 (commit)
   via  817f9fb715b4e90d28bc141dfccfc530b9e951dc (commit)
   via  3bd88821bbfc8066a63ddfc9959e78984bc17750 (commit)
   via  e348ba38814c9cc7c1c9892d0451096234dc39ab (commit)
   via  40bbaead44db8fcdef7087f4b05820bf90dee090 (commit)
  from  5f6ef895b1dac04c7fb1b63acab2d42c19f91922 (commit)

Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.

- Log -
commit 02ae97b0da220d9079c6c2c1ac3e3ab0d12c1ac7
Author: Eli Schwartz 
Date:   Mon Jun 8 22:03:18 2020 -0400

makepkg/repo-add: do not accept public-only keys for signing

If it's not listed by --list-secret-key we don't care if it has been
imported into your keyring, it's unusable. And you might not have a
private key at all in the no-keyid-specified case.

Signed-off-by: Eli Schwartz 
Signed-off-by: Allan McRae 

commit 899d39b635d46f9e2daff1aada75ea07f08fef64
Author: Eli Schwartz 
Date:   Mon Jun 8 21:59:18 2020 -0400

makepkg/repo-add: handle GPGKEY with spaces

We pass this to gpg -u and this gpg option can accept a number of
different formats, not just the historical hexadecimal fingerprint we
assumed. We should not barf hard if a format is used which happens to
contain spaces.

This also fixes a validation bug. When we initially check if the desired
key is available, we don't quote spaces, so gpg goes ahead and treats
each space-separated string as a *different key* to search for,
returning partial matches, and returning success if at least one key is
found. But gpg --detach-sign -u will certainly not accept multiple keys!

Fixes FS#66949

Signed-off-by: Eli Schwartz 
Signed-off-by: Allan McRae 

commit bf458cced7c0845f7b6fabb887d3878ae4cd51b2
Author: Eli Schwartz 
Date:   Tue Jun 2 17:50:24 2020 -0400

libmakepkg: fix regression in sending plain() output to stderr

In commit 882e707e40bbade0111cf3bdedbdac4d4b70453b we changed message
output to go to stdout by default, unless it was an error. The plain()
function doesn't *look* like an error function, but in practice it was
-- it's used to continue multiline messages, and all in-tree uses were
for warning/error.

This is a problem both because we're sending output to the wrong place,
and because in some cases, we were performing error logging from a
function which would otherwise return a value to be captured in a
variable using command substution.

Fix this and straighten out the API by providing two functions: one for
continuing msg output, and one which wraps this by sending output to
stderr, for continuing error output.

Change all callers to use the second function.

commit 381e11375569fa7588b1297e0e744749bdafe8f5
Author: Eli Schwartz 
Date:   Tue Jun 2 18:16:48 2020 -0400

makepkg: correctly handle missing download clients

This was broken in commit 882e707e40bbade0111cf3bdedbdac4d4b70453b,
which changed 'plain()' messages to go to stdout, which was then
captured as the download client in question: cmdline=("Aborting...").

The result was a very confusing error message e.g.

/usr/share/makepkg/source/file.sh: line 72: $'\E[1m': command not found

or with makepkg --nocolor:

/usr/share/makepkg/source/file.sh: line 72: Aborting...: command not found

The problem here is that we checked to see if an asynchronous subshell,
in our case <(...), failed, by checking if its captured stdout is
non-empty. Which is terrible, and also a limitation of old bash. But
bash 4.4 can use wait $! to retrieve the return value of an asynchronous
subshell. Now we target that as our minimum, we can sanely handle errors
in such functions.

Losing error messages on stdout by capturing them in a variable instead
of printing them, continues to be a problem, but this will be fixed
systematically in a later commit.

Signed-off-by: Eli Schwartz 
Signed-off-by: Allan McRae 

commit 817f9fb715b4e90d28bc141dfccfc530b9e951dc
Author: Eli Schwartz 
Date:   Mon May 25 23:52:16 2020 -0400

makepkg: guard against undefined git pinned sources

If something like source=(..."#commit=") is used, e.g. due to failed
variable expansion, we try to check out an empty refspec as nothing at
all, and end 

Re: [pacman-dev] [PATCH v2] srcinfo.sh: remove trailing newline

2020-06-10 Thread Denton Liu
Sorry, small typo:

On Wed, Jun 10, 2020 at 10:14:07PM -0400, Denton Liu wrote:
> When a .SRCINFO file is generated via `makepkg --printsrcinfo`, each
> section is concluded with an empty line. This means that at the end of
> the file, an empty line remains. This is considered a trailing
> whitespace error. In fact, `git diff --check` will about this saying

s/will/will warn/

> "new blank line at EOF."
> 
> Instead of closing each section off with an empty line, use the empty
> line to separate sections, omitting the empty line at the end of the
> file.


[pacman-dev] [PATCH v2] srcinfo.sh: remove trailing newline

2020-06-10 Thread Denton Liu
When a .SRCINFO file is generated via `makepkg --printsrcinfo`, each
section is concluded with an empty line. This means that at the end of
the file, an empty line remains. This is considered a trailing
whitespace error. In fact, `git diff --check` will about this saying
"new blank line at EOF."

Instead of closing each section off with an empty line, use the empty
line to separate sections, omitting the empty line at the end of the
file.
---
 scripts/libmakepkg/srcinfo.sh.in | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/scripts/libmakepkg/srcinfo.sh.in b/scripts/libmakepkg/srcinfo.sh.in
index 6e783279..d1e39f7d 100644
--- a/scripts/libmakepkg/srcinfo.sh.in
+++ b/scripts/libmakepkg/srcinfo.sh.in
@@ -30,7 +30,7 @@ srcinfo_open_section() {
printf '%s = %s\n' "$1" "$2"
 }
 
-srcinfo_close_section() {
+srcinfo_separate_section() {
echo
 }
 
@@ -94,7 +94,6 @@ srcinfo_write_global() {
 
srcinfo_open_section 'pkgbase' "${pkgbase:-$pkgname}"
srcinfo_write_section_details ''
-   srcinfo_close_section
 }
 
 srcinfo_write_package() {
@@ -104,7 +103,6 @@ srcinfo_write_package() {
 
srcinfo_open_section 'pkgname' "$1"
srcinfo_write_section_details "$1"
-   srcinfo_close_section
 }
 
 write_srcinfo_header() {
@@ -118,6 +116,7 @@ write_srcinfo_content() {
srcinfo_write_global
 
for pkg in "${pkgname[@]}"; do
+   srcinfo_separate_section
srcinfo_write_package "$pkg"
done
 }
-- 
2.27.0.107.g134631ef42


Re: [pacman-dev] [PATCH] makepkg: guard against undefined git pinned sources

2020-06-10 Thread Eli Schwartz
On 6/10/20 8:51 PM, Allan McRae wrote:
> On 26/5/20 1:52 pm, Eli Schwartz wrote:
>> If something like source=(..."#commit=") is used, e.g. due to failed
>> variable expansion, we try to check out an empty refspec as nothing at
>> all, and end up just running "git checkout". This happens because we
>> fail at variable expansion too -- so let's quote our variables properly
>> and make sure git sees this as an empty refspec, so it can error out.
>>
>> Also make sure it is interpreted as a ref instead of a path.
>>
>> Signed-off-by: Eli Schwartz 
>> ---
>>
>> This ensures that something like https://bugs.archlinux.org/task/66729
>> cannot happen again.
>>
> 
> Patch good.
> 
> Worth checking if this can happen with other VCS too.

Ironically (considering git is the primary VCS which we use and test) it
seems like all the other VCSes quote their args.

Except for svn, which runs

svn update -r ${ref}

and the -r requires an argument and fails if it is not provided.

-- 
Eli Schwartz
Bug Wrangler and Trusted User



signature.asc
Description: OpenPGP digital signature


Re: [pacman-dev] [PATCH v2] makepkg/repo-add: handle GPGKEY with spaces

2020-06-10 Thread Allan McRae
On 9/6/20 11:59 am, Eli Schwartz wrote:
> We pass this to gpg -u and this gpg option can accept a number of
> different formats, not just the historical hexadecimal fingerprint we
> assumed. We should not barf hard if a format is used which happens to
> contain spaces.
> 
> This also fixes a validation bug. When we initially check if the desired
> key is available, we don't quote spaces, so gpg goes ahead and treats
> each space-separated string as a *different key* to search for,
> returning partial matches, and returning success if at least one key is
> found. But gpg --detach-sign -u will certainly not accept multiple keys!
> 
> Fixes FS#66949
> 
> Signed-off-by: Eli Schwartz 
> ---
> 
> v2: fix case of GPGKEY="" with signing enabled reporting that no keys
> exist in the keyring. Only expand the quoted GPGKEY if it is non-empty.
>

Ack.


Re: [pacman-dev] [PATCH v2 2/2] libmakepkg: fix regression in sending plain() output to stderr

2020-06-10 Thread Allan McRae
On 3/6/20 8:16 am, Eli Schwartz wrote:
> In commit 882e707e40bbade0111cf3bdedbdac4d4b70453b we changed message
> output to go to stdout by default, unless it was an error. The plain()
> function doesn't *look* like an error function, but in practice it was
> -- it's used to continue multiline messages, and all in-tree uses were
> for warning/error.
> 
> This is a problem both because we're sending output to the wrong place,
> and because in some cases, we were performing error logging from a
> function which would otherwise return a value to be captured in a
> variable using command substution.
> 
> Fix this and straighten out the API by providing two functions: one for
> continuing msg output, and one which wraps this by sending output to
> stderr, for continuing error output.
> 
> Change all callers to use the second function.
> 
> Signed-off-by: Eli Schwartz 
> ---

I did not see this patch before reviewing v1.  This approach also looks
fine to me.  Pulling this version.

Allan


Re: [pacman-dev] [PATCH] makepkg: guard against undefined git pinned sources

2020-06-10 Thread Allan McRae
On 26/5/20 1:52 pm, Eli Schwartz wrote:
> If something like source=(..."#commit=") is used, e.g. due to failed
> variable expansion, we try to check out an empty refspec as nothing at
> all, and end up just running "git checkout". This happens because we
> fail at variable expansion too -- so let's quote our variables properly
> and make sure git sees this as an empty refspec, so it can error out.
> 
> Also make sure it is interpreted as a ref instead of a path.
> 
> Signed-off-by: Eli Schwartz 
> ---
> 
> This ensures that something like https://bugs.archlinux.org/task/66729
> cannot happen again.
> 

Patch good.

Worth checking if this can happen with other VCS too.

A


Re: [pacman-dev] [PATCH] build: add libintl dependency to meson and the .pc file

2020-06-10 Thread Allan McRae
On 19/5/20 5:18 am, Eli Schwartz wrote:
> In order to use gettext on systems where it is not part of libc, the
> correct linker flags are needed in libalpm.pc (for static compilation).
> This has never been the case.
> 
> The new meson build system currently only checks for ngettext in libc,
> but does not fall back to searching for the existence of -lintl; add it
> to the libalpm dependencies.
> 
> Signed-off-by: Eli Schwartz 
> ---
> 
> Fixes build on MSYS2, and possibly others. Part of
> https://github.com/msys2/MSYS2-packages/pull/1951
> 

>From that pull request, I can see the patch has been tested in the land
of MSYS2, so all good.

A


Re: [pacman-dev] [PATCH] makepkg: correctly handle missing download clients

2020-06-10 Thread Allan McRae
On 18/5/20 8:34 am, Eli Schwartz wrote:
> This was broken in commit 882e707e40bbade0111cf3bdedbdac4d4b70453b,
> which changed 'plain()' messages to go to stdout, which was then
> captured as the download client in question: cmdline=("Aborting...").
> 
> The result was a very confusing error message e.g.
> 
> /usr/share/makepkg/source/file.sh: line 72: $'\E[1m': command not found
> 
> or with makepkg --nocolor:
> 
> /usr/share/makepkg/source/file.sh: line 72: Aborting...: command not found
> 
> Solve this on two levels:
> - redirect this error() continuation to stderr so the user sees it.
> - catch erroring returns in get_downloadclient and propagate them
> 
> bash 4.4 can use wait $! to retrieve the return value of an asynchronous
> subshell such as <(...), which means, now we target that as our minimum,
> we can sanely handle errors in such functions.
> 
> Signed-off-by: Eli Schwartz 
> ---
> 
> Actually, maybe every use of plain() needs to do this. Or else make
> plain() do this by default. But it's technically used to "continue the
> previous message", and there's no guarantee it merits going to stderr,
> even though every time we use plain(), it does.
> > What to do...
> 

I can take this patch as is, but as you point out, this is a more
systemic issue with plain() always being used to follow error() or
warning() so all current usages should be on stderr.   So I'd like two
patches - one dealing with global stderr output issue, and one with the
$! usage.

Options for plain() are, we manually add >&2 when needed, or we provide
a wrapper function that does that.  My suggestion is:

plain() {
(( QUIET )) && return
local mesg=$1; shift
printf "${BOLD}${mesg}${ALL_OFF}\n" "$@"
}

bet rid of ${BOLD} for plain() and add a bold() function that prints to
stderr.

>  scripts/libmakepkg/source/file.sh.in | 1 +
>  scripts/libmakepkg/util/source.sh.in | 2 +-
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/libmakepkg/source/file.sh.in 
> b/scripts/libmakepkg/source/file.sh.in
> index 819320c2..28f373fb 100644
> --- a/scripts/libmakepkg/source/file.sh.in
> +++ b/scripts/libmakepkg/source/file.sh.in
> @@ -42,6 +42,7 @@ download_file() {
>   # find the client we should use for this URL
>   local -a cmdline
>   IFS=' ' read -a cmdline < <(get_downloadclient "$proto")
> + wait $! || exit
>   (( ${#cmdline[@]} )) || exit
>  
>   local filename=$(get_filename "$netfile")
> diff --git a/scripts/libmakepkg/util/source.sh.in 
> b/scripts/libmakepkg/util/source.sh.in
> index e0490661..1bf5b814 100644
> --- a/scripts/libmakepkg/util/source.sh.in
> +++ b/scripts/libmakepkg/util/source.sh.in
> @@ -165,7 +165,7 @@ get_downloadclient() {
>   if [[ ! -x $program ]]; then
>   local baseprog="${program##*/}"
>   error "$(gettext "The download program %s is not installed.")" 
> "$baseprog"
> - plain "$(gettext "Aborting...")"
> + plain "$(gettext "Aborting...")" >&2
>   exit 1 # $E_MISSING_PROGRAM
>   fi
>  
> 


Re: [pacman-dev] [PATCH] ci: cache packages

2020-06-10 Thread Allan McRae
On 21/5/20 9:38 am, Filipe Laíns wrote:
> Results in ~40s saved in each job.
> 
> Signed-off-by: Filipe Laíns 
> ---

Pulled.

Thanks,
A