Details
- Reviewers
JoshStrobl - Group Reviewers
Triage Team - Maniphest Tasks
- T9691: Package Request: Waybar
- Commits
- R5329:71c1178c3b5b: Initial commit of waybar
Built on local repo and confirmed waybar launched and functioned as expected on Sway
Diff Detail
- Repository
- R5329 waybar
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
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. | |
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!
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.
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. |
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.
| package.yml | ||
|---|---|---|
| 13 | I see no libappndicator entry in abi_used_libs. Is this really required to build waybar? | |
| 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. | |
| 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? | |
| 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. | |
| 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! | |
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).
Waybar actually built fine with libayatana-appindicator in place of appindicator. System tray also working without any patching (nice surprise!).
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.