[libvirt] [PATCH] examples: Fix installation on Windows

Andrea Bolognani posted 1 patch 10 weeks ago
Test syntax-check passed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20190513125514.21222-1-abologna@redhat.com
examples/Makefile.am | 35 ++++++++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 7 deletions(-)

[libvirt] [PATCH] examples: Fix installation on Windows

Posted by Andrea Bolognani 10 weeks ago
We can't rely on $(noinst_PROGRAMS) retaining its original
value, so let's use a separate $(EXAMPLES) variable instead.

Signed-off-by: Andrea Bolognani <abologna@redhat.com>
---
 examples/Makefile.am | 35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/examples/Makefile.am b/examples/Makefile.am
index c7688c7c3d..11c9f44917 100644
--- a/examples/Makefile.am
+++ b/examples/Makefile.am
@@ -34,11 +34,32 @@ LDADD = $(STATIC_BINARIES) $(WARN_CFLAGS) \
 	$(top_builddir)/src/libvirt.la \
 	$(top_builddir)/src/libvirt-admin.la
 
-noinst_PROGRAMS=dominfo/info1 dommigrate/dommigrate domsuspend/suspend \
-	domtop/domtop hellolibvirt/hellolibvirt object-events/event-test \
-	openauth/openauth rename/rename admin/list_servers admin/list_clients \
-	admin/threadpool_params admin/client_limits admin/client_info \
-	admin/client_close admin/logging
+# List of example programs. We need to list them here instead of using
+# $(noinst_PROGRAMS) directly because we want to have access to the
+# unmodified list during (un)installation, but at the same time automake
+# might tweak $(noinst_PROGRAMS) to eg. automatically add the .exe file
+# extension when targeting Windows.
+EXAMPLES = \
+	admin/client_close \
+	admin/client_info \
+	admin/client_limits \
+	admin/list_clients \
+	admin/list_servers \
+	admin/logging \
+	admin/threadpool_params \
+	dominfo/info1 \
+	dommigrate/dommigrate \
+	domsuspend/suspend \
+	domtop/domtop \
+	hellolibvirt/hellolibvirt \
+	object-events/event-test \
+	openauth/openauth \
+	rename/rename \
+	$(NULL)
+
+noinst_PROGRAMS = \
+	$(EXAMPLES) \
+	$(NULL)
 
 dominfo_info1_SOURCES = dominfo/info1.c
 dommigrate_dommigrate_SOURCES = dommigrate/dommigrate.c
@@ -88,13 +109,13 @@ examplesdir = $(docdir)/examples
 
 install-data-local: $(INSTALL_DATA_LOCAL)
 	$(mkinstalldirs) $(DESTDIR)$(examplesdir)
-	for p in $(noinst_PROGRAMS); do \
+	for p in $(EXAMPLES); do \
 		d=$$(dirname $$p); \
 		$(mkinstalldirs) $(DESTDIR)$(examplesdir)/$$d; \
 		$(INSTALL_DATA) $(srcdir)/$${p}.c $(DESTDIR)$(examplesdir)/$$d/; \
 	done
 
 uninstall-local: $(UNINSTALL_LOCAL)
-	for p in $(noinst_PROGRAMS); do \
+	for p in $(EXAMPLES); do \
 		rm -f $(DESTDIR)$(examplesdir)/$${p}.c; \
 	done
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] examples: Fix installation on Windows

Posted by Daniel P. Berrangé 9 weeks ago
On Mon, May 13, 2019 at 02:55:14PM +0200, Andrea Bolognani wrote:
> We can't rely on $(noinst_PROGRAMS) retaining its original
> value, so let's use a separate $(EXAMPLES) variable instead.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  examples/Makefile.am | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] examples: Fix installation on Windows

Posted by Daniel P. Berrangé 10 weeks ago
On Mon, May 13, 2019 at 02:55:14PM +0200, Andrea Bolognani wrote:
> We can't rely on $(noinst_PROGRAMS) retaining its original
> value, so let's use a separate $(EXAMPLES) variable instead.
> 
> Signed-off-by: Andrea Bolognani <abologna@redhat.com>
> ---
>  examples/Makefile.am | 35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> diff --git a/examples/Makefile.am b/examples/Makefile.am
> index c7688c7c3d..11c9f44917 100644
> --- a/examples/Makefile.am
> +++ b/examples/Makefile.am
> @@ -34,11 +34,32 @@ LDADD = $(STATIC_BINARIES) $(WARN_CFLAGS) \
>  	$(top_builddir)/src/libvirt.la \
>  	$(top_builddir)/src/libvirt-admin.la
>  
> -noinst_PROGRAMS=dominfo/info1 dommigrate/dommigrate domsuspend/suspend \
> -	domtop/domtop hellolibvirt/hellolibvirt object-events/event-test \
> -	openauth/openauth rename/rename admin/list_servers admin/list_clients \
> -	admin/threadpool_params admin/client_limits admin/client_info \
> -	admin/client_close admin/logging
> +# List of example programs. We need to list them here instead of using
> +# $(noinst_PROGRAMS) directly because we want to have access to the
> +# unmodified list during (un)installation, but at the same time automake
> +# might tweak $(noinst_PROGRAMS) to eg. automatically add the .exe file
> +# extension when targeting Windows.
> +EXAMPLES = \
> +	admin/client_close \
> +	admin/client_info \
> +	admin/client_limits \
> +	admin/list_clients \
> +	admin/list_servers \
> +	admin/logging \
> +	admin/threadpool_params \
> +	dominfo/info1 \
> +	dommigrate/dommigrate \
> +	domsuspend/suspend \
> +	domtop/domtop \
> +	hellolibvirt/hellolibvirt \
> +	object-events/event-test \
> +	openauth/openauth \
> +	rename/rename \
> +	$(NULL)
> +
> +noinst_PROGRAMS = \
> +	$(EXAMPLES) \
> +	$(NULL)
>  
>  dominfo_info1_SOURCES = dominfo/info1.c
>  dommigrate_dommigrate_SOURCES = dommigrate/dommigrate.c
> @@ -88,13 +109,13 @@ examplesdir = $(docdir)/examples

I wonder if we should just directly use the _SOURCES vars instead of making
the assumption that binary names match source files. eg

  EXAMPLE_SRCS = \
      $(dominfo_info1_SOURCES) \
      $(dommigrate_dommigrate_SOURCES) \
      .....

then


>  
>  install-data-local: $(INSTALL_DATA_LOCAL)
>  	$(mkinstalldirs) $(DESTDIR)$(examplesdir)
> -	for p in $(noinst_PROGRAMS); do \
> +	for p in $(EXAMPLES); do \

s/EXAMPLES/EXAMPLE_SRCS/

>  		d=$$(dirname $$p); \
>  		$(mkinstalldirs) $(DESTDIR)$(examplesdir)/$$d; \
>  		$(INSTALL_DATA) $(srcdir)/$${p}.c $(DESTDIR)$(examplesdir)/$$d/; \

s/.c//

>  	done
>  
>  uninstall-local: $(UNINSTALL_LOCAL)
> -	for p in $(noinst_PROGRAMS); do \
> +	for p in $(EXAMPLES); do \

s/EXAMPLES/EXAMPLE_SRCS/

>  		rm -f $(DESTDIR)$(examplesdir)/$${p}.c; \

s/.c//

>  	done

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] examples: Fix installation on Windows

Posted by Andrea Bolognani 10 weeks ago
On Mon, 2019-05-13 at 16:14 +0100, Daniel P. Berrangé wrote:
> On Mon, May 13, 2019 at 02:55:14PM +0200, Andrea Bolognani wrote:
> > +# List of example programs. We need to list them here instead of using
> > +# $(noinst_PROGRAMS) directly because we want to have access to the
> > +# unmodified list during (un)installation, but at the same time automake
> > +# might tweak $(noinst_PROGRAMS) to eg. automatically add the .exe file
> > +# extension when targeting Windows.
> > +EXAMPLES = \
> > +	admin/client_close \
> > +	admin/client_info \
> > +	admin/client_limits \
> > +	admin/list_clients \
> > +	admin/list_servers \
> > +	admin/logging \
> > +	admin/threadpool_params \
> > +	dominfo/info1 \
> > +	dommigrate/dommigrate \
> > +	domsuspend/suspend \
> > +	domtop/domtop \
> > +	hellolibvirt/hellolibvirt \
> > +	object-events/event-test \
> > +	openauth/openauth \
> > +	rename/rename \
> > +	$(NULL)
> > +
> > +noinst_PROGRAMS = \
> > +	$(EXAMPLES) \
> > +	$(NULL)
> 
> I wonder if we should just directly use the _SOURCES vars instead of making
> the assumption that binary names match source files. eg
> 
>   EXAMPLE_SRCS = \
>       $(dominfo_info1_SOURCES) \
>       $(dommigrate_dommigrate_SOURCES) \
>       .....

That would (probably, I haven't actually tested it) work, however it
seems to me like it would be much more likely that such a solution
would eventually fall out of sync than the one I'm proposing, since
when adding a new example you'd only notice the missing file if you
actively performed an installation and checked the contents of the
documentation directory, rather than not seeing the binary you're
working on which is a much more obvious failure.

The assumption "one example program, one C file" would be too heavy
handed in most contexts but for examples that's basically a guiding
principle, so I don't think it'll be a problem in practice, and we
can always revisit it later on if needs be.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] examples: Fix installation on Windows

Posted by Daniel P. Berrangé 10 weeks ago
On Tue, May 14, 2019 at 09:21:12AM +0200, Andrea Bolognani wrote:
> On Mon, 2019-05-13 at 16:14 +0100, Daniel P. Berrangé wrote:
> > On Mon, May 13, 2019 at 02:55:14PM +0200, Andrea Bolognani wrote:
> > > +# List of example programs. We need to list them here instead of using
> > > +# $(noinst_PROGRAMS) directly because we want to have access to the
> > > +# unmodified list during (un)installation, but at the same time automake
> > > +# might tweak $(noinst_PROGRAMS) to eg. automatically add the .exe file
> > > +# extension when targeting Windows.
> > > +EXAMPLES = \
> > > +	admin/client_close \
> > > +	admin/client_info \
> > > +	admin/client_limits \
> > > +	admin/list_clients \
> > > +	admin/list_servers \
> > > +	admin/logging \
> > > +	admin/threadpool_params \
> > > +	dominfo/info1 \
> > > +	dommigrate/dommigrate \
> > > +	domsuspend/suspend \
> > > +	domtop/domtop \
> > > +	hellolibvirt/hellolibvirt \
> > > +	object-events/event-test \
> > > +	openauth/openauth \
> > > +	rename/rename \
> > > +	$(NULL)
> > > +
> > > +noinst_PROGRAMS = \
> > > +	$(EXAMPLES) \
> > > +	$(NULL)
> > 
> > I wonder if we should just directly use the _SOURCES vars instead of making
> > the assumption that binary names match source files. eg
> > 
> >   EXAMPLE_SRCS = \
> >       $(dominfo_info1_SOURCES) \
> >       $(dommigrate_dommigrate_SOURCES) \
> >       .....
> 
> That would (probably, I haven't actually tested it) work, however it
> seems to me like it would be much more likely that such a solution
> would eventually fall out of sync than the one I'm proposing, since
> when adding a new example you'd only notice the missing file if you
> actively performed an installation and checked the contents of the
> documentation directory, rather than not seeing the binary you're
> working on which is a much more obvious failure.

I don't think such a bug is any more likely here than anywhere else
in our makefiles & we cope fine in general. We need a list of source
files & we have variables defining the source files, so it just feels
wrong to instead use a list of binary names and infer the source files
from that.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] examples: Fix installation on Windows

Posted by Andrea Bolognani 10 weeks ago
On Tue, 2019-05-14 at 10:16 +0100, Daniel P. Berrangé wrote:
> On Tue, May 14, 2019 at 09:21:12AM +0200, Andrea Bolognani wrote:
> > On Mon, 2019-05-13 at 16:14 +0100, Daniel P. Berrangé wrote:
> > > I wonder if we should just directly use the _SOURCES vars instead of making
> > > the assumption that binary names match source files. eg
> > > 
> > >   EXAMPLE_SRCS = \
> > >       $(dominfo_info1_SOURCES) \
> > >       $(dommigrate_dommigrate_SOURCES) \
> > >       .....
> > 
> > That would (probably, I haven't actually tested it) work, however it
> > seems to me like it would be much more likely that such a solution
> > would eventually fall out of sync than the one I'm proposing, since
> > when adding a new example you'd only notice the missing file if you
> > actively performed an installation and checked the contents of the
> > documentation directory, rather than not seeing the binary you're
> > working on which is a much more obvious failure.
> 
> I don't think such a bug is any more likely here than anywhere else
> in our makefiles & we cope fine in general. We need a list of source
> files & we have variables defining the source files, so it just feels
> wrong to instead use a list of binary names and infer the source files
> from that.

As I tried to explain above, the difference is that if you're adding
a new example and you forget to update $(noinst_PROGRAMS) or add the
corresponding $(whatever_SOURCES), you'll immediately figure out that
something is wrong because your example program will either fail to
compile or just not show up.

On the other hand, if you fail to update $(EXAMPLE_SRCS), nothing
obviously wrong will happen, your example program will compile and
run just fine, but the source code for it will not be installed as
documentation.

Given that we have failed to ship and install Devhelp-compatible
documentation correctly for literally *years* (since 2014 AFAICT)
without anyone noticing, I have little confidence we won't break it
again in the future, and when that happens I'd prefer if it didn't
just stay silently broken for ages.

If your approach is the only one you consider acceptable, though,
I'll cave in and adopt it: better broken, possibly, in the future
than broken, for sure, right now :)

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] examples: Fix installation on Windows

Posted by Andrea Bolognani 9 weeks ago
On Tue, 2019-05-14 at 11:59 +0200, Andrea Bolognani wrote:
> On Tue, 2019-05-14 at 10:16 +0100, Daniel P. Berrangé wrote:
> > On Tue, May 14, 2019 at 09:21:12AM +0200, Andrea Bolognani wrote:
> > > On Mon, 2019-05-13 at 16:14 +0100, Daniel P. Berrangé wrote:
> > > > I wonder if we should just directly use the _SOURCES vars instead of making
> > > > the assumption that binary names match source files. eg
> > > > 
> > > >   EXAMPLE_SRCS = \
> > > >       $(dominfo_info1_SOURCES) \
> > > >       $(dommigrate_dommigrate_SOURCES) \
> > > >       .....
> > > 
> > > That would (probably, I haven't actually tested it) work, however it
> > > seems to me like it would be much more likely that such a solution
> > > would eventually fall out of sync than the one I'm proposing, since
> > > when adding a new example you'd only notice the missing file if you
> > > actively performed an installation and checked the contents of the
> > > documentation directory, rather than not seeing the binary you're
> > > working on which is a much more obvious failure.
> > 
> > I don't think such a bug is any more likely here than anywhere else
> > in our makefiles & we cope fine in general. We need a list of source
> > files & we have variables defining the source files, so it just feels
> > wrong to instead use a list of binary names and infer the source files
> > from that.
> 
> As I tried to explain above, the difference is that if you're adding
> a new example and you forget to update $(noinst_PROGRAMS) or add the
> corresponding $(whatever_SOURCES), you'll immediately figure out that
> something is wrong because your example program will either fail to
> compile or just not show up.
> 
> On the other hand, if you fail to update $(EXAMPLE_SRCS), nothing
> obviously wrong will happen, your example program will compile and
> run just fine, but the source code for it will not be installed as
> documentation.
> 
> Given that we have failed to ship and install Devhelp-compatible
> documentation correctly for literally *years* (since 2014 AFAICT)
> without anyone noticing, I have little confidence we won't break it
> again in the future, and when that happens I'd prefer if it didn't
> just stay silently broken for ages.
> 
> If your approach is the only one you consider acceptable, though,
> I'll cave in and adopt it: better broken, possibly, in the future
> than broken, for sure, right now :)

So how should I proceed? Either way, I'd like to get the CI back
to green sooner rather than later.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list