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

python3Packages.av: fix build #327626

Merged
merged 2 commits into from
Jul 16, 2024
Merged

Conversation

emilazy
Copy link
Member

@emilazy emilazy commented Jul 16, 2024

Description of changes

Many of the tests depend on sample files fetched over HTTP and they keep adding more. Since they support a caching mechanism, use it to pre‐fetch all the used samples instead.

It might be nice to have a package for the full FATE suite, but it’s over 1 GiB in size so I just did this for now.

Fixes: ca258d2

Going to run a full nixpkgs-review on a master‐based version of this, but it’s probably fine to merge optimistically.

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.

Many of the tests depend on sample files fetched over HTTP and they
keep adding more. Since they support a caching mechanism, use it to
pre‐fetch all the used samples instead.

It might be nice to have a package for the full FATE suite, but it’s
over 1 GiB in size so I just did this for now.

Fixes: ca258d2
@Atemu
Copy link
Member

Atemu commented Jul 16, 2024

We might want to reconsider running these tests at all if they require such maintenance overhead or imply huge build time closures.

@emilazy
Copy link
Member Author

emilazy commented Jul 16, 2024

In this current PR it’s like 16 MiB of samples and one bash script to keep them updated in case they add more; I think it’s fine. The overhead was manually disabling every test that wants a sample file, which should no longer be necessary. There’s upstream issues at PyAV-Org/PyAV#623 and PyAV-Org/PyAV#955 (hi @mweinelt).

At some point it might be nice to run the full FATE test suite against our FFmpeg builds, really, at which point we’d have to start thinking about size.

@emilazy
Copy link
Member Author

emilazy commented Jul 16, 2024

This breaks Home Assistant’s weird PyAV fork thing… I’ll look into it tomorrow.

@Atemu
Copy link
Member

Atemu commented Jul 16, 2024

cc @mweinelt

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

nice!

@emilazy
Copy link
Member Author

emilazy commented Jul 16, 2024

I just talked with @mweinelt on Matrix and we’re going to try replacing Home Assistant’s outdated PyAV fork with the real thing. Will report back on the nixpkgs-review results when I have them.

@mweinelt mweinelt self-assigned this Jul 16, 2024
@emilazy emilazy requested a review from mweinelt as a code owner July 16, 2024 19:52
@emilazy
Copy link
Member Author

emilazy commented Jul 16, 2024

Alright, this has no regressions vs. Hydra on master (except whisper-ctranslate2 which doesn’t look related so it’s probably an issue with my regression testing methodology?). Should be good to merge.

@mweinelt mweinelt merged commit db8e14c into NixOS:staging Jul 16, 2024
23 of 27 checks passed
@emilazy emilazy deleted the push-yvluzttstnzz branch August 26, 2024 01:04
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.

4 participants