Skip to content

qsv: 0.131.1 -> 0.138.0, build simplification#356504

Merged
wegank merged 3 commits intoNixOS:masterfrom
itepastra:qsv
Nov 19, 2024
Merged

qsv: 0.131.1 -> 0.138.0, build simplification#356504
wegank merged 3 commits intoNixOS:masterfrom
itepastra:qsv

Conversation

@itepastra
Copy link
Member

Update qsv to version 0.138.0, also fix build

ZHF: #352882

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@ofborg ofborg bot requested review from detroyejr and uncenter November 17, 2024 04:55
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Nov 17, 2024
@uncenter
Copy link
Member

uncenter commented Nov 17, 2024

I'm getting this test error on darwin:

failures:

---- test_clipboard::clipboard_success stdout ----
thread 'test_clipboard::clipboard_success' panicked at tests/test_clipboard.rs:22:42:
called `Result::unwrap()` on an `Err` value: ClipboardNotSupported - "The selected clipboard is not supported with the current system configuration."
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@itepastra
Copy link
Member Author

It seems to use arboard for it's clipboard, which is made for X11 and thus won't work on darwin. I'll add it as a disabled test on darwin

@uncenter
Copy link
Member

Is it worth utilizing the new useFetchCargoVendor option from #349360 to avoid vendoring the Cargo.lock here? I believe we are only vendoring it because of the git deps.

@ofborg ofborg bot requested a review from uncenter November 18, 2024 09:26
@itepastra itepastra force-pushed the qsv branch 2 times, most recently from 3879ee8 to 25cf65f Compare November 18, 2024 15:18
@itepastra
Copy link
Member Author

Like this? @uncenter

@itepastra itepastra changed the title qsv: 0.131.1 -> 0.138.0 qsv: 0.131.1 -> 0.138.0, build simplification Nov 18, 2024
@uncenter
Copy link
Member

Yep that looks exactly right! Thank you!

@itepastra
Copy link
Member Author

itepastra commented Nov 18, 2024

I can't test if it still works correctly on darwin since I don't have a mac

@uncenter
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 356504


aarch64-darwin

✅ 1 package built:
  • qsv

@uncenter uncenter added the 12.approvals: 1 This PR was reviewed and approved by one person. label Nov 18, 2024
@uncenter
Copy link
Member

It seems to use arboard for it's clipboard, which is made for X11 and thus won't work on darwin. I'll add it as a disabled test on darwin

Interestingly the clipboard feature seems to work, but the test doesn't. Odd!

@itepastra
Copy link
Member Author

In the test it explicitly tries to call a library that is made fox X11, maybe it's real code doesn't do this. Am uncertain

@uncenter
Copy link
Member

No worries yeah.

@uncenter uncenter closed this Nov 18, 2024
@uncenter uncenter reopened this Nov 18, 2024
@uncenter
Copy link
Member

Wrong button oops 😓

Copy link
Contributor

@detroyejr detroyejr left a comment

Choose a reason for hiding this comment

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

This looks good to me and works on nixos! I have a slight preference to sort the skipped tests within each comment section to keep those grouped, but it's not super important.

@uncenter thanks for the cargo lock file suggestion. Wasn't aware of that and I'll be using that in the future!

@detroyejr detroyejr added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. label Nov 18, 2024
@uncenter uncenter added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Nov 18, 2024
@uncenter
Copy link
Member

Can I get these GitHub actions checks re-run? Looks like they all failed when I accidentally closed this, but they should be passing fine.

@itepastra
Copy link
Member Author

I just rebased the last commit to rerun the tests, didn't change anything

@ofborg ofborg bot requested review from detroyejr and uncenter November 18, 2024 21:05
@uncenter uncenter mentioned this pull request Nov 19, 2024
14 tasks
@ofborg ofborg bot requested review from detroyejr and uncenter November 19, 2024 10:44
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Nov 19, 2024
@wegank wegank merged commit e9398b5 into NixOS:master Nov 19, 2024
@itepastra itepastra deleted the qsv branch November 20, 2024 08:06
@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2024

Successfully created backport PR for release-24.11:

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

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants