| Message ID | 20190204142638.27021-16-mst@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [PULL,01/25] virtio: add checks for the size of the indirect table | expand |
No explanation of /why/ we want to build this by default ? The source header calls it a demo application and it has no man page. Given this IMHO we should *not* be building & installing it by default, as doing so defacto turns it into a user tool we have to support. On Mon, Feb 04, 2019 at 09:43:48AM -0500, Michael S. Tsirkin wrote: > From: Changpeng Liu <changpeng.liu@intel.com> > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > configure | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/configure b/configure > index 31cf6f584d..5c619d4e03 100755 > --- a/configure > +++ b/configure > @@ -5831,6 +5831,9 @@ if test "$want_tools" = "yes" ; then > if [ "$posix" = "yes" ] && [ "$curl" = "yes" ]; then > tools="elf2dmp $tools" > fi > + if [ "$linux" = "yes" ]; then > + tools="vhost-user-blk\$(EXESUF) $tools" > + fi > fi > if test "$softmmu" = yes ; then > if test "$linux" = yes; then > -- > MST > Regards, Daniel
Hmm I do think we want to build the contrib tools, otherwise they bitrot too quickly, witness follow-up patches that fix the compilation. And I think we need tests that actually use them. However I agree adding them to tools and installing is probably rushing things, e.g. there's no manpage even. Changpeng Liu could you post a patch that moves this away from tools, so it builds but isn't installed? If it's tricky I think I will revert this one for now .. On Mon, Feb 04, 2019 at 03:07:48PM +0000, Daniel P. Berrangé wrote: > > No explanation of /why/ we want to build this by default ? > > The source header calls it a demo application and it has no man > page. > > Given this IMHO we should *not* be building & installing it by > default, as doing so defacto turns it into a user tool we have > to support. > > > On Mon, Feb 04, 2019 at 09:43:48AM -0500, Michael S. Tsirkin wrote: > > From: Changpeng Liu <changpeng.liu@intel.com> > > > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com> > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > configure | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/configure b/configure > > index 31cf6f584d..5c619d4e03 100755 > > --- a/configure > > +++ b/configure > > @@ -5831,6 +5831,9 @@ if test "$want_tools" = "yes" ; then > > if [ "$posix" = "yes" ] && [ "$curl" = "yes" ]; then > > tools="elf2dmp $tools" > > fi > > + if [ "$linux" = "yes" ]; then > > + tools="vhost-user-blk\$(EXESUF) $tools" > > + fi > > fi > > if test "$softmmu" = yes ; then > > if test "$linux" = yes; then > > -- > > MST > > > > 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 :|
On Mon, Feb 04, 2019 at 10:19:42AM -0500, Michael S. Tsirkin wrote: > Hmm I do think we want to build the contrib tools, > otherwise they bitrot too quickly, witness follow-up > patches that fix the compilation. > > And I think we need tests that actually use them. > > However I agree adding them to tools and installing > is probably rushing things, e.g. there's no > manpage even. Yes, if we build, but do not install, the binary that would be ok Regards, Daniel
On Mon, Feb 04, 2019 at 03:29:21PM +0000, Daniel P. Berrangé wrote: > On Mon, Feb 04, 2019 at 10:19:42AM -0500, Michael S. Tsirkin wrote: > > Hmm I do think we want to build the contrib tools, > > otherwise they bitrot too quickly, witness follow-up > > patches that fix the compilation. > > > > And I think we need tests that actually use them. > > > > However I agree adding them to tools and installing > > is probably rushing things, e.g. there's no > > manpage even. > > Yes, if we build, but do not install, the binary that would be ok > > > Regards, > Daniel OK I reverted this for now. > -- > |: 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 :|
On Mon, Feb 04, 2019 at 08:48:14PM -0500, Michael S. Tsirkin wrote: > On Mon, Feb 04, 2019 at 03:29:21PM +0000, Daniel P. Berrangé wrote: > > On Mon, Feb 04, 2019 at 10:19:42AM -0500, Michael S. Tsirkin wrote: > > > Hmm I do think we want to build the contrib tools, > > > otherwise they bitrot too quickly, witness follow-up > > > patches that fix the compilation. > > > > > > And I think we need tests that actually use them. > > > > > > However I agree adding them to tools and installing > > > is probably rushing things, e.g. there's no > > > manpage even. > > > > Yes, if we build, but do not install, the binary that would be ok > > > > > > Regards, > > Daniel > > OK I reverted this for now. Yes, I requested this patch to avoid further bitrot. Installing isn't necessary though. Stefan
diff --git a/configure b/configure index 31cf6f584d..5c619d4e03 100755 --- a/configure +++ b/configure @@ -5831,6 +5831,9 @@ if test "$want_tools" = "yes" ; then if [ "$posix" = "yes" ] && [ "$curl" = "yes" ]; then tools="elf2dmp $tools" fi + if [ "$linux" = "yes" ]; then + tools="vhost-user-blk\$(EXESUF) $tools" + fi fi if test "$softmmu" = yes ; then if test "$linux" = yes; then