Bug 240138

Comment 1 Kubilay Kocak 2019-08-27 02:50:24 UTC

Most ports matching disk* are in the sysutils category, so that's probably a better primary category for it. 

We try to avoid devel unless its specifically and only development related, and only then if there's not a better primary category (where devel can be secondary)

Comment 2 Gleb Popov 2019-08-27 07:29:30 UTC

UDisks contains Linux-specific backed part. When considering porting it, I decided that it is easier to implement the same service for FreeBSD from scratch. This resulted in sysutils/bsdisks port. It implements minimal functionality required to mount volumes at the moment.

The libudisks part of UDisks project is a D-Bus bindings library, and should be portable.

(In reply to Gleb Popov from comment #2)
Has anyone asked the upstream if they are willing to accept patches to build just the client library or add FreeBSD-specific code? If the upstream accepts patches for non-Linux operating systems, I think it will be nice to have at least the client library, so we can stop converting projects using the client library to do D-Bus calls directly.

I just ran 'readelf -d /usr/libexec/udisks2/udisksd' on Linux, and I found it needs GLib. It may be a problem on FreeBSD because FreeBSD geom library is known to conflict with GLib. Both FreeBSD geom library and GLib use 'g_' prefix for their function names, which is known to cause problems for libgtop and force us to disable all code using geom in libgtop.

(In reply to Ting-Wei Lan from comment #3)
OK, this sounds to me like you and Gleb Popov are suggesting to do the following, not necessarily in that order:

1- split off libudisks from udisks2,
2- patch it so it can use bsdisks instead,
3- offer those patches to upstream for their consideration.

If my understanding is correct, who's in charge of drafting an email to the udisks2 upstream? Me? (I don't want to tread on anyone's toes, but I don't want this bug to sit around gathering dust if no one else has time to move on it.)

(In reply to Kubilay Kocak from comment #1)
Is the "maintainer feedback" flag asking me to agree with the change from devel/ to sysutils/ ? If so, how do I say I agree (which I do)? Change it to + ?

Comment 6 Gleb Popov 2019-08-28 07:07:05 UTC

(In reply to PauAmma from comment #4)
You shouldn't need p.2 at all and regarding ps. 1 and 3, I'd recommend you do the following:

- name the port sysutils/libudisks
- use UDisks2 distfile
- patch configure script to pass on FreeBSD (it looks for udev, which is not applicable for us)
- build only libudisks target and install its products

In ideal case, this shouldn't even require communicating with upstream.

I'd do this myself, but I'm away from computer ATM.

(In reply to Gleb Popov from comment #6)
It is not required, but it is usually better to let the upstream know that there are people interested in building only the client library. Ideally, all required patches will be merged upstream and no patch will be required in the port.

I just submitted a pull request to upstream to allow building without the Linux-only udisks daemon: https://github.com/storaged-project/udisks/pull/693. It can be built by passing --disable-daemon --disable-gtk-doc to configure, but there are a few warnings when using the client library with bsdisks.

(process:23042): GLib-GIO-WARNING **: 12:21:10.961: Trying to set property Configuration of type aa{sv} but according to the expected interface the type is a(sa{sv})

(process:23042): GLib-GIO-WARNING **: 12:21:10.961: Trying to set property Symlinks of type as but according to the expected interface the type is aay

Comment 9 Gleb Popov 2019-09-08 10:03:03 UTC

(In reply to Ting-Wei Lan from comment #8)

Great news!

> there are a few warnings when using the client library with bsdisks.

Should be fixed in bsdisks HEAD revision. Please try it out.

(In reply to Gleb Popov from comment #9)
The warning for Symlinks is now resolved, but Configuration still shows warnings.

(gnome-control-center:64166): GLib-GIO-WARNING **: 18:22:04.156: Trying to set property Configuration of type (sa{sv}) but according to the expected interface the type is a{sv}

Comment 11 Gleb Popov 2019-09-08 10:39:27 UTC

(In reply to Ting-Wei Lan from comment #10)
Should be fixed too now. Thanks for testing.

(In reply to Gleb Popov from comment #11)
Yes, it is fixed. There is no warning now.

(In reply to Ting-Wei Lan from comment #8)
Wow! You're way faster that me. Thanks. *goes look at PR to see what needed to be done*

https://github.com/storaged-project/udisks/pull/693 has been accepted and merged by upstream.

Created attachment 208102 [details]
Patch for new port sysutils/libudisks

Tested against sysutils/bsdisks
Also tested with:
- portlint -AN
- port test from porttools (the upstream head, https://github.com/skreuzer/porttools)
- poudriere

Comment 16 Gleb Popov 2019-10-06 08:29:04 UTC

Patches in files/ are huge. Why do you need all these? I had an impression that there should be only some tweaks in autotools stuff.

(In reply to Gleb Popov from comment #16)
Upstream requires gtk-doc build to work even if --disable-daemon is used. To fix gtk-doc build, functions and types which are parts of the daemon have to be moved to different files. Otherwise, gtk-doc will complain that it can't find some functions and types when the daemon isn't built.

There are totally 8 commits in the pull request.

Comment on attachment 208102 [details]
Patch for new port sysutils/libudisks

>+# pkaction takes care of libpolkit-agent-1 and libpolkit-gobject-1
>+# which are what we really want (see UDISKSCTL_LIB_DEPENDS).
>+BUILD_DEPENDS=	bash:shells/bash gtkdocize:textproc/gtk-doc \

Is there any reason to put two dependencies in one line here?

>+OPTIONS_DEFINE=	UDISKSCTL
>+OPTIONS_SUB=	yes
>+UDISKSCTL_DESC=	Install udisksctl command line utility
>+UDISKSCTL_LIB_DEPENDS=	libpolkit-agent-1.so:sysutils/polkit \
>+			libpolkit-gobject-1.so:sysutils/polkit

Can we enable UDISKSCTL by default? It is just a small tool that is useful for testing. It also makes the port similar to the upstream default because the upstream project doesn't have an option to disable udisksctl.

>+
>+# config.status errors out without --disable-dependency-tracking, with or
>+# without USES=gmake.
>+do-configure:
>+	cd ${WRKSRC} && \
>+	./autogen.sh --disable-daemon --disable-gtk-doc --disable-nls \

Why --disable-nls is used? If you think NLS should be optional, put it under NLS option. NLS is enabled by default. --disable-gtk-doc should also be optional, so users can decide if the documentation is needed.

(In reply to Gleb Popov from comment #16)
What Ting-Wei Lan said. Nearly all those patches will go away when udisks 2.9.0 is released and the port updated to it. The only one that may remain is one inspired from devel/libdbusmenu/files/patch-libdbusmenu-glib_Makefile.in that makes the upstream Makefiles respect PREFIX for everything. Otherwise, there are 2 files that would end under /usr/local no matter what.

(In reply to Ting-Wei Lan from comment #18)
> Is there any reason to put two dependencies in one line here?

To answer your question as asked: because it fit within the 80 columns of my terminal window and because neither the Porter's Handbook nor portlint said to do it otherwise. (Should they?) But to answer it as you probably meant it: since I have to make the other changes you requested, I'll split that line as well.

Created attachment 208219 [details]
Should take care of all changes requested.

Comment 22 Gleb Popov 2019-10-10 07:21:00 UTC

Is

GH_TAGNAME=	udisks-2.8.4

really required? Looks like default value.

Other than that looks good to me. I'm a bit hesitant to commit such big patches in files/ but waiting for next release is an even worse idea, I think.

(In reply to Gleb Popov from comment #22)

Without it (and with distfiles adjusted to reflect the changed filename) I get:

% make fetch
===>  License LGPL20+ accepted by the user
===>   libudisks-2.8.4 depends on file: /usr/local/sbin/pkg - found
=> storaged-project-udisks-2.8.4_GH0.tar.gz doesn't seem to exist in /usr/home/pauamma/ports/distfiles/.
=> Attempting to fetch https://codeload.github.com/storaged-project/udisks/tar.gz/2.8.4?dummy=/storaged-project-udisks-2.8.4_GH0.tar.gz
fetch: https://codeload.github.com/storaged-project/udisks/tar.gz/2.8.4?dummy=/storaged-project-udisks-2.8.4_GH0.tar.gz: Not Found
=> Attempting to fetch http://distcache.FreeBSD.org/ports-distfiles/storaged-project-udisks-2.8.4_GH0.tar.gz
fetch: http://distcache.FreeBSD.org/ports-distfiles/storaged-project-udisks-2.8.4_GH0.tar.gz: Not Found
=> Couldn't fetch it - please try to retrieve this
=> port manually into /usr/home/pauamma/ports/distfiles/ and try again.
*** Error code 1

With it, make fetch works normally:

% make fetch
===>  License LGPL20+ accepted by the user
===>   libudisks-2.8.4 depends on file: /usr/local/sbin/pkg - found
=> storaged-project-udisks-2.8.4-udisks-2.8.4_GH0.tar.gz doesn't seem to exist in /usr/home/pauamma/ports/distfiles/.
=> Attempting to fetch https://codeload.github.com/storaged-project/udisks/tar.gz/udisks-2.8.4?dummy=/storaged-project-udisks-2.8.4-udisks-2.8.4_GH0.tar.gz
fetch: https://codeload.github.com/storaged-project/udisks/tar.gz/udisks-2.8.4?dummy=/storaged-project-udisks-2.8.4-udisks-2.8.4_GH0.tar.gz: size unknown
fetch: https://codeload.github.com/storaged-project/udisks/tar.gz/udisks-2.8.4?dummy=/storaged-project-udisks-2.8.4-udisks-2.8.4_GH0.tar.gz: size of remote file is not known
storaged-project-udisks-2.8.4-udisks-2.8.4_GH0        1345 kB 1229 kBps    02s
===> Fetching all distfiles required by libudisks-2.8.4 for building

Comment on attachment 208219 [details]
Should take care of all changes requested.

>Index: sysutils/libudisks/pkg-plist
>===================================================================
>--- sysutils/libudisks/pkg-plist	(nonexistent)
>+++ sysutils/libudisks/pkg-plist	(working copy)
>@@ -0,0 +1,149 @@
>+%%UDISKSCTL%%bin/udisksctl
>+include/udisks2/udisks/udisks-generated.h
>+include/udisks2/udisks/udisks.h
>+include/udisks2/udisks/udisksclient.h
>+include/udisks2/udisks/udisksenums.h
>+include/udisks2/udisks/udisksenumtypes.h
>+include/udisks2/udisks/udiskserror.h
>+include/udisks2/udisks/udisksobjectinfo.h
>+include/udisks2/udisks/udiskstypes.h
>+include/udisks2/udisks/udisksversion.h
>+lib/girepository-1.0/UDisks-2.0.typelib
>+lib/libudisks2.a
>+lib/libudisks2.so
>+lib/libudisks2.so.0
>+lib/libudisks2.so.0.0.0
>+libdata/pkgconfig/udisks2.pc
>+%%UDISKSCTL%%man/man1/udisksctl.1.gz
>+%%UDISKSCTL%%share/bash-completion/completions/udisksctl
>+%%DOCS%%share/gir-1.0/UDisks-2.0.gir

Why gir file is only installed when DOCS is enabled? I know gir file isn't needed during runtime, but I think the convention is to always install it when gobject-introspection is enabled.

(In reply to Gleb Popov from comment #22)
It is required because the tag is udisks-2.8.4, not just 2.8.4.

But probably we can use

DISTVERSIONPREFIX= udisks-

instead of setting GH_TAGNAME so we don't have to write the version number twice in the same file.

Created attachment 208253 [details]
Second round of changes

(In reply to PauAmma from comment #26)
The patch looks good to me now. The only remaining problem I found is likely a bsdisks bug: 'udisksctl dump' shows nothing when it is run the first time. Running 'udisksctl dump' again shows the correct result.

Comment 28 commit-hook 2019-10-12 18:56:40 UTC

A commit references this bug:

Author: arrowd
Date: Sat Oct 12 18:55:52 UTC 2019
New revision: 514350
URL: https://svnweb.freebsd.org/changeset/ports/514350

Log:
  sysutils/libudisks: New port. A client library to UDisks daemon, which some applications are using to interface with UDisks.

  PR:		240138

Changes:
  head/sysutils/Makefile
  head/sysutils/libudisks/
  head/sysutils/libudisks/Makefile
  head/sysutils/libudisks/distinfo
  head/sysutils/libudisks/files/
  head/sysutils/libudisks/files/patch-Makefile.am
  head/sysutils/libudisks/files/patch-configure.ac
  head/sysutils/libudisks/files/patch-data_Makefile.am
  head/sysutils/libudisks/files/patch-doc_Makefile.am
  head/sysutils/libudisks/files/patch-doc_man_Makefile.am
  head/sysutils/libudisks/files/patch-doc_udisks2-docs.xml.daemon.man.in
  head/sysutils/libudisks/files/patch-doc_udisks2-docs.xml.daemon.part.in
  head/sysutils/libudisks/files/patch-doc_udisks2-docs.xml.daemon.sed
  head/sysutils/libudisks/files/patch-doc_udisks2-docs.xml.in.in
  head/sysutils/libudisks/files/patch-doc_udisks2-sections.txt.daemon.sections.in
  head/sysutils/libudisks/files/patch-doc_udisks2-sections.txt.daemon.sed
  head/sysutils/libudisks/files/patch-doc_udisks2-sections.txt.in.in
  head/sysutils/libudisks/files/patch-doc_udisks2.types.daemon.in
  head/sysutils/libudisks/files/patch-doc_udisks2.types.daemon.sed
  head/sysutils/libudisks/files/patch-doc_udisks2.types.in.in
  head/sysutils/libudisks/files/patch-tools_Makefile.am
  head/sysutils/libudisks/files/patch-udisks_Makefile.am
  head/sysutils/libudisks/files/patch-udisks_udisksclient.c
  head/sysutils/libudisks/pkg-descr
  head/sysutils/libudisks/pkg-plist

Comment 29 Gleb Popov 2019-10-12 18:58:24 UTC

Thanks everyone involved.