Page MenuHomeSolus

Initial commit of waybar
AcceptedPublic

Authored by h3o on Thu, Sep 2, 10:33 PM.

Details

Reviewers
JoshStrobl
Group Reviewers
Triage Team
Maniphest Tasks
T9691: Package Request: Waybar
Summary

Initial commit of waybar
Resolve T9691
Depends on D11783 D11784 D11810 D11811 D11812

Test Plan

Built on local repo and confirmed waybar launched and functioned as expected on Sway

Diff Detail

Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 1744
Build 1744: arc lint + arc unit

Event Timeline

h3o created this revision.Thu, Sep 2, 10:33 PM
h3o requested review of this revision.Thu, Sep 2, 10:33 PM
JoshStrobl requested changes to this revision.Fri, Sep 3, 12:10 PM
JoshStrobl added a subscriber: JoshStrobl.

You're also missing the revisions / stack info like I requested in one of your other patches.

Saying what other packages this depends on in the summary isn't necessary. It is obvious by the abi reports and builddeps.

package.yml
12

builddeps should be sorted pkgconfig first, then explicitly named packages after. within that sorting, it should go 0-9A-Za-z, so the builddeps should be re-ordered.

Expanding on this, you have multiple duplicate builddeps. Be sure to check the dependencies section of each package when you're looking at their info, for example the deps of libgtk-3-devel

pspec_x86_64.xml
22

This should be patched to be made stateless. In other words, it should read from /etc/waybar if the folder exists, otherwise it should fallback to reading /usr/share/waybar. There is no reason it should be in a "xdg" subdir, so that needs to be fixed both during configure time as well as via patches. Shipping configs and styles in the current manner isn't acceptable, as modifications to these files will be overridden with package updates.

This revision now requires changes to proceed.Fri, Sep 3, 12:10 PM
h3o added a comment.EditedSat, Sep 4, 3:13 PM

You're also missing the revisions / stack info like I requested in one of your other patches.

Josh, could I trouble you to elaborate on this a bit more? I built waybar in a local repo against chrono-date + updated versions of fmt, sndio, libnl, and spdlog. I thought I was able to just mention this in the summary as an alternative to using parent/child revision functionality (not familiar with this). Please let me know how to proceed. I've already create a patch to make stateless and added to the config + reordered and removed duplicate dependencies, so just need to figure out this last step before I re-submit those changes for review.

btw speaking of the stateless patch, I was getting some help from ppl in IRC about how to go do that and DataDrake mentioned an alternative directory for configs (/usr/share/defaults). Any preference between /etc vs /usr/share/defaults? I've created a patch for both so just need to know which one to include and apply. Thanks!

algent added a subscriber: algent.EditedSat, Sep 4, 4:31 PM

Hi @h3o this is your current summary:

Initial commit of waybar
Depends on chrono-date (D11783).
Also depends on the following existing packages being updated: fmt, libnl, sndio, and spdlog

To link other patches to this diff, edit it to:

Initial commit of waybar
Resolve T9691

Depends on D11783 D11784

Where:
T9691 will link waybar task and it will automatically closed as resolved when this patch it is landed in repository.
D11783 will link chrono-date patch.
D11784 will link fmt patch.
For libnl, sndio and spdlog I don't see any revision. So you have to submit them, and then just link to the diff that depends on it.

If fmt depends on spdlog then you have to add Depends on <spdlog-diff> to the fmt diff not waybar (Note: Replace <spdlog-diff> with corresponding DXXXX )

As example you can see this diff https://dev.getsol.us/D11450 for calibre.

h3o updated this revision to Diff 28704.Sat, Sep 4, 8:37 PM

Initial commit of waybar

h3o marked an inline comment as done.Sat, Sep 4, 8:41 PM
JoshStrobl requested changes to this revision.Mon, Sep 6, 11:27 AM

I am not entirely sure you understand the purpose of stateless. We should be providing a vendor-provided configuration in a designated path (in our case /usr/share/defaults* or /usr/share/* depending on the application). This vendor-provided configuration should be overridden by any system-wide configuration in /etc. If there is user specific configuration, it would be appropriate for that to be read and be treated with highest priority. Typically that is in the XDG_CONFIG_DIR / XDG_HOME_DIR+.config (on most Linux OSes).

We should not be trying to read the vendor config first, we should be checking the paths of the user, using that path if it exists, then checking the system-wide configuration, returning that if it exists, then falling back to our vendor provided configuration.

files/0001-Patch-to-make-waybar-stateless.patch
32 ↗(On Diff #28704)

No this should remain.

34 ↗(On Diff #28704)

No this should remain.

42 ↗(On Diff #28704)

This should remain.

44 ↗(On Diff #28704)

Shouldn't be specifying this as the file won't exist.

45 ↗(On Diff #28704)

You already added this one.

51 ↗(On Diff #28704)

No this should still read from the prior path first.

54 ↗(On Diff #28704)

No this should remain. system-wide configuration should still be allowed but fallback when it doesn't exist.

55 ↗(On Diff #28704)

No this should remain. system-wide configuration should still be allowed but fallback when it doesn't exist.

57 ↗(On Diff #28704)

I don't think you need this twice.

This revision now requires changes to proceed.Mon, Sep 6, 11:27 AM
h3o updated this revision to Diff 28728.Mon, Sep 6, 3:57 PM

Revised stateless patch

h3o marked 9 inline comments as done.Mon, Sep 6, 4:04 PM

Created and submitted new patch. If i understood correctly, all existing directories should remain and we are just to add the /usr/share/defaults/ directory to the bottom of the list to ensure vendor config does not override user config whenever package is updated.

livingsilver94 added inline comments.
package.yml
13

I see no libappndicator entry in abi_used_libs. Is this really required to build waybar?

h3o added inline comments.Mon, Sep 6, 6:50 PM
package.yml
13

Thanks for looking into this @livingsilver94 . I was also able to build without libappindicator, but then it doesn't have support for the system tray when I actually run waybar.

h3o added inline comments.Mon, Sep 6, 6:55 PM
package.yml
13

Thanks for looking into this @livingsilver94 . I was also able to build without libappindicator, but then it doesn't have support for the system tray when I actually run waybar.

EDIT: maybe should just be listed as a rundep?

h3o marked an inline comment as done.Mon, Sep 6, 7:17 PM
livingsilver94 added inline comments.Tue, Sep 7, 10:00 AM
package.yml
13

Probably. Try to set libappindicator as rundep. If it works, all good. Honestly Waybar should be using libayatana-appindicator, but I can probably patch it myself after this patch gets merged.

algent removed a subscriber: algent.Tue, Sep 7, 10:12 AM
h3o marked an inline comment as done.Tue, Sep 7, 12:49 PM
h3o added inline comments.
package.yml
13

Tried including libappindicator as a rundep but doesn't work. It actually downloaded libayatana-appindicator as one of the dependencies. It builds ok, but when I go to install it prompts me to download libappindicator as a dependency and then it errors out due to conflicts with libayatana-indicator. Unless Josh says otherwise, will let you look into patching it afterward. Thanks!

JoshStrobl requested changes to this revision.Wed, Sep 15, 4:36 PM

That's because libappindicator was replaced with libayatana-appindicator and just not fully deprecated yet. It needs to be built with libayatana-indicator or without any system tray support (which isn't ideal, so I'd encourage that the indicator support be patched).

This revision now requires changes to proceed.Wed, Sep 15, 4:36 PM
h3o updated this revision to Diff 28828.Wed, Sep 15, 10:05 PM
h3o marked an inline comment as done.

Replaced appindicator with libayatana-appindicator in package.yml

h3o added a comment.Wed, Sep 15, 10:11 PM

Waybar actually built fine with libayatana-appindicator in place of appindicator. System tray also working without any patching (nice surprise!).

h3o added a comment.Wed, Sep 15, 10:12 PM

Note: also updated the %meson_build and %meson_install macros to %ninja_build and %ninja_install.

I took a look at the code briefly and I think Waybar communicates with an appindicator library via dbus, and dbus API did not change with the Ayatana fork. Although I'm not 100% sure, this is probably the reason.

JoshStrobl accepted this revision.Mon, Sep 27, 5:33 PM

LGTM, excellent work @h3o

This revision is now accepted and ready to land.Mon, Sep 27, 5:33 PM