Note: This is a public test instance of Red Hat Bugzilla. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback at bugzilla.redhat.com.
Bug 2251581 - Review Request: gala - Gala window manager
Summary: Review Request: gala - Gala window manager
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Neal Gompa
QA Contact: Fedora Extras Quality Assurance
URL: https://github.com/elementary/gala
Whiteboard:
Depends On:
Blocks: PantheonDesktopPackageReviews
TreeView+ depends on / blocked
 
Reported: 2023-11-26 17:44 UTC by Fabio Valentini
Modified: 2023-12-01 12:55 UTC (History)
2 users (show)

Fixed In Version: gala-7.1.3-1.fc40
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2023-12-01 12:55:09 UTC
Type: ---
Embargoed:
ngompa13: fedora-review+


Attachments (Terms of Use)

Description Fabio Valentini 2023-11-26 17:44:41 UTC
Spec URL: https://decathorpe.fedorapeople.org/gala.spec
SRPM URL: https://decathorpe.fedorapeople.org/gala-7.1.3-1.fc39.src.rpm

Description:
Gala is Pantheon's Window Manager, part of the elementary project.

Fedora Account System Username: decathorpe

Comment 1 Fabio Valentini 2023-11-26 17:44:44 UTC
This package built on koji:  https://koji.fedoraproject.org/koji/taskinfo?taskID=109584280

Comment 2 Neal Gompa 2023-11-26 17:56:04 UTC
Taking this review.

Comment 3 Neal Gompa 2023-11-26 17:58:49 UTC
I would strongly recommend subpackaging out the X11 and Wayland files. Among other things, this also makes it much easier for you to declare the runtime dependencies for X11 and Wayland Gala properly.

For example, you're currently missing the required "xorg-x11-server-Xorg" runtime dependency for gala-x11, and "xorg-x11-server-Xwayland" runtime dependency for gala-wayland.

Comment 4 Neal Gompa 2023-11-26 18:01:01 UTC
You are also missing a firstboot Provides for gala itself.

It'll look something like this:

> # http://bugzilla.redhat.com/605675
> Provides: firstboot(windowmanager) = gala

Comment 5 Fedora Review Service 2023-11-26 18:01:06 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6695893
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2251581-gala/fedora-rawhide-x86_64/06695893-gala/fedora-review/review.txt

Found issues:

- A package with this name already exists. Please check https://src.fedoraproject.org/rpms/gala
  Read more: https://docs.fedoraproject.org/en-US/packaging-guidelines/Naming/#_conflicting_package_names

Please know that there can be false-positives.

---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 6 Fabio Valentini 2023-11-26 18:09:42 UTC
(In reply to Neal Gompa from comment #3)
> I would strongly recommend subpackaging out the X11 and Wayland files. Among
> other things, this also makes it much easier for you to declare the runtime
> dependencies for X11 and Wayland Gala properly.
> 
> For example, you're currently missing the required "xorg-x11-server-Xorg"
> runtime dependency for gala-x11, and "xorg-x11-server-Xwayland" runtime
> dependency for gala-wayland.

To be honest, I'd rather remove the wayland stuff entirely than do something more complicated here.
The wayland support is still rudimentary / experimental, and won't be in a shippable state for some time.

(In reply to Neal Gompa from comment #4)
> You are also missing a firstboot Provides for gala itself.
> 
> It'll look something like this:
> 
> > # http://bugzilla.redhat.com/605675
> > Provides: firstboot(windowmanager) = gala

What the hell is this? I've never seen anything like this.
I don't think gala provides what is necessary for whatever "firstboot" is, and the LightDM greeter for Pantheon uses a different compositor entirely.

Comment 7 Neal Gompa 2023-11-26 19:28:26 UTC
(In reply to Fabio Valentini from comment #6)
> (In reply to Neal Gompa from comment #3)
> > I would strongly recommend subpackaging out the X11 and Wayland files. Among
> > other things, this also makes it much easier for you to declare the runtime
> > dependencies for X11 and Wayland Gala properly.
> > 
> > For example, you're currently missing the required "xorg-x11-server-Xorg"
> > runtime dependency for gala-x11, and "xorg-x11-server-Xwayland" runtime
> > dependency for gala-wayland.
> 
> To be honest, I'd rather remove the wayland stuff entirely than do something
> more complicated here.
> The wayland support is still rudimentary / experimental, and won't be in a
> shippable state for some time.
> 

I would prefer the Wayland stuff to be shipped as a package, even if it's not used by default yet. Subpackaging it also makes it easier to not include in a spin image if it's not ready yet.

(In reply to Fabio Valentini from comment #6)
> (In reply to Neal Gompa from comment #4)
> > You are also missing a firstboot Provides for gala itself.
> > 
> > It'll look something like this:
> > 
> > > # http://bugzilla.redhat.com/605675
> > > Provides: firstboot(windowmanager) = gala
> 
> What the hell is this? I've never seen anything like this.
> I don't think gala provides what is necessary for whatever "firstboot" is,
> and the LightDM greeter for Pantheon uses a different compositor entirely.

It's required mostly by initial-setup to use the correct window manager. Strictly speaking, it's not necessary if you've got your own firstboot/initial-setup system to use instead of Anaconda's.

Comment 8 Fabio Valentini 2023-11-27 12:52:27 UTC
Spec URL: https://decathorpe.fedorapeople.org/gala.spec
SRPM URL: https://decathorpe.fedorapeople.org/gala-7.1.3-1.fc39.src.rpm

Changes:

- x11 subpackage: contains systemd user session unit for gala on X11
- wayland subpackage: contains systemd user session unit for gala on Wayland

Both get a dependency on gnome-session (which is a hard dependency for the systemd units) and on xorg-x11-server-Xorg / xorg-x11-server-Xwayland, respectively.

Note that the session definitions I'm currently using don't use systemd user session support (yet), so in fact, *both* subpackages are unused.

Comment 9 Neal Gompa 2023-11-27 15:24:10 UTC
In your noarch subpackages, I see this:

> Requires:       %{name}%{?_isa} = %{version}-%{release}

You need to drop "%{?_isa}" because it will not be properly installable otherwise.

Comment 10 Fabio Valentini 2023-11-27 15:27:33 UTC
(In reply to Neal Gompa from comment #9)
> In your noarch subpackages, I see this:
> 
> > Requires:       %{name}%{?_isa} = %{version}-%{release}
> 
> You need to drop "%{?_isa}" because it will not be properly installable
> otherwise.

Good point. Local build installed fine, but there's no guarantee that noarch packages will always match arch ...

Files updated.

Comment 11 Neal Gompa 2023-11-27 16:02:41 UTC
Review notes:

* Package follows Fedora Packaging Guidelines
* Package builds and installs
* Package licensing is correctly handled
* No serious issues from rpmlint

Note: the devel package has /usr/share/vala and /usr/share/vala/vapi without owners. You should co-own it for now and ask for the vala maintainer to create a -filesystem subpackage that packages can depend on for those directories.

PACKAGE APPROVED.

Comment 12 Fabio Valentini 2023-11-30 18:50:03 UTC
Thank you for the review!

I will make the package co-own /usr/share/vala and /usr/share/vala/vapi for now.

https://pagure.io/releng/issue/11818

Comment 13 Fabio Valentini 2023-12-01 12:55:09 UTC
Imported and built:
https://bodhi.fedoraproject.org/updates/FEDORA-2023-ad4be625e8


Note You need to log in before you can comment on or make changes to this bug.