Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

meson: decouple session tracking mechanism from unit and sysusers #417

Merged
merged 1 commit into from
Jul 18, 2024

Conversation

bluca
Copy link
Member

@bluca bluca commented Jan 18, 2024

Session tracking build options are about whether logind or other APIs
are used in the polkit code, at compilation and linking time.

units, sysusers and tmpfiles files have nothing to do with code changes
or APIs, they simply install config files, that can just be ignored if
they are not needed. Use pkg-config if available, but otherwise have
a hard-coded fallback with the well-known defaults.

This also fixes another bug, 'systemdsystemunitdir' is specified as
an option the systemd_dep variable is not defined, but the sysusers.d
directory lookup uses it, causing a build failure:

dh_auto_configure -- \
	-Dexamples=false \
	-Dintrospection=true \
	-Dman=true \
	-Dsystemdsystemunitdir=/usr/lib/systemd/system \
	-Dtests=true \
	-Dgtk_doc=true -Dsession_tracking=libsystemd-login
	cd obj-x86_64-linux-gnu && DEB_PYTHON_INSTALL_LAYOUT=deb LC_ALL=C.UTF-8 meson setup .. --wrap-mode=nodownload --buildtype=plain --prefix=/usr --sysconfdir=/etc --localstatedir=/var --libdir=lib/x86_64-linux-gnu -Dpython.bytecompile=-1 -Dexamples=false -Dintrospection=true -Dman=true -Dsystemdsystemunitdir=/usr/lib/systemd/system -Dtests=true -Dgtk_doc=true -Dsession_tracking=libsystemd-login
The Meson build system
Version: 1.3.1
Source dir: /builds/bluca/polkit/debian/output/source_dir Build dir: /builds/bluca/polkit/debian/output/source_dir/obj-x86_64-linux-gnu Build type: native build
Project name: polkit
Project version: 124

<...>

Run-time dependency libsystemd found: YES 255
Checking for function "sd_uid_get_display" with dependency libsystemd: YES
Checking for function "sd_pidfd_get_session" with dependency libsystemd: YES
../meson.build:222:37: ERROR: Unknown variable "systemd_dep".

Follow-up for 24f1e0a

meson.build Outdated Show resolved Hide resolved
@bluca bluca changed the title meson: fix build failure when -Dsystemdsystemunitdir is specified meson: decouple session tracking mechanism from unit and sysusers May 15, 2024
meson.build Outdated
systemd_systemdsystemunitdir = systemd_dep.get_pkgconfig_variable('systemdsystemunitdir')
endif

systemd_sysusers_dir = systemd_dep.get_pkgconfig_variable('sysusers_dir', default: '/usr/lib/sysusers.d')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should at least provide an option to manually specify the directory like for system units, in case there is no libsystemd.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unrelated to libsystemd, it's a separate and independent pkg-config file

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, then in case there's no systemd.pc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then it's a bug in the package/build script/etc and it needs to be provided

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unnecessarily unkind to packagers to add a new required dependency just to get a single path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why is it acceptable to fall back to the default value if the .pc file is present but doesn't contain the required option, but it's not acceptable to fall back to the default value if the .pc file is not present?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, that can be dropped too

Copy link
Member

@jrybar-rh jrybar-rh May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean to ad fuel to the fire but, TBH, configuring 'dependency' if 'dependency' is not (and probably never will be) present on the system (be it systemd or whatever) seems like abomination to me (/me just shot himself in foot, probably), even though we use prepared and good-looking system like pkgconf. @bluca, this is nothing personal, I just believe there are cleaner ways than dropping unused config files in possibly-yet-non-existent directories on systems that have right to go different ways. But maybe I'm still a purist in this topic :D

Copy link

@classabbyamp classabbyamp Jun 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't force a stub .pc file. The existence of systemd.pc (even a stub) can imply that systemd exists on the system, and there will be programs that will make that assumption and configure themselves incorrectly.

(speaking as a maintainer of a downstream distro that happens to not use most systemd components)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't mean to ad fuel to the fire but, TBH, configuring 'dependency' if 'dependency' is not (and probably never will be) present on the system (be it systemd or whatever) seems like abomination to me (/me just shot himself in foot, probably), even though we use prepared and good-looking system like pkgconf. @bluca, this is nothing personal, I just believe there are cleaner ways than dropping unused config files in possibly-yet-non-existent directories on systems that have right to go different ways. But maybe I'm still a purist in this topic :D

I disagree, but it's your project, so as you wish - the pkgconf is now optional, and the well-known default installation directories are used as a fallback if missing.

@fanyx
Copy link

fanyx commented Jun 2, 2024

Please be aware that in the current state this commit is in you would be breaking 042897e again.

meson.build Outdated Show resolved Hide resolved
…pfiles

Session tracking build options are about whether logind or other APIs
are used in the polkit code, at compilation and linking time.

units, sysusers and tmpfiles files have nothing to do with code changes
or APIs, they simply install config files, that can just be ignored if
they are not needed. Use pkg-config if available, but otherwise have
a hard-coded fallback with the well-known defaults.

This also fixes another bug, 'systemdsystemunitdir' is specified as
an option the systemd_dep variable is not defined, but the sysusers.d
directory lookup uses it, causing a build failure:

dh_auto_configure -- \
	-Dexamples=false \
	-Dintrospection=true \
	-Dman=true \
	-Dsystemdsystemunitdir=/usr/lib/systemd/system \
	-Dtests=true \
	-Dgtk_doc=true -Dsession_tracking=libsystemd-login
	cd obj-x86_64-linux-gnu && DEB_PYTHON_INSTALL_LAYOUT=deb LC_ALL=C.UTF-8 meson setup .. --wrap-mode=nodownload --buildtype=plain --prefix=/usr --sysconfdir=/etc --localstatedir=/var --libdir=lib/x86_64-linux-gnu -Dpython.bytecompile=-1 -Dexamples=false -Dintrospection=true -Dman=true -Dsystemdsystemunitdir=/usr/lib/systemd/system -Dtests=true -Dgtk_doc=true -Dsession_tracking=libsystemd-login
The Meson build system
Version: 1.3.1
Source dir: /builds/bluca/polkit/debian/output/source_dir
Build dir: /builds/bluca/polkit/debian/output/source_dir/obj-x86_64-linux-gnu
Build type: native build
Project name: polkit
Project version: 124

<...>

Run-time dependency libsystemd found: YES 255
Checking for function "sd_uid_get_display" with dependency libsystemd: YES
Checking for function "sd_pidfd_get_session" with dependency libsystemd: YES
../meson.build:222:37: ERROR: Unknown variable "systemd_dep".

Follow-up for 24f1e0a
@jrybar-rh jrybar-rh merged commit 3a8fddb into polkit-org:main Jul 18, 2024
32 checks passed
@jrybar-rh
Copy link
Member

I got a message that tools like tmpfiles.d and systemd-sysusers can also exist separately from systemd, hence it make sense (since we use those tools) to provide configuration files for them. IF they are not desired on a specific distro, a package maintainer for that distro can always exclude those files. Agreed?
Anyway, thanks, @bluca, for your time and your work!

@bluca
Copy link
Member Author

bluca commented Jul 18, 2024

I got a message that tools like tmpfiles.d and systemd-sysusers can also exist separately from systemd, hence it make sense (since we use those tools) to provide configuration files for them

Yes there are separate implementations in bash for both. Thanks.

@bluca bluca deleted the meson_fix branch July 18, 2024 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants