[libvirt PATCH] ci: Makefile: Add distcheck target

Erik Skultety posted 1 patch 3 years, 10 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/15d90bddb2b265032aabe78e78706a0d403ffea9.1592388005.git.eskultet@redhat.com
ci/Makefile | 3 +++
1 file changed, 3 insertions(+)
[libvirt PATCH] ci: Makefile: Add distcheck target
Posted by Erik Skultety 3 years, 10 months ago
>From time to time we see failures in our gitlab pipelines that we as
developers don't see locally, simply because we don't run "make
distcheck" that often which is what the gitlab containers run and which
can indeed reveal mistakes like not shipping the necessary test data.
Since most of us prefer local testing over waiting for the gitlab
pipeline to finish, introduce the distcheck target to the CI which we
have inside the repo under ci/.

Signed-off-by: Erik Skultety <eskultet@redhat.com>
---
 ci/Makefile | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ci/Makefile b/ci/Makefile
index 7840cc8d89..e1c626ed34 100644
--- a/ci/Makefile
+++ b/ci/Makefile
@@ -239,6 +239,9 @@ ci-build@%:
 ci-check@%:
 	$(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_MAKE_ARGS="check"
 
+ci-distcheck@%:
+	$(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_MAKE_ARGS="distcheck"
+
 ci-list-images:
 	@echo
 	@echo "Available x86 container images:"
-- 
2.26.2

Re: [libvirt PATCH] ci: Makefile: Add distcheck target
Posted by Andrea Bolognani 3 years, 10 months ago
On Wed, 2020-06-17 at 12:00 +0200, Erik Skultety wrote:
> +ci-distcheck@%:
> +	$(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_MAKE_ARGS="distcheck"

I'm not sure we want to have such a proxy for all possible make
targets, especially since CI_MAKE_ARGS exists and is specifically
intended for the purpose of running

  $ make ci-build@centos-8 CI_MAKE_ARGS="syntax-check distcheck"

or whatever.

Anyway, if you're going to add new proxy targets please at least
document them in ci-help, so that users can find out about them
without having to dig into the Makefile.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] ci: Makefile: Add distcheck target
Posted by Erik Skultety 3 years, 10 months ago
On Wed, Jun 17, 2020 at 03:18:48PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-06-17 at 12:00 +0200, Erik Skultety wrote:
> > +ci-distcheck@%:
> > +	$(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_MAKE_ARGS="distcheck"
>
> I'm not sure we want to have such a proxy for all possible make
> targets, especially since CI_MAKE_ARGS exists and is specifically
> intended for the purpose of running
>
>   $ make ci-build@centos-8 CI_MAKE_ARGS="syntax-check distcheck"

I think that ship has sailed the moment we added ci-check.
What I'll do is respin this patch with extending the ci-help output as well as
adding CI_MAKE_ARGS to the help output, because like you said, one has to skim
through the Makefile to see what's available to them.

Erik

Re: [libvirt PATCH] ci: Makefile: Add distcheck target
Posted by Andrea Bolognani 3 years, 10 months ago
On Wed, 2020-06-17 at 16:05 +0200, Erik Skultety wrote:
> On Wed, Jun 17, 2020 at 03:18:48PM +0200, Andrea Bolognani wrote:
> > On Wed, 2020-06-17 at 12:00 +0200, Erik Skultety wrote:
> > > +ci-distcheck@%:
> > > +	$(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_MAKE_ARGS="distcheck"
> > 
> > I'm not sure we want to have such a proxy for all possible make
> > targets, especially since CI_MAKE_ARGS exists and is specifically
> > intended for the purpose of running
> > 
> >   $ make ci-build@centos-8 CI_MAKE_ARGS="syntax-check distcheck"
> 
> I think that ship has sailed the moment we added ci-check.
> What I'll do is respin this patch with extending the ci-help output as well as
> adding CI_MAKE_ARGS to the help output, because like you said, one has to skim
> through the Makefile to see what's available to them.

No ships have sailed, we can still make all the changes we deem
necessary as well as avoid those we consider undesirable: ci-check
existing does not mean that we have blanket permission for adding
ci-cov, ci-pdf, ci-mostlyclean and everything else.

Anyway, if you think ci-distcheck is worth having I will not stand
in the way - as long as you document it properly :)

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [libvirt PATCH] ci: Makefile: Add distcheck target
Posted by Erik Skultety 3 years, 10 months ago
On Wed, Jun 17, 2020 at 04:42:00PM +0200, Andrea Bolognani wrote:
> On Wed, 2020-06-17 at 16:05 +0200, Erik Skultety wrote:
> > On Wed, Jun 17, 2020 at 03:18:48PM +0200, Andrea Bolognani wrote:
> > > On Wed, 2020-06-17 at 12:00 +0200, Erik Skultety wrote:
> > > > +ci-distcheck@%:
> > > > +	$(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_MAKE_ARGS="distcheck"
> > >
> > > I'm not sure we want to have such a proxy for all possible make
> > > targets, especially since CI_MAKE_ARGS exists and is specifically
> > > intended for the purpose of running
> > >
> > >   $ make ci-build@centos-8 CI_MAKE_ARGS="syntax-check distcheck"
> >
> > I think that ship has sailed the moment we added ci-check.
> > What I'll do is respin this patch with extending the ci-help output as well as
> > adding CI_MAKE_ARGS to the help output, because like you said, one has to skim
> > through the Makefile to see what's available to them.
>
> No ships have sailed, we can still make all the changes we deem
> necessary as well as avoid those we consider undesirable: ci-check
> existing does not mean that we have blanket permission for adding
> ci-cov, ci-pdf, ci-mostlyclean and everything else.
>
> Anyway, if you think ci-distcheck is worth having I will not stand
> in the way - as long as you document it properly :)

I sent a patch for expanding the ci-help output which you've already reviewed
and I settled on the decision that it's enough to document it.
Consider this patch dropped.

Erik

Re: [libvirt PATCH] ci: Makefile: Add distcheck target
Posted by Michal Privoznik 3 years, 10 months ago
On 6/17/20 12:00 PM, Erik Skultety wrote:
>>From time to time we see failures in our gitlab pipelines that we as
> developers don't see locally, simply because we don't run "make
> distcheck" that often which is what the gitlab containers run and which
> can indeed reveal mistakes like not shipping the necessary test data.
> Since most of us prefer local testing over waiting for the gitlab
> pipeline to finish, introduce the distcheck target to the CI which we
> have inside the repo under ci/.
> 
> Signed-off-by: Erik Skultety <eskultet@redhat.com>
> ---
>   ci/Makefile | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/ci/Makefile b/ci/Makefile
> index 7840cc8d89..e1c626ed34 100644
> --- a/ci/Makefile
> +++ b/ci/Makefile
> @@ -239,6 +239,9 @@ ci-build@%:
>   ci-check@%:
>   	$(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_MAKE_ARGS="check"
>   
> +ci-distcheck@%:
> +	$(MAKE) -C $(CI_ROOTDIR) ci-build@$* CI_MAKE_ARGS="distcheck"
> +
>   ci-list-images:
>   	@echo
>   	@echo "Available x86 container images:"
> 

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>

Michal