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

gui-apps/waybar: fix compilation with libfmt-8 #21506

Conversation

JonasToth
Copy link
Contributor

This PR applies a patch to waybar to build with libfmt-8.
The upstream fix is already accepted, but not merged.

Upstream: Alexays/Waybar#1144
Closes: https://bugs.gentoo.org/797649
Signed-off-by: Jonas Toth gentoo@jonas-toth.eu

This PR applies a patch to waybar to build with libfmt-8.
The upstream fix is already accepted, but not merged.

Upstream: Alexays/Waybar#1144
Closes: https://bugs.gentoo.org/797649
Signed-off-by: Jonas Toth <gentoo@jonas-toth.eu>
@gentoo-bot
Copy link

Pull Request assignment

Submitter: @JonasToth
Areas affected: ebuilds
Packages affected: gui-apps/waybar

gui-apps/waybar: @JonasToth, @gentoo/proxy-maint

Linked bugs

Bugs linked: 797649


In order to force reassignment and/or bug reference scan, please append [please reassign] to the pull request title.

Docs: Code of ConductCopyright policy (expl.) ● DevmanualGitHub PRsProxy-maint guide

@gentoo-bot gentoo-bot added self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else) assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. labels Jul 2, 2021
@gentoo-repo-qa-bot
Copy link
Collaborator

Pull request CI report

Report generated at: 2021-07-02 13:10 UTC
Newest commit scanned: cbbcf6a
Status: ✅ good

There are existing issues already. Please look into the report to make sure none of them affect the packages in question:
https://qa-reports.gentoo.org/output/gentoo-ci/3b52b27ad6/output.html

Copy link
Contributor

@ionenwks ionenwks left a comment

Choose a reason for hiding this comment

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

Just a few trivialities.

Also, I'll merge the changes from #21306 at same time seeing how you already gave it your Okay.

Edit: I see you've revbumped too, normally you wouldn't need to revbump for a build failure fix, but with that PR will do given := needs it.

Comment on lines +1 to +2
diff --git a/include/util/format.hpp b/include/util/format.hpp
index 288d8f0..61e8c85 100644
Copy link
Contributor

@ionenwks ionenwks Jul 7, 2021

Choose a reason for hiding this comment

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

Rather than put the upstream link in the commit message it'd be nice to have it in the patch itself (to show its origin), add the bgo link in it too. Links in commit message also annoyingly notify upstream's issue with every rebase on github.

I could do it myself on merge but be nice to strip useless metadata too, e.g. these "diff --git" or index lines repeated through the patch, the sed in the devmanual[1] is handy for (and so is that whole page).

Also, looks like upstream's PR was merged, so may want to update the commit message for that too.

[1] https://devmanual.gentoo.org/ebuild-writing/misc-files/patches/index.html

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, just noticed both patches are identical.

It's okay to have 1 patch with a mismatching version used for both.

@@ -50,6 +50,8 @@ DEPEND="
"
RDEPEND="${DEPEND}"

PATCHES=( "${FILESDIR}/${P}-fmt-8.0.0.patch" )
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a nitpick, not necessary but it's nice to have the patch on its own line, fits better with what other ebuilds do and easier to add more or remove without restyling, e.g.

PATCHES=(
    file.patch
)

@ionenwks
Copy link
Contributor

ionenwks commented Jul 8, 2021

Well, the other one got merged first, so please rebase too :)

Again, no need for revbumps for a build failure, so keep the new revision rather than revbump again.

@JonasToth
Copy link
Contributor Author

Tank you, I am on vacation right now, so the changes will need a few more days. But at the end of the week this pr should be ready :)

@ionenwks
Copy link
Contributor

ionenwks commented Jul 12, 2021

Tank you, I am on vacation right now, so the changes will need a few more days. But at the end of the week this pr should be ready :)

@JonasToth In that case I could finish the little changes there is to do if you ack, considering this is broken in ~arch right now may be better not to wait too long.

@JonasToth
Copy link
Contributor Author

Yes, I am fine with that. I have no way to do this until at least Saturday.
So if you have the time, go ahead :)

Thank you very much!

@JonasToth JonasToth deleted the gui-apps/fix-waybar-fmtlib-dependency branch May 29, 2022 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assigned PR successfully assigned to the package maintainer(s). bug linked Bug/Closes found in footer, and cross-linked with the PR. self-maintained The PR changes only packages that are maintained by the submitter (i.e. no need to ask anybody else)
Projects
None yet
4 participants