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.

Sign up for GitHub

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

Jump to bottom

New package: seatd-0.4.0 #24580

Merged
merged 1 commit into from Nov 17, 2020
Merged

New package: seatd-0.4.0 #24580

merged 1 commit into from Nov 17, 2020

Conversation

ifreund
Copy link
Contributor

@ifreund ifreund commented Aug 31, 2020 •
edited

This is not ready to be merged as upstream does not yet version the shared library files. Also, tests are currently patched out due to build failure in release mode (this is already fixed upstream but unreleased).

The only consumer of this daemon/library is the wlroots master branch as far as I know, so it doesn't really make sense to merge before the next wlroots release anyways.

Edit: this is good to merge

I expect that seatd will become a popular alternative to elogind on void linux, so I've made this preliminary package to facilitate early testing.

ericonr
ericonr reviewed Aug 31, 2020
View reviewed changes
Copy link
Member

@ericonr ericonr left a comment

Choose a reason for hiding this comment

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

Nice going, we'll see when wlroots gets a new release with support!

srcpkgs/seatd/template Outdated Show resolved Hide resolved
srcpkgs/seatd/template Outdated
version=0.2.0
revision=1
build_style=meson
configure_args="-Dexamples=disabled -Dlogind=enabled"
Copy link
Member

Choose a reason for hiding this comment

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

Make elogind a build option, perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be optional and we need a parallel option in wlroots.

Copy link
Member

@ericonr ericonr Aug 31, 2020 •
edited

Choose a reason for hiding this comment

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

Is wlroots going to move exclusively to libseat? Would be nice if we could control everything with libseat build options.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I that's the long term plan as far as I know, but I wouldn't expect that to happen before the next wlroots release. Agreed that that being able to control everything with libseat build options would be best.

srcpkgs/seatd/template Outdated Show resolved Hide resolved
srcpkgs/seatd/template Outdated Show resolved Hide resolved
srcpkgs/seatd/template Outdated Show resolved Hide resolved
srcpkgs/seatd/files/seatd/run Outdated Show resolved Hide resolved
@ericonr
Copy link
Member

ericonr commented Aug 31, 2020

I believe the build is trying to use scdoc from the target, not the host. Should look into how that's being done.

@ifreund
Copy link
Contributor Author

ifreund commented Aug 31, 2020

I believe the build is trying to use scdoc from the target, not the host. Should look into how that's being done.

Added a fix for this to the patch, will submit it upstream in a minute as well.

Also changed the service to pass -g seatd and added an install message.

@ifreund ifreund changed the title New package: seatd-0.2.0 New package: seatd-0.3.0 Sep 8, 2020
@ifreund ifreund marked this pull request as ready for review September 8, 2020 11:25
@ifreund
Copy link
Contributor Author

ifreund commented Sep 8, 2020

This should be ready to go now. Upstream pushed a new tag including fixes for the issues mention and versioning the .so.

I also added a default disabled build option for elogind support as I think most users of seatd will be using it because they do not want to use elogind.

ericonr
ericonr reviewed Sep 8, 2020
View reviewed changes
common/shlibs Outdated Show resolved Hide resolved
srcpkgs/seatd/template Outdated Show resolved Hide resolved
@ifreund
Copy link
Contributor Author

ifreund commented Oct 21, 2020

Updated to seatd 0.4.0

Duncaen
Duncaen reviewed Oct 21, 2020
View reviewed changes
srcpkgs/seatd/INSTALL.msg Outdated
option which sets the group owning the socket. Thus, you will most
likely want to add your user to the seatd group:

# usermod -aG seatd <username>
Copy link
Member

Choose a reason for hiding this comment

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

Install messages are not documentation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The fact that the seatd group in particular is used is because of the -g seatd flag passed in the run file and is specific to void's package. Is that still not worth mentioning? I'd be happy to remove the install message as well.

Copy link
Member

Choose a reason for hiding this comment

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

Install messages spam the output and users don't read them, adding more just makes users more unlikely to read them if there are actually important messages.

You could add a README.voidlinux file like the sqmail package does.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Eh, I just removed the INSTALL.msg, I think this info is probably a better fit for the "Session and Seat Management" void docs page. The seatd man page and the 1 line run file should be plenty for most users to figure out how to use it.

@ifreund ifreund changed the title New package: seatd-0.3.0 New package: seatd-0.4.0 Oct 21, 2020
@ghost
Copy link

ghost commented Oct 30, 2020 •
edited by ghost

Not sure if I misunderstood anything, but wouldn't it be better to separate libseat from seatd and make seatd depend on it? This way when wlroots with libseat support gets released (it's already implemented in master), it could be compiled with libseat support instead of elogind and depend on libseat.

@ifreund
Copy link
Contributor Author

ifreund commented Oct 30, 2020 •
edited

Not sure if I misunderstood anything, but wouldn't it be better to separate libseat from seatd and make seatd depend on it? This way when wlroots with libseat support gets released (it's already implemented in master), it could be compiled with libseat support instead of elogind and depend on libseat.

libseat is separated from seatd, but seatd doesn't depend on it. seatd is a standalone daemon while libseat is a library for programs such as a wayland compositor to interact with that daemon (and optionally logind as well).

When the next wlroots version is released it will indeed depend on libseat by default, though I'm not sure we should remove elogind dependence by default just yet. These decisions can be exposed as build options of course.

@ghost
Copy link

ghost commented Oct 30, 2020 •
edited by ghost

I see. So shouldn't a separate PR for libseat be made (since this package only adds seatd)? I should learn to read

@ifreund
Copy link
Contributor Author

ifreund commented Oct 30, 2020

I see. So shouldn't a separate PR for libseat be made (since this package only adds seatd)?

libseat is added as a subpackage in this PR

@ghost ghost mentioned this pull request Oct 30, 2020
Closed
@ghost ghost mentioned this pull request Nov 6, 2020
Closed
ghost
ghost suggested changes Nov 8, 2020
View reviewed changes
srcpkgs/seatd/template Outdated Show resolved Hide resolved
srcpkgs/seatd/files/seatd/run Outdated Show resolved Hide resolved
@ghost ghost mentioned this pull request Nov 8, 2020
Merged
ghost
ghost suggested changes Nov 10, 2020
View reviewed changes
srcpkgs/seatd/template Show resolved Hide resolved
@ifreund
New package: seatd-0.4.0
9537c76 
The elogind build option is enabled by default so that we can enable
only the libseat wlroots backend by default. This is also the right
default if other projects start using seatd.
@ericonr
Copy link
Member

ericonr commented Nov 17, 2020

This is ready to be merged, right?

@ifreund
Copy link
Contributor Author

ifreund commented Nov 17, 2020

This is ready to be merged, right?

@ericonr Indeed, this PR is good to go.

@ericonr ericonr merged commit b79c6df into void-linux:master Nov 17, 2020
@ifreund ifreund deleted the seatd branch November 17, 2020 18:31
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Duncaen Duncaen Duncaen left review comments

@ericonr ericonr ericonr left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ifreund @ericonr @Duncaen