At present, our master branch unconditionally builds in the object storage functionality (using the objectstore_dummy implementation). There's a few ways this isn't great:
* it has an undocumented dependency on lib/sqldb, but doesn't try to link it in, so the build fails unless you --enable-httpd, which is currently the only thing that tries to link in lib/sqldb. (This is what's causing the HarborMaster build failures, fwiw.) * it brings in a bunch of experimental code with no way to compile that out * there's some configure settings for dealing with it, but they don't interact well together I had a chat with Ken and Nicola today, and have put together some changes, with the following goals: * as light a touch as reasonable * add an --enable-objectstore configure option (default: no) to control whether objectstore code is built at all * keep the existing --with-openio[...] and --with-caringo options for selecting the backend * ditch the --enable-dummy_objstore option altogether * use the dummy implementation only if objectstore is enabled but neither backend has been selected The changes are on the "v30/optional-objectstore" branch on my github, for now: https://github.com/elliefm/cyrus-imapd/tree/v30/optional-objectstore This isn't on master yet cause there's some caveats: The implementation necessitates a bunch of "#ifdef ENABLE_OBJECTSTORE" around the code paths that use it (imap/append.c and imap/mailbox.c). I don't much care for this -- it makes maintenance and testing down the track fiddly. But the alternatives seem to be the current state, which we already know is broken, or adding a no-op objectstore implementation. The latter seems pretty nasty from the way we use this: there's lots of code like "if the option is set in imapd.conf, do it the objectstore way, otherwise, do it the normal way". If we were to just drop in a no-op objectstore implementation then cyrus admins would be only a config setting away from data loss, and that feels all kinds of wrong. So, #ifdefs. The dummy implementation is not a no-op implementation, it's a flat file-based emulation. "Dummy" is possibly a misleading name here, by which I mean, it mislead me until I read the code. Maybe we should rename it, but for now: light touch. The caringo/openio detection code is pretty frail and needs attention. For example, if you --with-caringo=yes without having the dependencies installed, it just happily adds the dependencies straight into LDFLAGS(!)... which makes subsequent unrelated checks that try to compile a code fragment fail unexpectedly. The openio path has the good grace to fail clumsily on its own when the dependencies are missing, though it is just as guilty of blatting into stuff it shouldn't touch. The --enable-objectstore option still has the undocumented dependency on --enable-http (in order to bring in lib/sqldb). Light touch. I'll end up fixing this at some point anyway if no-one else does first, because the backup code I'm working on has a similar dependency. I'm not sure about the terminology here -- "object store" / "object storage" / various capitalisations / without spaces. I've just made some wild guesses for now. If you have an opinion, tell me about it. The imapd.conf setting for enabling object storage is always present, whether the feature is compiled in or not. There's a fair bit of work in making this not the case (changing the scripts that generate lib/imapopts.[ch] and man/imapd.conf.5 from lib/imapoptions). And we already have other settings that are nonsensical unless optional features are compiled in, so there's precedent in just leaving it as is -- at least for now. Light touch. And for all that, I don't have the dependencies for either Caringo or OpenIO handy, so I haven't tested those code paths beyond verifying that it fails as expected when I try to enable them. I've tried not to alter what was already there, but I can't verify that I haven't accidentally broken something somehow. If you've been able to get either of these actually working, I'd appreciate if you could try again with my branch and let me know if it still works. It does pass our tests without objectstore, and with the dummy objectstore implementation. So, those sure are some caveats hey. I think the main questions at this point are: * are we happy with the combination of --enable-objectstore to turn the feature on, plus --with-[backend] to select a backend, and default to dummy if no other backend was selected? * are we happy/comfortable/resigned to the #ifdef'd approach to compiling it out? * did I manage to avoid breaking anything that used to work? If those are yes, then I'll merge it onto master, and the rest can be figured out as we go. Cheers, ellie
