Fwupd-Backend Integration on discover requires fwupd (https://github.com/hughsie/fwupd/)
Closed, ResolvedPublic

Description

I recently merged the Fwupd-Backend, on discover master. The Fwupd-Backend Requires fwupd ((https://github.com/hughsie/fwupd/) on the CI system, to build discover.

Restricted Application added a subscriber: sysadmin. · View Herald TranscriptAug 3 2018, 3:13 AM
ngraham reassigned this task from apol to bcooksley.Aug 3 2018, 3:20 AM
ngraham added a subscriber: apol.

Couple of issues with this:

  1. You've introduced a new mandatory dependency without the required two week notification in advance period. This is a violation of community policy surrounding dependency changes for projects covered by the CI system. Fortunately it is easy to satisfy in this case, but none the less we should have been informed in advance of this landing.
  1. fwupd is not available outside of Linux, yet Discover is supported (for compilation at least) on FreeBSD. I don't know if it had any functional backends before, but if it has you've now denied those users the option of using Discover - because one backend cannot be built. You'll need to make this optional.

I will add fwupd-devel to the CI images for Linux shortly.

adridg added a subscriber: adridg.Aug 3 2018, 10:08 AM

I also spotted some discussion of this on IRC. I think the fwupd is now optional everywhere since a55b13c3f3a5ba4ab081dbf5f0493d35a39a743c . However, current git master gives me:

-- Checking for module 'flatpak>=0.6.12'
--   Package 'flatpak', required by 'virtual:world', not found
CMake Error in cmake/FindLIBFWUPD.cmake:
  A logical block opening on the line

    /home/adridg/src/kde/discover/cmake/FindLIBFWUPD.cmake:34 (if)

  is not closed.
Call Stack (most recent call first):
  CMakeLists.txt:33 (find_package)

.. the CMake error is because a55b13c3f3a5ba4ab081dbf5f0493d35a39a743c does not have an endif() added in the new if-clause.

Some other comments, not specifically related to this task or that commit:

  • Your 3-clause BSD license refers to "the University" which is of course copied from the original 3-clause Berkley license. You are not a University, and the wording can be better (most use "the author")
  • The if-style in FindLIBFWUPD.cmake is rather old-fashioned; I don't think modern style repeats the conditions any more.

Hey adridg now the missing endif() is fixed by this commit (https://github.com/KDE/discover/commit/5dbb15a3007bc00abfe85455c8de545dc6fc5de8) other than that, thanks for pointing out the mistake in the license, I just copied that from another CMake file, I will fix the License in my next patch.

Bcooksley, sorry for violating community policy, I asked for the procedure for adding the dependency on IRC and was not knowing about this rule, will sure keep in mind the next time.

bcooksley closed this task as Resolved.Aug 5 2018, 12:06 AM

Thanks for fixing that.

Fortunately it was pretty easy to get this all sorted out!

As the builds are passing again i'm going to close this now.