allow building without mdraid & crypto

[heitbaum]

Draft wip patch following up against

• allow building without mdraid & crypto #554
• libblockdev libcryptsetup should be a flagged dependency #487
  Hopefully addresses some of the concerns previously raised:
• keeping the functions in place but returning no-op by restricting the ifdefs to the actual function bodies
• Keeping a single code path

if this is on the right track then can look at adding the g_dbus_method_invocation_return_error (invocation, UDISKS_ERROR, UDISKS_ERROR_NOT_SUPPORTED, "Support for yyy is unavailable."); the same as is proposed on #1389

[tbzatek]

Well I appreciate your effort but I'm not convinced that we should make these two features optional. The prime directive in this project is to never break any API, ABI or its behaviour. The problem is that existing consumers have built their stuff based on public API documentation, observed behaviour and returned error codes. Since neither of the features in question were declared as optional, our consumers expect to be always available and there's no handling in place for the opposite case. Missing interfaces or objects may pose a problem and lead to crashes. As we have no insight on how our consumers are using our API, we try to keep it rock solid. Some features like ATA SMART do expose a property indicating presence of probed data and clients are using that as an indication of a feature presence. There's nothing like that available for mdraid or crypto and it's too late to add that since due to existing consumers.

FWIW, I'm not convinced I want to merge #1390, the amount of #ifdefs is horrible and hard to maintain. Might need to find a different approach in libblockdev.

[heitbaum]

Our operating system does not have either mdraid nor crypto, thus with an option like this then udisks can’t be added. It would be ideal to have the options available in upstream, as hoarding downstream patches is its own problem. Fully onboard about api / abi, but “the ask” previously was to keep all the functions there (use ifdef within the function bodies) but with them returning as not implemented - I believe that this patch version (whilst needing the return_error to be added achieves this.)

Please let me know the best way to move this forward.

[tbzatek]

I'm still thinking about a good enough compromise to accommodate both parties' needs.

It would be ideal to have the options available in upstream, as hoarding downstream patches is its own problem.

Well that's another problem - the core UDisks team doesn't have resources to officially support these options. There's a growing list of important issues reported and we currently can't even keep up fixing it. So adding another combination of features officially is simply not an option. If your proposal ever happens, it will be declared as unsupported - e.g. warning messages on daemon startup to clearly indicate this fact, a build-time big warning about unsupported build, etc. Until somebody steps up and signs up for long-term maintenance for the disabled-features scenario.

Another aspect is the CI - somebody would need to step up again and come up with new and modified tests for a second compile + test run having some feature disabled and sign up for long-term maintenance of these tests. The current set of CI tests have only a single set of features enabled based on particular distro availability. Even then we do spend significant amount of time maintaining the CI and fixing new issues that arise from distros evolving. UDisks and libblockdev do consume a lot of external libraries and commands and this is generally very prone to bitrot. Maintaining a degree of quality of our projects this way is one of the primary goals. That's just another reason to limit the amount of combinations we'd need to test.

On example of such undermaintenance is #1365, so this is already happening. As a result the released code might not even compile or run properly until properly covered by CI tests.

Even with all these there's still a danger of existing consumers misbehaving on corner cases, expecting things to be present while they're not. Not everything can be covered by CI tests.

I understand the general tendency to move as many things upstream to lessen the downstream maintenance burden, but given all the above, not sure if it's a big win for you.

“the ask” previously was to keep all the functions there (use ifdef within the function bodies) but with them returning as not implemented

It's more complicated than that unfortunately, fully agree with this approach for method calls though. But property values, linked object paths, presence of D-Bus interfaces and objects is something that can't be easily worked around by returning an error. That's where you'd see errors like "No such interface" or even crashes with existing consumers that expect something to be present while it's not.

So this is my official position to ensure a degree of quality. An open question that remains is how do we accommodate consumers on restricted or embedded systems who only call a small amount of methods or use UDisks for basic device enumeration.

Bear in mind that even on restricted systems there are still ways to come across a storage technology that doesn't seem obvious to be ever present. You may download an image and set it up as a loop device, it may contain LUKS or mdraid inside. You may run hardware that has no way for connecting an NVMe SSD, but you may still use NVMe over Fabrics (this is why libnvme is mandatory btw.).