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.

@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
@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.

@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.

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

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
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
3 checks passed
tjni added a commit that referenced this pull request Aug 21, 2023
tjni added a commit that referenced this pull request Aug 26, 2023
Manually fixed a merge conflict between #227900
and #246022.
tjni added a commit that referenced this pull request Aug 27, 2023
Resolve correctly the three-way merge between #251681,
#227900, and #246022
github-actions bot pushed a commit to arcnmx/nixpkgs-lib that referenced this pull request Aug 29, 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