[virt-tools-list] [PATCH virt-viewer 2/2] Move tests under /tests directory
Pavel Grunt
pgrunt at redhat.com
Fri Mar 11 16:30:03 UTC 2016
On Fri, 2016-03-11 at 12:05 -0300, Eduardo Lima (Etrunko) wrote:
> On 03/11/2016 11:44 AM, Fabiano Fidêncio wrote:
> [snip]
> >
> > >
> > > diff --git a/tests/Makefile.am b/tests/Makefile.am
> > > new file mode 100644
> > > index 0000000..470733b
> > > --- /dev/null
> > > +++ b/tests/Makefile.am
> > > @@ -0,0 +1,30 @@
> > > +NULL =
> > > +
> > > +AM_CPPFLAGS
> > > =
> > > \
> > > + -DLOCALE_DIR=\""$(datadir)/locale"\" \
> > > + -
> > > I$(top_srcdir)/src/ \
> > > + -
> > > I$(top_srcdir)/tests/ \
> > > + $(GLIB2_CFLAGS)
> > > \
> > > + $(GTK_CFLAGS)
> > > \
> > > + $(LIBXML2_CFLAGS)
> > > \
> > > + $(WARN_CFLAGS)
> > > \
> > > + $(NULL)
> > Pavel, I know you basically copied & pasted the old code here. But
> > do
> > we need {GTK,LIBXML2}_CFLAGS here? Our tests are not using none of
> > those if I'm not mistaken.
> >
> > I would accept a first patch removing these unneeded CPPFLAGS/LIBS.
> >
> > Also, can we have the "\" aligned?
> >
> > >
> > > +
> > > +LDADD=
> > > \
> > > + $(top_builddir)/src/libvirt-viewer-util.la \
> > > + $(GLIB2_LIBS)
> > > \
> > > + $(GTK_LIBS)
> > > \
> > > + $(LIBXML2_LIBS)
> > > \
> > > + $(NULL)
> > > +
> > Same here.
> >
> The problem with having backslash symbols aligned is that each one's
> preferred editor might have different setups. Makefiles use to
> require
> tab indentation (not sure if still a hard requirement), so different
> tabstops may result in different alignment.
>
> My suggestion is to just use a single space after the file/variable
> and
> then the backslash symbol, while the begin of each line is still tab
> indented. For instance:
>
> +LDADD= \
> + $(top_builddir)/src/libvirt-viewer-util.la \
> + $(GLIB2_LIBS) \
> + $(GTK_LIBS) \
> + $(LIBXML2_LIBS) \
> + $(NULL)
>
right, my bad, I though we are using tab=4spaces
I would go for your suggestion about space+'\'
Thanks for the review guys,
Pavel
More information about the virt-tools-list
mailing list