Skip to content

Comments

flameshot: 12.1.0-unstable-2025-05-04 -> 13.0.1#431307

Merged
SuperSandro2000 merged 4 commits intoNixOS:masterfrom
dmkhitaryan:flameshot
Aug 16, 2025
Merged

flameshot: 12.1.0-unstable-2025-05-04 -> 13.0.1#431307
SuperSandro2000 merged 4 commits intoNixOS:masterfrom
dmkhitaryan:flameshot

Conversation

@dmkhitaryan
Copy link
Contributor

@dmkhitaryan dmkhitaryan commented Aug 6, 2025

Things done

Diff: flameshot-org/flameshot@v12.1.0...v13.0.1
Changelog: https://github.com/flameshot-org/flameshot/releases/tag/v13.0.1

Also packages its dependencies, qt-color-widgets + qhotkey.

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@Sigmanificient
Copy link
Member

This CI is failing because you are not defined into the maintainers list

@nixpkgs-ci nixpkgs-ci bot added the 12.first-time contribution This PR is the author's first one; please be gentle! label Aug 6, 2025
@dmkhitaryan
Copy link
Contributor Author

This CI is failing because you are not defined into the maintainers list

Thank you for noticing. I added it to the init package but forgot to take it out since the other PR with that commit is pending approval and merge (and figured making multiple "add maintainer" commits wouldn't be good either).

@Sigmanificient
Copy link
Member

Sigmanificient commented Aug 6, 2025

Unfortunately, your best option is to put the maintainer commit into both pr, and rebase one of the two when the other gets merged first. This way it does not block this pr from being merged

@dmkhitaryan dmkhitaryan force-pushed the flameshot branch 2 times, most recently from 40bc119 to f45359e Compare August 6, 2025 01:22
@Sigmanificient
Copy link
Member

Wow I did not expected flameshot to have over 20k objects files, this is going to take a while to compile 😅

@dmkhitaryan
Copy link
Contributor Author

Wow I did not expected flameshot to have over 20k objects files, this is going to take a while to compile 😅

Yeah, that'll be fun.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Aug 6, 2025
@dmkhitaryan dmkhitaryan force-pushed the flameshot branch 3 times, most recently from 4c36e2e to b9a643a Compare August 6, 2025 15:46
@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. labels Aug 6, 2025
@dmkhitaryan
Copy link
Contributor Author

nixpkgs-review result for #431307

Generated using nixpkgs-review-gha

Command: nixpkgs-review pr 431307

Logs: https://github.com/dmkhitaryan/nixpkgs-review-gha/actions/runs/16782408421


x86_64-linux

✅ 3 packages built:
  • flameshot
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)
  • libsForQt5.qt-color-widgets (plasma5Packages.qt-color-widgets)

aarch64-linux

✅ 3 packages built:
  • flameshot
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)
  • libsForQt5.qt-color-widgets (plasma5Packages.qt-color-widgets)

x86_64-darwin (sandbox = false)

✅ 2 packages built:
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)
  • libsForQt5.qt-color-widgets (plasma5Packages.qt-color-widgets)

aarch64-darwin (sandbox = false)

✅ 2 packages built:
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)
  • libsForQt5.qt-color-widgets (plasma5Packages.qt-color-widgets)

flameshot doesn't build on Darwin supposedly due to kguiaddons-6.16.0 being unsupported.

@acid-bong
Copy link
Contributor

acid-bong commented Aug 6, 2025

@K900 (as an author of #292288), how can we port a KDE+Qt6 app to Darwin? is a large change in pkgs/kde required?

@K900
Copy link
Contributor

K900 commented Aug 6, 2025

Honestly, most of KF6 stuff should build just fine on Darwin, but I don't want to flag it supported unless someone wants to step up and actually keep it that way.

@dmkhitaryan
Copy link
Contributor Author

At this point I am doubtful about the Darwin support for the package altogether. Trying to build it by temporarily allowing unsupported systems revealed two more issues:

  1. valgrind is marked as broken on Darwin
  2. Even when likewise temporarily allowing for broken packages, kdePackages.wayland fails to build, throwing this error:
../src/wayland-client.c:2006:9: error: call to undeclared function 'ppoll'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
 2006 |                 ret = ppoll(pfd, 1, remaining_timeout, NULL);
      |                       ^
../src/wayland-client.c:2006:9: note: did you mean 'poll'?
/nix/store/hjmg118wbm4fkdinkxvvvd9y6m2rbf5a-apple-sdk-11.3/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/sys/poll.h:113:12: note: 'poll' declared here
  113 | extern int poll(struct pollfd *, nfds_t, int) __DARWIN_ALIAS_C(poll);
      |            ^
1 error generated.

Thus having kguiaddons and flameshot fail to build as well. Having checked the man poll(2) page, the used standard is specific to Linux. Not to say that it is unsolvable, but being mainly on Linux, I frankly have little idea of how to approach this problem elegantly. Therefore, I would like to ask if it is reasonable to instead remove Darwin from the list of supported platforms by this package.

@K900
Copy link
Contributor

K900 commented Aug 8, 2025

We may need to be a little smarter about auto-deps for this, but it should be possible to fix.

@dmkhitaryan
Copy link
Contributor Author

We may need to be a little smarter about auto-deps for this, but it should be possible to fix.

That is promising. What would the process of resolving this problem roughly look like? Might as well give it a try first and learn from it (or maybe someone else more knowledgeable does).

@K900
Copy link
Contributor

K900 commented Aug 8, 2025

Basically make the Linux-specific dependencies in pkgs/kde actually conditional on Linux, and then possibly add more precise meta.platforms and make the auto-dep logic in mkKdeDerivation consider that (probably via availableOn).

@acid-bong
Copy link
Contributor

Maybe ask the upstream whether it's that simple to provide Qt6 on Darwin?

@K900
Copy link
Contributor

K900 commented Aug 8, 2025

There's really no reason it shouldn't be. Except that we haven't done it yet.

@dmkhitaryan
Copy link
Contributor Author

dmkhitaryan commented Aug 8, 2025

There's really no reason it shouldn't be. Except that we haven't done it yet.

Indeed, Qt6 is properly supported. I looked across Issues and PRs and have found that to be the case. So, I decided to look at the project's CMake lists again. That's when I realised two things.

  1. There's no need to add kguiaddons to Darwin because the project uses it for Wayland-specific purposes. I thus wrapped it as a built input that only gets passed if the platform is not Darwin. That got the project to actually build. Until it failed at the very end due to codesign error, which is the second issue.
  2. When comparing building from v12 to v13, the developers have added signing to the package. This led to the failure to build, since codesign was not present in the environment. Honestly, I am not entirely sure if and what tools in Nix exist to address this beyond darwin.sigtool (which hasn't received meaningful updates since 2022). Given that the previous version of the package did not have this, I patched it out in the new version as well.

TLDR after going through the steps above I have gotten the package to build and run as seen in the image below (in a moment of excitement I took the screenshot with Flameshot as seen below). I will tidy up the mess over/after the weekend and then commit those changes as well.

Flameshot on aarch64-darwin image

That leaves me with one question for now: Is it worth opening an issue over at Flameshot given the relatively convoluted patching of CMakeLists it takes to package the app in Nixpkgs? Or is this common, something you simply have to deal with?


P.S: I forgot the solution also involved adding another package that is not present in Nixpkgs, QHotKey

@dmkhitaryan dmkhitaryan force-pushed the flameshot branch 2 times, most recently from 06236fd to 747e7ca Compare August 10, 2025 10:10
@MarcelCoding
Copy link
Member

MarcelCoding commented Aug 12, 2025

Does not core dumps and kind of works for me (Gnome and ENV var QT_QPA_PLATFORM=wayland) with NuschtOS/nuschtpkgs@f89b735

Although, copying the screenshot with ctrl + c or the copy button does not work. Saving the image to disk works.

@dmkhitaryan
Copy link
Contributor Author

Thank you everyone for pointing out what was overlooked.

@SuperSandro2000 @MarcelCoding Regarding the GNOME/GTK issue, given the changes in build in v13 of Flameshot, wrapping the old way led to errors; particularly, on MacOS that caused duplication issues. Still, after a bit of digging, I have found that setting dontWrapQtApps is redundant, as indicated here; including wrapQtAppsHook as a native build input will cover what is necessary for Wayland, and it's already included in the package.
I did not get any errors when running the binary on Niri and (after installing for testing) GNOME either (for reference, I am on the unstable channel).

As for the copy-pasting problem, that is an upstream issue: https://bugs.kde.org/show_bug.cgi?id=508069
I wonder if it is also tied to Flameshot crashing if you screenshot via clicking the tray icon the second time (iff the first one screenshot placed something in the clipbaord). Either way, it's up to waiting for the resolution, then testing again.

Finally, regarding the missing icons on Darwin. @gepbird
That, I have no clue currently; What is suspicious is that I don't see qtsvg among other libraries in the framework despite it being included. I have tried to pass it via propagatedBuildInputs, but to no avail. I'll take the next few days to look through build of Flameshot again to see if I missed some macOS quirks, then update all commits also based on the reviews.

@MarcelCoding
Copy link
Member

MarcelCoding commented Aug 12, 2025

Yeah, that's basically the change I mentioned in #431307 (comment). The only difference is that I've also added the qtwayland dependency. But interesting that it works for you without that on gnome/wayland.

And thanks for the mention of https://bugs.kde.org/show_bug.cgi?id=508069. I will keep an eye on that!

@gepbird
Copy link
Contributor

gepbird commented Aug 12, 2025

Finally, regarding the missing icons on Darwin. @gepbird
That, I have no clue currently; What is suspicious is that I don't see qtsvg among other libraries in the framework despite it being included. I have tried to pass it via propagatedBuildInputs, but to no avail. I'll take the next few days to look through build of Flameshot again to see if I missed some macOS quirks, then update all commits also based on the reviews.

Thanks for looking into it, some info about my environment:
I encountered it on x86_64-linux, not Darwin. To test this PR, first I stopped the systemd user unit that is defined by the flameshot module in home-manager. Then I built the package on this PR, ran the binary and got noticed there are no icons, and made sure on HEAD~4 this worked. I'm using X11, dwm.

Now I found out deleting my home-manager generated configuration fixes the icons, most likely that module just needs to be updated. I tried it again, and I still don't have icons, I'm not sure how I got there for the first time. I also get this output:

QPainter::begin: Paint device returned engine == 0, type: 2
QPainter::setRenderHint: Painter must be active to set rendering hints
QPainter::setCompositionMode: Painter not active
QPainter::translate: Painter not active
QPainter::setPen: Painter not active
QPainter::setBrush: Painter not active
QPainter::drawEllipse: Painter not active
QPainter::setBrush: Painter not active
QPainter::drawEllipse: Painter not active

I'll check the flameshot changes and bisect my config I didn't find anything related in the changelog

@dmkhitaryan
Copy link
Contributor Author

@gepbird I wasn't able to replicate your issues on X11 (at least, not on i3wm), whether via executing the binary or passing the PR package to the Home-manager module. However, that was prior to reverting some changes which I pushed just earlier, particularly regarding Qt wrapping. That resolved the issue with invisible icons on Darwin, I wonder if it also addresses your case.

@gepbird
Copy link
Contributor

gepbird commented Aug 14, 2025

@dmkhitaryan Thank you, the icons work!

@Patrick-Poitras
Copy link
Contributor

Note: 13.0.1 is out since last week.

@dmkhitaryan dmkhitaryan changed the title flameshot: 12.1.0-unstable-2025-05-04 -> 13.0.0 flameshot: 12.1.0-unstable-2025-05-04 -> 13.0.1 Aug 15, 2025
@dmkhitaryan
Copy link
Contributor Author

Note: 13.0.1 is out since last week.

Updated and checked it.

nixpkgs-review result for #431307

Generated using nixpkgs-review-gha

Command: nixpkgs-review pr 431307

Logs: https://github.com/dmkhitaryan/nixpkgs-review-gha/actions/runs/16987875874


x86_64-linux

✅ 3 packages built:
  • flameshot
  • kdePackages.qhotkey (qt6Packages.qhotkey)
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)

aarch64-linux

✅ 3 packages built:
  • flameshot
  • kdePackages.qhotkey (qt6Packages.qhotkey)
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)

x86_64-darwin (sandbox = true)

✅ 3 packages built:
  • flameshot
  • kdePackages.qhotkey (qt6Packages.qhotkey)
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)

aarch64-darwin (sandbox = true)

✅ 3 packages built:
  • flameshot
  • kdePackages.qhotkey (qt6Packages.qhotkey)
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Tested on x86_64-linux + X11, the functions I use work well, thanks for updating!

Code looks mostly good, just a few nits below.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 15, 2025
Copy link
Member

@MarcelCoding MarcelCoding left a comment

Choose a reason for hiding this comment

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

Did not take a deep look at the changes. But tested it and works for me on Gnome/Wayland/25.05.

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Aug 15, 2025
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Aug 16, 2025
@SuperSandroBot
Copy link

nixpkgs-review result

Generated using nixpkgs-review-gha

Command: nixpkgs-review pr 431307

Logs: https://github.com/SuperSandro2000/nixpkgs-review-gha/actions/runs/17010985196


x86_64-linux

✅ 3 packages built:
  • flameshot
  • kdePackages.qhotkey (qt6Packages.qhotkey)
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)

aarch64-linux

✅ 3 packages built:
  • flameshot
  • kdePackages.qhotkey (qt6Packages.qhotkey)
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)

x86_64-darwin (sandbox = true)

✅ 3 packages built:
  • flameshot
  • kdePackages.qhotkey (qt6Packages.qhotkey)
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)

aarch64-darwin (sandbox = true)

✅ 3 packages built:
  • flameshot
  • kdePackages.qhotkey (qt6Packages.qhotkey)
  • kdePackages.qt-color-widgets (qt6Packages.qt-color-widgets)

@SuperSandro2000 SuperSandro2000 merged commit 1f5f5ac into NixOS:master Aug 16, 2025
29 of 31 checks passed
@dmkhitaryan dmkhitaryan deleted the flameshot branch August 16, 2025 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 3+ This PR was reviewed and approved by three or more persons. 12.first-time contribution This PR is the author's first one; please be gentle!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants