Created attachment 270720 [details] [review]
OvirtBroker isnt used if it is not available

If HAVE_OVIRT is not defined ovirt-broker.vala is not included for
compilation so the condition has to be a preprocessor directive.

Review of attachment 270720 [details] [review]:

::: src/app.vala
@@ +140,2 @@
         brokers.insert ("libvirt", LibvirtBroker.get_default ());
+#if HAVE_OVIRT

I'd like Christophe to review this but there was a reason we didn't use '#if' on vala level. IIRC the reason was that we distribute generated C files in that tarball so people (typically distribute packagers) don't need vala compiler to build Boxes from tarball. This means that if person creating the tarball doesn't have ovirt, the tarball will distribute Boxes without ovirt support compiled in C files.

9(In reply to comment #2)
> Review of attachment 270720 [details] [review]:
> 
> ::: src/app.vala
> @@ +140,2 @@
>          brokers.insert ("libvirt", LibvirtBroker.get_default ());
> +#if HAVE_OVIRT
> 
> I'd like Christophe to review this but there was a reason we didn't use '#if'
> on vala level. IIRC the reason was that we distribute generated C files in that
> tarball so people (typically distribute packagers) don't need vala compiler to
> build Boxes from tarball. This means that if person creating the tarball
> doesn't have ovirt, the tarball will distribute Boxes without ovirt support
> compiled in C files.

Knowing this and with my limited vala capabilities I see the best possibility would be to create a c header and include it on vala level in a vapi file. We can then move the directive to that C file.

Do you agree?

(In reply to comment #3)
> 9(In reply to comment #2)
> > Review of attachment 270720 [details] [review] [details]:
> > 
> > ::: src/app.vala
> > @@ +140,2 @@
> >          brokers.insert ("libvirt", LibvirtBroker.get_default ());
> > +#if HAVE_OVIRT
> > 
> > I'd like Christophe to review this but there was a reason we didn't use '#if'
> > on vala level. IIRC the reason was that we distribute generated C files in that
> > tarball so people (typically distribute packagers) don't need vala compiler to
> > build Boxes from tarball. This means that if person creating the tarball
> > doesn't have ovirt, the tarball will distribute Boxes without ovirt support
> > compiled in C files.
> 
> Knowing this and with my limited vala capabilities I see the best possibility
> would be to create a c header and include it on vala level in a vapi file.

That is the case already. Perhaps its not working as it should somehow.

(In reply to comment #4)
> (In reply to comment #3)
> > 9(In reply to comment #2)
...
> > Knowing this and with my limited vala capabilities I see the best possibility
> > would be to create a c header and include it on vala level in a vapi file.
> 
> That is the case already. Perhaps its not working as it should somehow.

As I look at the file,
brokers.insert ("ovirt", OvirtBroker.get_default ());
will be parsed by valac independendly of the Config.HAVE_OVIRT. It will just not be evaluated at runtime but thats not enought if the ovirt-broker.vala is not included.

(In reply to comment #2)
> Review of attachment 270720 [details] [review]:
> IIRC the reason was that we distribute generated C files in that
> tarball so people (typically distribute packagers) don't need vala compiler to
> build Boxes from tarball. This means that if person creating the tarball
> doesn't have ovirt, the tarball will distribute Boxes without ovirt support
> compiled in C files.

Yup

(In reply to comment #4)
> That is the case already. Perhaps its not working as it should somehow.

Currently, it's relying on gcc to not attempt to compile code in a if (0) block. This works with -O2 and -O0 with my f20 gcc, with RHEL6 gcc and worked with the compiler that was in latest stable fedora when I added ovirt support. Maybe this is what is causing issue on your system? A quick test-case should be:

int main(int argc, char **argv)
{

    if (0) {
        stub_function_totototot();
    }

    return 0;
}

(In reply to comment #6)
> (In reply to comment #2)
> > Review of attachment 270720 [details] [review] [details]:
> > IIRC the reason was that we distribute generated C files in that
> > tarball so people (typically distribute packagers) don't need vala compiler to
> > build Boxes from tarball. This means that if person creating the tarball
> > doesn't have ovirt, the tarball will distribute Boxes without ovirt support
> > compiled in C files.
> 
> Yup
> 
> (In reply to comment #4)
> > That is the case already. Perhaps its not working as it should somehow.
> 
> Currently, it's relying on gcc to not attempt to compile code in a if (0)
> block. This works with -O2 and -O0 with my f20 gcc, with RHEL6 gcc and worked
> with the compiler that was in latest stable fedora when I added ovirt support.
> Maybe this is what is causing issue on your system? A quick test-case should
> be:
> 
> int main(int argc, char **argv)
> {
> 
>     if (0) {
>         stub_function_totototot();
>     }
> 
>     return 0;
> }

lasse@lssteady ~/prog/gnome/exp % vim tst.c
lasse@lssteady ~/prog/gnome/exp % gcc -O0 tst.c 
lasse@lssteady ~/prog/gnome/exp % gcc -O2 tst.c 
lasse@lssteady ~/prog/gnome/exp % 

I just pasted your code to tst.c and it compiled fine.

I personally don't like code that relies on certain optimizations to function properly. At least some users have this issue. (gnome-boxes is known in the ArchLinux wiki not to compile. At least not using jhbuild [1]. Also see [2] if you want more info about the message and another case.)

I can probably post the patch there but is this really a good way of coding if it needs a patch for some persons to compile...?

[1] https://wiki.archlinux.org/index.php/JHBuild#Other_broken_modules
[2] https://mail.gnome.org/archives/gnome-boxes-list/2013-April/msg00018.html

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #2)
> > > Review of attachment 270720 [details] [review] [details] [details]:
> > > IIRC the reason was that we distribute generated C files in that
> > > tarball so people (typically distribute packagers) don't need vala compiler to
> > > build Boxes from tarball. This means that if person creating the tarball
> > > doesn't have ovirt, the tarball will distribute Boxes without ovirt support
> > > compiled in C files.
> > 
> > Yup
> > 
> > (In reply to comment #4)
> > > That is the case already. Perhaps its not working as it should somehow.
> > 
> > Currently, it's relying on gcc to not attempt to compile code in a if (0)
> > block. This works with -O2 and -O0 with my f20 gcc, with RHEL6 gcc and worked
> > with the compiler that was in latest stable fedora when I added ovirt support.
> > Maybe this is what is causing issue on your system? A quick test-case should
> > be:
> > 
> > int main(int argc, char **argv)
> > {
> > 
> >     if (0) {
> >         stub_function_totototot();
> >     }
> > 
> >     return 0;
> > }
> 
> lasse@lssteady ~/prog/gnome/exp % vim tst.c
> lasse@lssteady ~/prog/gnome/exp % gcc -O0 tst.c 
> lasse@lssteady ~/prog/gnome/exp % gcc -O2 tst.c 
> lasse@lssteady ~/prog/gnome/exp % 
> 
> I just pasted your code to tst.c and it compiled fine.
> 
> I personally don't like code that relies on certain optimizations to function
> properly. At least some users have this issue. (gnome-boxes is known in the
> ArchLinux wiki not to compile. At least not using jhbuild [1].

Really nice that they never bothered to tell us about this. :(

> Also see [2] if
> you want more info about the message and another case.)
 
Since those guys never really followed up on this issue, it got completely forgotten. :(

> I can probably post the patch there but is this really a good way of coding if
> it needs a patch for some persons to compile...?

Yeah, you shouldn't need a downstream hack/patch to be able to build Boxes, unless the distro is doing something weird (which it doesn't seem to).

(In reply to comment #6)
> (In reply to comment #2)
> > Review of attachment 270720 [details] [review] [details]:
> > IIRC the reason was that we distribute generated C files in that
> > tarball so people (typically distribute packagers) don't need vala compiler to
> > build Boxes from tarball. This means that if person creating the tarball
> > doesn't have ovirt, the tarball will distribute Boxes without ovirt support
> > compiled in C files.
> 
> Yup
> 
> (In reply to comment #4)
> > That is the case already. Perhaps its not working as it should somehow.
> 
> Currently, it's relying on gcc to not attempt to compile code in a if (0)
> block. This works with -O2 and -O0

Ouch. Thats sounds really hackish. How about we enclose all govirt-relying code in a specific module(s) and have the same API in dummy module(s) and then choose which one to compile against in Makefile.am depending on if ovirt is enabled or not?

(In reply to comment #8)
> > Also see [2] if
> > you want more info about the message and another case.)
> 
> Since those guys never really followed up on this issue, it got completely
> forgotten. :(

Yup, fairly sure I've seen people with this build issue in the past, but I did not manage to reproduce when I tried, so I dropped the ball on it.

(In reply to comment #9)
> Ouch. Thats sounds really hackish. 

Well, this is not intentionally hackish as I only found out about this yesterday :) When writing this conditional code, I was happy with everything building with the right code generated, never realized this was relying on this if (0) thing. 

> How about we enclose all govirt-relying code
> in a specific module(s) and have the same API in dummy module(s) and then
> choose which one to compile against in Makefile.am depending on if ovirt is
> enabled or not?

Yup.

Created attachment 273191 [details] [review]
Adds empty ovirt dummy to allow ovirt-free compilation

It took my some time to learn automake besides my usual activities, but finally: here's the patch.

I tried it out, it compiles from vala with and without ovirt and the distributed thing also compiles without ovirt if the distributor compiled with ovirt.

The one thing I didn't figure out: it would be good to have at least a warning if one tries to distribute this without having ovirt installed because then no user can have ovirt. If you can name me any references about related automake commands (I didn't find them on my initial research) I will add this too.

(In reply to comment #11)
> Created an attachment (id=273191) [details] [review]
> Adds empty ovirt dummy to allow ovirt-free compilation
> 
> It took my some time to learn automake besides my usual activities, but
> finally: here's the patch.
> 
> I tried it out, it compiles from vala with and without ovirt and the
> distributed thing also compiles without ovirt if the distributor compiled with
> ovirt.
> 
> The one thing I didn't figure out: it would be good to have at least a warning
> if one tries to distribute this without having ovirt installed because then no
> user can have ovirt. If you can name me any references about related automake
> commands (I didn't find them on my initial research) I will add this too.

I can find that out for you later (its pretty simple actually and doesn't exactly involve any auomake commands) and also review rest of the patch. However, I seriously doubt packagers give much attention to warnings from configure so I'd recommend we do this how Ryan Lortie suggests one should handle optional deps: Require the dep by default but provide any option to disable it. This is what will definitely get packagers attention and they can then decide what path to take.

Created attachment 273393 [details] [review]
Add empty ovirt

If you now compile, the ovirt-broker.vapi will be distributed among
the other files allowing users to compile it from C level without
ovirt.

Directly compiling from vala sources is also supported through this
changeset.

(In reply to comment #13)
> Created an attachment (id=273393) [details] [review]
> Add empty ovirt
> 
> If you now compile, the ovirt-broker.vapi will be distributed among
> the other files allowing users to compile it from C level without
> ovirt.
> 
> Directly compiling from vala sources is also supported through this
> changeset.

I think I uploaded a wrong patch. Testing this is a bit chaotic with several PCs (one having ovirt and one not having it.)

Created attachment 273824 [details] [review]
Add empty ovirt

If you now compile, the ovirt-broker.vapi will be distributed among
the other files allowing users to compile it from C level without
ovirt.

Directly compiling from vala sources is also supported through this
changeset.

Review of attachment 273824 [details] [review]:

Whenever compiling without ovirt one has to append --enable-ovirt=no after this patch and everything will work.

I tested the following cases with this patch:

* Compilation with ovirt installed (works)
* Package generation with ovirt installed (works)
   * Compilation of this generated package with ovirt installed (works)
   * Compilation of this generated package without ovirt installed and --enable-ovirt=no (works)
   * Compilation of this generated package without ovirt installed without additional flags (does not work: message: ovirt required)
* Compilation without ovirt installed and --enable-ovirt=no (works)

I'll have to do some retesting with all the different OSes and machines at home. Dont use the given patch (even though I think it is at least near to a working solution)

Created attachment 275135 [details] [review]
Add empty ovirt

This patch allows the following scenarios:
- Configure and compile Boxes with ovirt normally
- Configure and compile Boxes without ovirt with --enable-ovirt=no
- Distribute Boxes with ovirt (make distcheck)
- Configure and compile the distributed Boxes with ovirt
- Configure and compile the distributed Boxes without ovirt with --enable-ovirt=no

The following scenarios don't work and shouldn't work:
- Configure Boxes without ovirt normally (implicit --enable-ovirt=yes)
- Configure distributed Boxes without ovirt normally (implicit --enable-ovirt=yes)

The following scenarios don't work:
- Distribute Boxes without ovirt

All these scenarios were tested with a newly cleanly cloned environment so that older builds have no effect on this. (I think this was the case in my tests before since I got inconsistent results back then.)

As I see it now the only possibly non-intendet behaviour would be distributing Boxes without ovirt.

When I try distributing without ovirt, autotools generates the target ovirt-broker.vapi anyway. (HAVE_OVIRT should be false.) While I am not an expert in autotools (this is actually my first contact) so I hope I can maybe get some help or hints for this problem.

(In reply to comment #18)
> Created an attachment (id=275135) [details] [review]
> Add empty ovirt
> 
> This patch allows the following scenarios:
> - Configure and compile Boxes with ovirt normally
> - Configure and compile Boxes without ovirt with --enable-ovirt=no
> - Distribute Boxes with ovirt (make distcheck)
> - Configure and compile the distributed Boxes with ovirt
> - Configure and compile the distributed Boxes without ovirt with
> --enable-ovirt=no
> 
> The following scenarios don't work and shouldn't work:
> - Configure Boxes without ovirt normally (implicit --enable-ovirt=yes)

I don't get this. Are you talking about running configure on dist tarball? If so, please make it clear in all these scenerios whether you are talking about tarball or git checkout. Right now it all sounds very mixed and therefore confusing.

> - Configure distributed Boxes without ovirt normally (implicit
> --enable-ovirt=yes)
> 
> The following scenarios don't work:
> - Distribute Boxes without ovirt
> 
> All these scenarios were tested with a newly cleanly cloned environment so that
> older builds have no effect on this. (I think this was the case in my tests
> before since I got inconsistent results back then.)
> 
> As I see it now the only possibly non-intendet behaviour would be distributing
> Boxes without ovirt.
> 
> When I try distributing without ovirt, autotools generates the target
> ovirt-broker.vapi anyway. (HAVE_OVIRT should be false.) While I am not an
> expert in autotools (this is actually my first contact) so I hope I can maybe
> get some help or hints for this problem.

Sure, I'll have a look but keep in mind nobody is an autotools expert, really. :)

What works:

Git:
 - Configure and compile Boxes from git with ovirt normally
 - Configure and compile Boxes from git without ovirt with --enable-ovirt=no

 - Distribute Boxes with ovirt (make distcheck)

Tarball:
 - Configure and compile Boxes tarball with ovirt
 - Configure and compile Boxes tarball without ovirt with --enable-ovirt=no

Is this clearer? (With distributed boxes i meant the tarball in the original post)

Review of attachment 275135 [details] [review]:

Oh, the info I commented on previous seems not to be part of git log. I think the non-working schenerious you mention in your comment should also be mentioned in the commit log.

::: configure.ac
@@ +104,3 @@
               AS_HELP_STRING([--enable-ovirt=yes|no|auto], [Enable support for OVirt]),
               [enable_ovirt=$enableval],
+              [enable_ovirt=yes])

We shouldn't need to do this.

::: src/ovirt-broker-empty.vala
@@ +5,3 @@
+ * but the user can compile without it.
+ */
+#if HAVE_OVIRT

If you want to use the #if in source file, you wouldn't need a new dummy file. The gust of this solution was for Makefile.am to decide which C files to use depending on if ovirt support is enabled or not.

"well in that case you won't need a seperate C file, you can decide to use the appropriate (generated) C file in Makefile itself
you can put ovirt stuff in a separate static library
and list the ovirt vala files as sources for that naturally but get Makefile to compile them only to C level
actually you don't want to list them as sources
for the library
you want to list the generated C files as sources of the library
and have an intermediate target for building the vala files to C files
then add that target as dependency of the static library source(s)

<flo> zeenix: I moved the optional part out in a independend static library and only statically link it if necessary. Worked well enough.
seems its a tested solution :)"

If you want to see example of how this is done, you should ask flo on #vala IRC channel for links to the project he is doing it in.

(In reply to comment #20)
> What works:
> 
> Git:
>  - Configure and compile Boxes from git with ovirt normally
>  - Configure and compile Boxes from git without ovirt with --enable-ovirt=no
> 
>  - Distribute Boxes with ovirt (make distcheck)
> 
> Tarball:
>  - Configure and compile Boxes tarball with ovirt
>  - Configure and compile Boxes tarball without ovirt with --enable-ovirt=no
> 
> Is this clearer?

Much :) If I understood correctly, with your current patch, tarball must be created with '--enable-ovirt=yes'? That is something we'd want to avoid if possible.

> (With distributed boxes i meant the tarball in the original
> post)

Yeah, I understood where you made it explicit.

(In reply to comment #22)
> (In reply to comment #20)
> Much :) If I understood correctly, with your current patch, tarball must be
> created with '--enable-ovirt=yes'? That is something we'd want to avoid if
> possible.

I thought we wanted exactly this to force the distributor to think about ovirt if he doesn't have it. (Although it doesn't make sense to do this if distributing without ovirt is not possible - but if I understood you right we wanted this to be possible with explicitly stating that ovirt should not be included.)

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #20)
> > Much :) If I understood correctly, with your current patch, tarball must be
> > created with '--enable-ovirt=yes'? That is something we'd want to avoid if
> > possible.
> 
> I thought we wanted exactly this to force the distributor to think about ovirt
> if he doesn't have it.

distributor doesn't create tarballs, they consume it. What we wanted was for '--enable-ovirt=yes' to be default so that ovirt support has to be always disabled explicitly and it doesn't happen by accident.

I cant recreate this issue anymore.

I'm marking this as enhancement since this is merely about this now:

> > Currently, it's relying on gcc to not attempt to compile code in a if (0)
> > block. This works with -O2 and -O0
>
> Ouch. Thats sounds really hackish.

(In reply to comment #25)
> I cant recreate this issue anymore.

If you can't reproduce with optimizations options other than -O2 and -O0, then there is no hack involved and there is nothing to fix.

I tried to reproduce it and failed. THerefore closing this bug like you suggested.