GNOME Bugzilla – Bug 582439
[PATCH] Fix build on maemo platform
Last modified: 2011-01-16 23:37:51 UTC
On maemo, exceptions and properties are disabled. Attached patch adds some #ifdefs to fix the build.
Created attachment 134549 [details] [review] Fix build when exceptions and properties are disabled There are some other issues on maemo platform because Nokia does not build all the plugins (ogg, theora, text, etc. are missing) and as such I had to edit configure.am and Makefile_am_fragment to be able to build gstreamermm. This is not part of the patch but it would probably be good if the build system would autodetect the available plugins somehow.
I believe that those dependencies only need to be there when generating the files, in maintainer-mode, when using autogen.sh. It should be able to build from a regular tarball without those dependencies, so they don't need to be detected. People will just get runtime errors when trying to instantiate those classes on Maemo. I am surprised that your patch only patches examples. Surely some of the actual code uses "throw"? Have you really disabled exceptions with a g++ flag?
(In reply to comment #1) > There are some other issues on maemo platform because Nokia does not build all > the plugins (ogg, theora, text, etc. are missing) and as such I had to edit > configure.am and Makefile_am_fragment to be able to build gstreamermm. This is > not part of the patch but it would probably be good if the build system would > autodetect the available plugins somehow. (In reply to comment #2) > I believe that those dependencies only need to be there when generating the > files, in maintainer-mode, when using autogen.sh. It should be able to build > from a regular tarball without those dependencies, so they don't need to be > detected. People will just get runtime errors when trying to instantiate those > classes on Maemo. I guess the examples can be modified so that the plug-ins that don't exist on Maemo are not used. That should fix the build. I wonder which plug-ins can be used as an example "audio" player. Right now the ogg plug-ins are used. Ultimately, we could remove the audio player examples if necessary. > I am surprised that your patch only patches examples. Surely some of the actual > code uses "throw"? Yes, there is other code that throws Glib::Exception. I think that #ifdefs have already been included in those cases. Please correct me if I'm wrong.
(In reply to comment #3) > I guess the examples can be modified so that the plug-ins that don't exist on > Maemo are not used. That should fix the build. Again, the plugins are a _runtime_ dependency. So there's no need to use different plugins in the example just to fix the build. > > I am surprised that your patch only patches examples. Surely some of the actual > > code uses "throw"? > > Yes, there is other code that throws Glib::Exception. I think that #ifdefs > have already been included in those cases. Please correct me if I'm wrong. Are you using -fnoexceptions?
(In reply to comment #4) > (In reply to comment #3) > > I guess the examples can be modified so that the plug-ins that don't exist on > > Maemo are not used. That should fix the build. > > Again, the plugins are a _runtime_ dependency. So there's no need to use > different plugins in the example just to fix the build. Ah, that's right. The source files for the plug-ins are included in the tarball so building should be no problem. > > > I am surprised that your patch only patches examples. Surely some of the actual > > > code uses "throw"? > > > > Yes, there is other code that throws Glib::Exception. I think that #ifdefs > > have already been included in those cases. Please correct me if I'm wrong. > > Are you using -fnoexceptions? I don't know so I'm probably wrong.
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > Again, the plugins are a _runtime_ dependency. So there's no need to use > > different plugins in the example just to fix the build. > > Ah, that's right. The source files for the plug-ins are included in the > tarball so building should be no problem. One thing I forgot, though, is that plug-ins that don't exist on the build machine are not included in the build, so errors might come up in examples that use a plug-in that does not exist on the build machine. This was done in part because if a plug-in does not exist and it is included in the build, no program would execute because when wrap_init() runs, it would fail when it tries to initialize the missing plug-in (see the long-winded bug #565454 : http://bugzilla.gnome.org/show_bug.cgi?id=565454). > > > Yes, there is other code that throws Glib::Exception. I think that #ifdefs > > > have already been included in those cases. Please correct me if I'm wrong. > > > > Are you using -fnoexceptions? > > I don't know so I'm probably wrong. Clarification: If this is a maemo option, I haven't had the time to try building there yet, though I'll try later to get a sense of how building works there and see if any errors can be fixed.
> Are you using -fnoexceptions? Not yet, I think. I just made sure it builds when the other libraries are build without exceptions but I will look into building gstreamermm without exceptions. I think gmmproc already does generate correct code that can work without exceptions but there may be some hand-coded methods that need work.
> > Are you using -fnoexceptions? > > I don't know so I'm probably wrong. Sorry, I thought I was talking to Johannes. Johannes, this was a rather obvious requirement.
> One thing I forgot, though, is that plug-ins that don't exist on the build > machine are not included in the build, so errors might come up in examples that > use a plug-in that does not exist on the build machine. > > This was done in part because if a plug-in does not exist and it is included in > the build, no program would execute because when wrap_init() runs, it would > fail when it tries to initialize the missing plug-in (see the long-winded bug > #565454 : http://bugzilla.gnome.org/show_bug.cgi?id=565454). That's unfortunate. That means that the standard distributed package of gstreamermm on, for instance, Ubuntu, will fail at runtime even if I'm not using one of these extra plug-ins, if the C plugins are not available. That will hit most people. Obviously we need a way to make wrap_init() fail more gracefully.
(In reply to comment #8) > Sorry, I thought I was talking to Johannes. Johannes, this was a rather obvious > requirement. Sure, but I cannot do the second step before the first. As I said, I am working on it.
(In reply to comment #9) > Obviously we need a way to make wrap_init() fail more gracefully. This is the crash that I guess you experienced:
+ Trace 215466
I think we just need a null check in Glib::wrap_register(). I'll try that tomorrow. I have already removed the ifdefs from my checkout of gstreamermm.
> I think we just need a null check in Glib::wrap_register(). I'll try that > tomorrow. I have already removed the ifdefs from my checkout of gstreamermm. Wait. The #ifdefs in wrap_init() should allow for the build to include plug-ins even if they don't exist. I was just testing and I have a preliminary patch that includes all plug-ins in the build (whether they exist on the build system or not) and the build is successful even if the plug-ins don't exist on the build system. Also, as you said, there will only be a _runtime_ error if the user attempts using a plug-in that is not on the running system but not a compilation problem. A patch follows. If it fixes things, it can be committed. I'd be glad to make a release soon after.
Created attachment 134667 [details] [review] Patch to include all plug-ins in build
(In reply to comment #12) > > I think we just need a null check in Glib::wrap_register(). I'll try that > > tomorrow. I have already removed the ifdefs from my checkout of gstreamermm. > > Wait. The #ifdefs in wrap_init() should allow for the build to include > plug-ins even if they don't exist. But what possible help are those ifdefs in the API? Why not just fix the crash and carry on? Well, I have just fixed this in glibmm. We'll need to commit that to the relevant glibmm-2.something branch for Maemo. The defines don't do anything extra - for instance they don't seem to prevent the use of any API that would stop gstreamermm from building. I have removed these ifdefs in gstreamermm - feel free to revert that if I have misunderstood. Now a gstreamermm-using C++ application can start using a plugin via C++ as soon as the C plugin has been installed (via the missing-plugin installer UI, for instance), without requiring a rebuild of gstreamermm. > I was just testing and I have a preliminary > patch that includes all plug-ins in the build (whether they exist on the build > system or not) and the build is successful even if the plug-ins don't exist on > the build system. That was necessary too. Thanks. I committed that for you. It might also now be possible to remove the forked generate_wrap_init.pl.in but maybe it does something else too.
You are right, of course. I was just trying re-post the following below while you were answering. If the forked generate_wrap_init.pl.in can be removed that's fine also: > That's unfortunate. That means that the standard distributed package of > gstreamermm on, for instance, Ubuntu, will fail at runtime even if I'm not > using one of these extra plug-ins, if the C plugins are not available. That > will hit most people. > > Obviously we need a way to make wrap_init() fail more gracefully. > I think we just need a null check in Glib::wrap_register(). I'll try that > tomorrow. I have already removed the ifdefs from my checkout of gstreamermm. Oh, you're talking about a distributed package (I missed it a little). In that case, now I see that the #ifdefs may not be valid for the running system so, yes then, removing them would be better. The patch I submitted would only be to make sure that building from a tarball is successful even if a plug-in that was wrapped and distributed in the tarball is not available on the build system.
Regarding the exceptions I have a question: Bin::add and Bin::remove throw std::exceptions. IMHO it would be better to just print the message ("Cannot add/remove null element) to stderr when using no exceptions because this is something usually handled by a g_return_if_fail() in gtk+. Doing things like bin->add(e1)->add(e2) will most likely crash anyway because the exceptions are never catched and this seems more a check for a programming error than a real use-case for exceptions.
I think that's OK for a first attempt. You could create a handle_error() method that had the ifdef only inside there, to keep it simple. Later we probably do need a version of add() and remove() that take an error output parameter. That means that the "chaining" syntax can't be used, but I don't see a way around that. > Doing things like > bin->add(e1)->add(e2) > will most likely crash anyway because the exceptions are never catched Well they should be caught. Please correct any example code that doesn't catch the exceptions.
Created attachment 134932 [details] [review] Patch to fix build on maemo With this patch gstreamermm builds with -Os -fno-exceptions besides the mentioned problem with missing plugins.
Sorry, the patch still misses some things. Don't know why it already build here. I will update it ASAP.
And for the nth, time, please stick to the existing code conventions. For instance, no spaces before brackets, as you have done here; + if ( I hope there are no tabs in the patch.
(In reply to comment #20) > And for the nth, time, please stick to the existing code conventions. For > instance, no spaces before brackets, as you have done here; > + if ( > > I hope there are no tabs in the patch. > http://git.gnome.org/cgit/gstreamermm/tree/examples/optiongroup/main.cc does not follow the no space convention. Should I use it anyway, fix the whole file or stick with the existing style?
> http://git.gnome.org/cgit/gstreamermm/tree/examples/optiongroup/main.cc does > not follow the no space convention. Should I use it anyway, fix the whole file > or stick with the existing style? It mostly does. Feel free to fix the few lines where it doesn't, in a separate commit, please.
Created attachment 135010 [details] [review] Updated patch OK, this took some more iteration that I expected... Anyway, with this patch I could successfully build gstreamermm on maemo (and could also build the .deb). There are some coding style inconsitances left: - Some of the code uses "if (" and "method(", some used "if(" and "method(". I sticked to what the rest of the file does for now. - I will add another patch that changes everything to "if(" "and method(".
+ try + { + pipeline->add(element_source)->add(element_filter)->add(element_sink); + } + catch (std::runtime_error& ex) + { I don't understand how that could build without exceptions. Also, exceptions should always be caught as _const_ references. Otherwise, please commit. Thanks.
Johannes, I corrected the coding styles I could find before you submitted a patch (I hope you don't mind). If you find any others, please feel free to correct those also. Thanks.
(In reply to comment #25) > Johannes, I corrected the coding styles I could find before you submitted a > patch (I hope you don't mind). If you find any others, please feel free to > correct those also. Thanks. Actually that was a quite bad idea because I had to merge my patch manually again because all the reference lines changed. It would have been easier to do it the other way round. Anyway, not your fault. Applied the patch now: http://git.gnome.org/cgit/gstreamermm/commit/?id=85f1a94efced063bd1c55c306f0284096a99ca2d @murrayc: I think try/catch blocks are just a no-op when exceptions are disabled and compilation only fails when using throw but I fixed it anyway.