[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