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

qt6.qtdeclarative: re-enable qml caching #339706

Open
wants to merge 2 commits into
base: staging-next
Choose a base branch
from

Conversation

outfoxxed
Copy link
Contributor

@outfoxxed outfoxxed commented Sep 5, 2024

Description of changes

Re-enables qml caching.

The QML tag ensures only the same version of qtdeclarative that generated a cache can load it, and the qmlc file names are now hashed with their store path to ensure they are unique. AOT compiled / bytecode caches have no versioning issues to worry about and are also re-enabled.

Potential issues:

  • Currently we avoid qmlc collisions by including the store path of the application in the qmlc hash. This is problematic because it won't replace old caches when the derivation changes, just make new ones. I'm holding off on making changes to the structure of qmlc files until I get some input on this, but I think not cluttering the user's cache with outdated files is a good idea when possible.
  • Replacing the qmlc version tag may not actually be required, especially because qt mirror source code should have a .tag file. We should drop that commit if possible.
  • I'm not sure what the patch file convention is supposed to be, as there are a bunch of both numbered and non numbered patches in the qt patches folder.

I've tested this with a couple kde programs and made roughly sure it works. Further testing will be done for a final revision.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@outfoxxed
Copy link
Contributor Author

@outfoxxed
Copy link
Contributor Author

@K900 Made the changes we discussed. The hashes are now included in the library hash field for nix store paths.

I've opted to just refuse to cache bare qml files on nix store paths for now, but embedded qml files and non store files will be cached.

@outfoxxed
Copy link
Contributor Author

*Fixed error messages on mismatch

@outfoxxed
Copy link
Contributor Author

outfoxxed commented Oct 18, 2024

*fixed conflicts with changes on master

Note: patch has been tested for: mismatched qtdeclarative, mismatched applications, qml on bare store paths.

@outfoxxed
Copy link
Contributor Author

outfoxxed commented Oct 30, 2024

Should I add myself to qtdeclarative's maintainers for this patch as well?

@K900
Copy link
Contributor

K900 commented Oct 30, 2024

If you're willing to, sure.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Oct 30, 2024
@outfoxxed
Copy link
Contributor Author

added, otherwise unchained

@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Oct 31, 2024
@github-actions github-actions bot added the 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions label Dec 10, 2024
@K900 K900 changed the base branch from staging to staging-next December 10, 2024 10:13
@nix-owners
Copy link

nix-owners bot commented Dec 10, 2024

The PR's base branch is set to staging, but 372 commits from the master branch are included. Make sure you know the right base branch for your changes, then:

  • If the changes should go to the master branch, change the base branch to master
  • If the changes should go to the staging branch, rebase your PR onto the merge base with the staging branch:
    # git rebase --onto $(git merge-base upstream/staging HEAD) $(git merge-base upstream/master HEAD)
    git rebase --onto fa22dc9e8e3be834f7da92223a0f03767468e151 cd34221a4b06d3b9af727f18322a45af1c787733
    git push --force-with-lease

@github-actions github-actions bot added 10.rebuild-linux: 1001-2500 10.rebuild-darwin: 5001+ 10.rebuild-linux: 5001+ and removed 6.topic: python 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: GNOME GNOME desktop environment and its underlying platform 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: policy discussion 6.topic: vscode 6.topic: games 6.topic: k3s Kubernates distribution (https://k3s.io/) 6.topic: deepin Desktop environment and its components 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 10.rebuild-linux: 501+ 10.rebuild-linux: 501-1000 10.rebuild-darwin: 101-500 10.rebuild-linux: 1001-2500 labels Dec 10, 2024
@SuperSandro2000
Copy link
Member

@K900 we want to merge this?

@K900
Copy link
Contributor

K900 commented Dec 10, 2024

Next cycle.

@outfoxxed
Copy link
Contributor Author

@K900 we want to merge this?

Note that I haven't tested the patch against 6.8.1 yet, though no changes were required.

I was thinking I would wait for staging-next to have more dependencies done before testing it, but just remembered I have an environment for 6.8.1 on top of unstable already. I can retest the patch itself there tomorrow, though I can't test the nix expression.

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: 101-500 10.rebuild-linux: 501+ 10.rebuild-linux: 1001-2500 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

4 participants