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

qt: include qttranslations properly #246022

Merged
merged 4 commits into from
Aug 15, 2023
Merged

qt: include qttranslations properly #246022

merged 4 commits into from
Aug 15, 2023

Conversation

K900
Copy link
Contributor

@K900 K900 commented Jul 29, 2023

Description of changes

Qt loads its own translations from a hardcoded path, and those are used (among other things) for determining RTL layout preferences in applications, so they are definitely something we want to have.

This adds a qtbase/qttools rebuild to the chain, but it's fast enough that it's probably fine.

Another approach would be to load translation paths from the environment, and inject it in wrapQtAppsHook, but that seems like more complexity for very questionable build time savings.

Fixes #86054. Fixes #240101.

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.11 Release Notes (or backporting 23.05 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.

@K900
Copy link
Contributor Author

K900 commented Jul 29, 2023

Also something like this will also need to be done for Qt6, I just started looking into 5 because the original reported issue was that Plasma isn't handling RTL correctly.

@mahmoudajawad
Copy link

The reported issue: #240101

Thanks for your time and effort, @K900.

@K900
Copy link
Contributor Author

K900 commented Jul 29, 2023

Drafting for now to clean up the workarounds all over the tree.

@K900 K900 force-pushed the qt5-translations branch from 3aef9df to 9414b35 Compare July 29, 2023 11:06
@github-actions github-actions bot added 6.topic: fetch 6.topic: TeX Issues regarding texlive and TeX in general labels Jul 29, 2023
@K900 K900 changed the base branch from staging to master July 29, 2023 11:07
@github-actions github-actions bot removed 6.topic: fetch 6.topic: TeX Issues regarding texlive and TeX in general labels Jul 29, 2023
@K900 K900 changed the title qt5: include qttranslations properly qt: include qttranslations properly Jul 29, 2023
@ofborg ofborg bot added 2.status: merge conflict This PR has merge conflicts with the target branch and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jul 29, 2023
@K900 K900 force-pushed the qt5-translations branch from 9414b35 to 31d9ca7 Compare July 29, 2023 12:05
@K900
Copy link
Contributor Author

K900 commented Jul 29, 2023

Verified all the affected packages build and run except stellarium (still building qtwebengine-6) and valentina (builds, but crashes on startup, same on master).

@K900
Copy link
Contributor Author

K900 commented Jul 29, 2023

Fixed qt6.qtwebengine and Stellarium.

@K900 K900 force-pushed the qt5-translations branch from 398ac52 to d0c3b91 Compare July 31, 2023 16:50
@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 31, 2023
@K900 K900 requested a review from NickCao August 3, 2023 13:31
@K900 K900 force-pushed the qt5-translations branch from d0c3b91 to b00e313 Compare August 3, 2023 13:32
@NickCao
Copy link
Member

NickCao commented Aug 4, 2023

Can we skip the bootstrapping like just build qttranslations without qtbase?

@K900
Copy link
Contributor Author

K900 commented Aug 4, 2023

No, it needs qttools to actually generate the final files.

@K900 K900 force-pushed the qt5-translations branch from b00e313 to d11dcf2 Compare August 4, 2023 06:04
Copy link
Member

@NickCao NickCao left a comment

Choose a reason for hiding this comment

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

I'm happy with the changes. Waiting for others' opinions.

@K900 K900 force-pushed the qt5-translations branch 2 times, most recently from 502d9fb to 6b7235e Compare August 6, 2023 14:30
@@ -15,6 +15,7 @@
# optional dependencies
, cups ? null, postgresql ? null
, withGtk3 ? false, dconf, gtk3
, qttranslations ? null
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this ? null needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it will break baseScope but I moved things around a bit since the last time I tried. I'll have to double check.

Copy link
Contributor

@Mindavi Mindavi left a comment

Choose a reason for hiding this comment

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

Haven't tested but the diff seems fine. I don't know that much about the scopes part, but presumably it's now tested and at least fixes some of the LTR/RTL issues

@wineee
Copy link
Member

wineee commented Aug 9, 2023

@ofborg build qt6.qtwebengine

K900 added 4 commits August 15, 2023 22:11
Qt loads its own translations from a hardcoded path, and those are used
(among other things) for determining RTL layout preferences in applications,
so they are definitely something we want to have.

This adds a qtbase/qttools rebuild to the chain, but it's fast enough that it's
probably fine.

Another approach would be to load translation paths from the environment,
and inject it in wrapQtAppsHook, but that seems like more complexity
for very questionable build time savings.
There are two kinds of changes here:
- removing explicit qttranslations path hardcoding from applications that were patched to do it
- replacing qttranslations in buildInputs with qttools for packages that really depend on the latter

After this, qttranslation is never used outside Qt itself, as it should.
@K900 K900 force-pushed the qt5-translations branch from 6b7235e to 6edd6f7 Compare August 15, 2023 19:12
@K900
Copy link
Contributor Author

K900 commented Aug 15, 2023

I'm just going to merge this before we accumulate more treewide horkage.

@K900 K900 merged commit 2d38e63 into NixOS:staging Aug 15, 2023
@alyssais
Copy link
Member

Since this change, it's not possible to use qt5.qttranslations, or anything that depends on it, in nativeBuildInputs when cross compiling. For example, pkgsCross.aarch64-multiplatform.pkgsBuildHost.qt5.qttranslations fails to build:

this derivation will be built:
  /nix/store/rfgxh3b2abdncfxyf5xhiwv9x2r35cad-qttranslations-5.15.10.drv
qttranslations> building '/nix/store/rfgxh3b2abdncfxyf5xhiwv9x2r35cad-qttranslations-5.15.10.drv'
qttranslations> Error: detected mismatched Qt dependencies:
qttranslations>     /nix/store/bnc68g66zdygbri398vwvq90s92l8xvl-qtbase-5.15.10-dev
qttranslations>     /nix/store/ddmypadxkl61f68lp8vif42rqp6ym3nm-qtbase-5.15.10-dev
error: build of '/nix/store/rfgxh3b2abdncfxyf5xhiwv9x2r35cad-qttranslations-5.15.10.drv' failed

This worked before this change, and is orthogonal to cross compiling qt itself — pkgsBuildHost.qt5.qttranslations should be exactly the same as pkgsBuildBuild.qt5.qttranslations, but somehow isn't any more.

@Artturin
Copy link
Member

#269756

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