| Message ID | 20211010234655.585526-1-christian@paral.in |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [PATCH-NEXT,v3,1/6] support/download/dl-wrapper: add concept of download post-processing | expand |
Hello Christian, Thanks for your work on this topic! I didn't review the patches in details, but here are a couple of comments on the form: - The initial series also had support for Cargo, I think we want to retain that as the idea was to solve both Go and Cargo at the same time, to make sure we have a solution that works for both. - PATCH-NEXT doesn't make any sense right now, there is no "next" branch. - Your patch series lacks a cover letter with a description of the changes between versions. So of your commits have a v1 -> v2 changelog, but we don't know what changed in v3. Best regards, Thomas On Sun, 10 Oct 2021 16:46:50 -0700 Christian Stewart <christian@paral.in> wrote: > From: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > > In order to support package managers such as Cargo (Rust) or Go, we > want to run some custom logic after the main download, but before > packing the tarball and checking the hash. > > To implement this, this commit introduces a concept of download > post-processing: if -p <something> is passed to the dl-wrapper, then > support/download/<something>-post-process will be called. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > Signed-off-by: Christian Stewart <christian@paral.in> > --- > support/download/dl-wrapper | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper > index 3315bd410e..2d74554213 100755 > --- a/support/download/dl-wrapper > +++ b/support/download/dl-wrapper > @@ -25,7 +25,7 @@ main() { > local -a uris > > # Parse our options; anything after '--' is for the backend > - while getopts ":c:d:D:o:n:N:H:rf:u:q" OPT; do > + while getopts ":c:d:D:o:n:N:H:rf:u:qp:" OPT; do > case "${OPT}" in > c) cset="${OPTARG}";; > d) dl_dir="${OPTARG}";; > @@ -37,6 +37,7 @@ main() { > r) recurse="-r";; > f) filename="${OPTARG}";; > u) uris+=( "${OPTARG}" );; > + p) post_process="${OPTARG}";; > q) quiet="-q";; > :) error "option '%s' expects a mandatory argument\n" "${OPTARG}";; > \?) error "unknown option '%s'\n" "${OPTARG}";; > @@ -135,6 +136,12 @@ main() { > continue > fi > > + if [ -n "${post_process}" ] ; then > + ${OLDPWD}/support/download/${post_process}-post-process \ > + -o "${tmpf}" \ > + -n "${raw_base_name}" > + fi > + > # cd back to free the temp-dir, so we can remove it later > cd "${OLDPWD}" >
Hi Thomas, On Mon, Oct 11, 2021 at 12:04 AM Thomas Petazzoni <thomas.petazzoni@bootlin.com> wrote: > > Hello Christian, > > Thanks for your work on this topic! I didn't review the patches in > details, but here are a couple of comments on the form: > > - The initial series also had support for Cargo, I think we want to > retain that as the idea was to solve both Go and Cargo at the same > time, to make sure we have a solution that works for both. I don't have the experience with Rust to sign off on the Cargo portion, and I was unsure if it was confirmed to be ready for merging. So in the interest of getting these packages in (including gocryptfs) I sent the Go portion only. Can send the full thing with the Cargo as well... if really necessary, but can't we just do it in 2 patch series? > - PATCH-NEXT doesn't make any sense right now, there is no "next" > branch. Ok, was just indicating that it's somewhat "next release" material (not for backport), I guess. > - Your patch series lacks a cover letter with a description of the > changes between versions. So of your commits have a v1 -> v2 > changelog, but we don't know what changed in v3. I just dropped a "TODO." Best regards, Christian
Hello Christian, On Mon, 11 Oct 2021 00:13:02 -0700 Christian Stewart <christian@paral.in> wrote: > > - The initial series also had support for Cargo, I think we want to > > retain that as the idea was to solve both Go and Cargo at the same > > time, to make sure we have a solution that works for both. > > I don't have the experience with Rust to sign off on the Cargo > portion, and I was unsure if it was confirmed to be ready for merging. > So in the interest of getting these packages in (including gocryptfs) > I sent the Go portion only. When I sent the series, I had neither the experience with Go nor Rust :-) > Can send the full thing with the Cargo as well... if really necessary, > but can't we just do it in 2 patch series? The idea of having a single series was to make sure we are able to implement a mechanism that works (at least) for two different vendoring-based package management systems. > > - PATCH-NEXT doesn't make any sense right now, there is no "next" > > branch. > > Ok, was just indicating that it's somewhat "next release" material > (not for backport), I guess. Whether something is to be considered for backport or not is a decision that Peter Korsgaard does. And new features are by nature generally not considered for backporting. Cheers! Thomas
On Sun, 10 Oct 2021 16:46:50 -0700 Christian Stewart via buildroot <buildroot@buildroot.org> wrote: > From: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > > In order to support package managers such as Cargo (Rust) or Go, we > want to run some custom logic after the main download, but before > packing the tarball and checking the hash. > > To implement this, this commit introduces a concept of download > post-processing: if -p <something> is passed to the dl-wrapper, then > support/download/<something>-post-process will be called. > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@bootlin.com> > Signed-off-by: Christian Stewart <christian@paral.in> > --- > support/download/dl-wrapper | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) I have marked this patch series as Superseded, as I have posted: https://patchwork.ozlabs.org/project/buildroot/list/?series=279698 which replaces it. Thomas
diff --git a/support/download/dl-wrapper b/support/download/dl-wrapper index 3315bd410e..2d74554213 100755 --- a/support/download/dl-wrapper +++ b/support/download/dl-wrapper @@ -25,7 +25,7 @@ main() { local -a uris # Parse our options; anything after '--' is for the backend - while getopts ":c:d:D:o:n:N:H:rf:u:q" OPT; do + while getopts ":c:d:D:o:n:N:H:rf:u:qp:" OPT; do case "${OPT}" in c) cset="${OPTARG}";; d) dl_dir="${OPTARG}";; @@ -37,6 +37,7 @@ main() { r) recurse="-r";; f) filename="${OPTARG}";; u) uris+=( "${OPTARG}" );; + p) post_process="${OPTARG}";; q) quiet="-q";; :) error "option '%s' expects a mandatory argument\n" "${OPTARG}";; \?) error "unknown option '%s'\n" "${OPTARG}";; @@ -135,6 +136,12 @@ main() { continue fi + if [ -n "${post_process}" ] ; then + ${OLDPWD}/support/download/${post_process}-post-process \ + -o "${tmpf}" \ + -n "${raw_base_name}" + fi + # cd back to free the temp-dir, so we can remove it later cd "${OLDPWD}"