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

qownnotes: build with Qt6 #227301

Merged
merged 1 commit into from
May 3, 2023
Merged

Conversation

totoroot
Copy link
Contributor

@totoroot totoroot commented Apr 20, 2023

Description of changes
  • Build qownnotes with Qt6
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.05 Release Notes (or backporting 22.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@pbek
Copy link
Contributor

pbek commented Apr 20, 2023

You can drop qtxmlpatterns altogether, I removed the need for that dependency when porting the app to Qt6.

@totoroot
Copy link
Contributor Author

Rebased onto master after version update got merged.
Removed dependency qtxmlpatterns.

@pbek pbek mentioned this pull request Apr 21, 2023
12 tasks
@SuperSandro2000
Copy link
Member

@SuperSandro2000 Can you please review and let me know whether adding support for Qt6 like this is alright? Per default qownnotes would still be built using Qt5.

Why do we need both options? If qt6 just works, we can only offer that.

@pbek
Copy link
Contributor

pbek commented Apr 24, 2023

@SuperSandro2000 Can you please review and let me know whether adding support for Qt6 like this is alright? Per default qownnotes would still be built using Qt5.

Why do we need both options? If qt6 just works, we can only offer that.

Yeah, I was asking the same in #227301 (comment).
Would there be an advantage in keeping both, e.g. Better integration or disk space?

@totoroot
Copy link
Contributor Author

I saw this merged PR and the comment by @peterhoeg and thought that Qt6 might not be usable for some, so I wanted to be on the safe side like they did with zeal.

If there is no advantage in keeping both, then I will happily adapt the PR to only use Qt6.

@pbek
Copy link
Contributor

pbek commented Apr 24, 2023

And 32bit isn't supported by Qt6 any more...

The issue with qt6 is that the applications look like crap under KDE because the theme isn't picked up automatically.
#202103 (comment)

Yes, I feared as much.
In KDE Neon I had no UI issues with the Qt6 version of QOwnNotes.

Copy link
Contributor

@pbek pbek left a comment

Choose a reason for hiding this comment

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

qownnotes-qt6 builds and works nice. I coudn't find any issues.

@wegank
Copy link
Member

wegank commented Apr 27, 2023

Needs rebase at least.

@totoroot totoroot force-pushed the update/qownnotes-qt6 branch 2 times, most recently from 5d66b7b to dbfdf61 Compare May 3, 2023 21:16
@totoroot totoroot changed the title qownnotes: add option for using Qt6 qownnotes: build with Qt6 May 3, 2023
@totoroot
Copy link
Contributor Author

totoroot commented May 3, 2023

Since the consensus seems to be that there is no real problem in moving to building with Qt6, I changed the PR to only build with Qt6.
I guess we'll see whether someone complains about this move breaking something.

On NixOS with KDE Plasma QOwnNotes built with Qt6 looks just fine. I've also successfully built it on x86_64-darwin, and the only issue I've seen, is that the title bar is not adapting to my dark system theme, but I guess that's manageable.

@totoroot totoroot marked this pull request as ready for review May 3, 2023 21:21
@totoroot
Copy link
Contributor Author

totoroot commented May 3, 2023

Not relevant to this PR, but I'm getting a bunch of warnings like this for these two plugins.

Warning: In /nix/store/milahyvgak2ym1d5hk2zyl7pbljljjs7-qtwayland-5.15.8-bin/lib/qt-5.15.8/plugins/wayland-shell-integration/libwl-shell.so:
  Plugin uses incompatible Qt library (5.15.0) [release] ((null):0, (null))   nqqjc2vn-systemd-253.3-dev
...
Warning: In /nix/store/milahyvgak2ym1d5hk2zyl7pbljljjs7-qtwayland-5.15.8-bin/lib/qt-5.15.8/plugins/wayland-graphics-integration-client/libxcomposite-glx.so:
  Plugin uses incompatible Qt library (5.15.0) [release] ((null):0, (null))   1.29-dev/include -I/nix/st

@pbek are you aware of this?

@totoroot
Copy link
Contributor Author

totoroot commented May 3, 2023

Result of nixpkgs-review pr 227301 run on x86_64-linux 1

1 package built:
  • qownnotes

@totoroot
Copy link
Contributor Author

totoroot commented May 3, 2023

Result of nixpkgs-review pr 227301 run on x86_64-darwin 1

1 package built:
  • qownnotes

@wegank
Copy link
Member

wegank commented May 3, 2023

Result of nixpkgs-review pr 227301 run on aarch64-darwin 1

1 package built:
  • qownnotes

@wegank
Copy link
Member

wegank commented May 3, 2023

Not relevant to this PR, but I'm getting a bunch of warnings like this for these two plugins.

Warning: In /nix/store/milahyvgak2ym1d5hk2zyl7pbljljjs7-qtwayland-5.15.8-bin/lib/qt-5.15.8/plugins/wayland-shell-integration/libwl-shell.so:
  Plugin uses incompatible Qt library (5.15.0) [release] ((null):0, (null))   nqqjc2vn-systemd-253.3-dev
...
Warning: In /nix/store/milahyvgak2ym1d5hk2zyl7pbljljjs7-qtwayland-5.15.8-bin/lib/qt-5.15.8/plugins/wayland-graphics-integration-client/libxcomposite-glx.so:
  Plugin uses incompatible Qt library (5.15.0) [release] ((null):0, (null))   1.29-dev/include -I/nix/st

@pbek are you aware of this?

This is not specific to qownnotes, see #227795.

@wegank wegank merged commit aa22d5c into NixOS:master May 3, 2023
@pbek
Copy link
Contributor

pbek commented May 4, 2023

Thank you!

@pbek
Copy link
Contributor

pbek commented May 4, 2023

I got similar (but different) warnings:

[08:52:26] [warning] In /nix/store/dlqrinrvpw234k2q1hsb5x4y0k90jw46-qtdeclarative-5.15.8-bin/lib/qt-5.15.8/qml/QtQml/Models.2/libmodelsplugin.so: Plugin uses incompatible Qt library (5.15.0) [release]
[08:52:26] [warning] In /nix/store/dlqrinrvpw234k2q1hsb5x4y0k90jw46-qtdeclarative-5.15.8-bin/lib/qt-5.15.8/qml/QtQml/WorkerScript.2/libworkerscriptplugin.so: Plugin uses incompatible Qt library (5.15.0) [release]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants