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

ffmpeg-livepeer: init at 7.0.1; livepeer: 0.5.20 -> 0.8.0 #354396

Merged
merged 5 commits into from
Nov 13, 2024

Conversation

Bot-wxt1221
Copy link
Member

@Bot-wxt1221 Bot-wxt1221 commented Nov 8, 2024

Things done

ZHF: #352882

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

@Bot-wxt1221 Bot-wxt1221 marked this pull request as draft November 8, 2024 06:01
@Bot-wxt1221

This comment was marked as resolved.

@Bot-wxt1221 Bot-wxt1221 changed the title livepeer: 0.5.20 -> 0.8.0 [WIP] livepeer: 0.5.20 -> 0.8.0 Nov 8, 2024
@ofborg ofborg bot requested a review from elitak November 8, 2024 06:57
@Bot-wxt1221 Bot-wxt1221 changed the title [WIP] livepeer: 0.5.20 -> 0.8.0 livepeer: 0.5.20 -> 0.8.0 Nov 8, 2024
@Bot-wxt1221 Bot-wxt1221 marked this pull request as ready for review November 8, 2024 06:59
@Bot-wxt1221 Bot-wxt1221 changed the title livepeer: 0.5.20 -> 0.8.0 ffmpeg-livepeer: init at 7.0.1; livepeer: 0.5.20 -> 0.8.0 Nov 8, 2024
@Bot-wxt1221 Bot-wxt1221 force-pushed the livepeer branch 2 times, most recently from 80f5e82 to f1f7306 Compare November 8, 2024 07:21
@Bot-wxt1221 Bot-wxt1221 added the 0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign label Nov 8, 2024
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package and removed 10.rebuild-darwin: 1 10.rebuild-linux: 1 labels Nov 8, 2024
@Bot-wxt1221 Bot-wxt1221 requested a review from a team November 8, 2024 23:28
@stepbrobd
Copy link
Member

stepbrobd commented Nov 8, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 354396


aarch64-darwin

❌ 1 package failed to build:
  • livepeer
✅ 7 packages built:
  • ffmpeg-livepeer
  • ffmpeg-livepeer.bin
  • ffmpeg-livepeer.data
  • ffmpeg-livepeer.dev
  • ffmpeg-livepeer.doc
  • ffmpeg-livepeer.lib
  • ffmpeg-livepeer.man

Failed in checkPhase (btw I have sandbox enabled on my machine):

# github.com/livepeer/lpms/ffmpeg
decoder.c:286:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
E1108 23:39:34.326309   15528 ai_http.go:472] Invalid auth type
E1108 23:39:34.327021   15528 ai_http.go:480] Invalid shared secret
E1108 23:39:34.327042   15528 handlers.go:1596] HTTP Response Error statusCode=401 err=invalid secret
E1108 23:39:34.327078   15528 ai_http.go:486] Error getting mime type mime: no media type
E1108 23:39:34.327179   15528 ai_http.go:486] Error getting mime type mime: no media type
E1108 23:39:34.327256   15528 ai_http.go:493] Could not parse task ID strconv.ParseInt: parsing "": invalid syntax
--- FAIL: TestAIWorkerResults_BadRequestType (0.00s)
panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: bind: operation not permitted [recovered]
        panic: httptest: failed to listen on a port: listen tcp6 [::1]:0: bind: operation not permitted

goroutine 66 [running]:
testing.tRunner.func1.2({0x1056518a0, 0x140006fc230})
        /nix/store/sc8n76lwg70fxcwnkxhqqfvfpvclqijy-go-1.23.2/share/go/src/testing/testing.go:1632 +0x1bc
testing.tRunner.func1()
        /nix/store/sc8n76lwg70fxcwnkxhqqfvfpvclqijy-go-1.23.2/share/go/src/testing/testing.go:1635 +0x334
panic({0x1056518a0?, 0x140006fc230?})
        /nix/store/sc8n76lwg70fxcwnkxhqqfvfpvclqijy-go-1.23.2/share/go/src/runtime/panic.go:785 +0x124
net/http/httptest.newLocalListener()
        /nix/store/sc8n76lwg70fxcwnkxhqqfvfpvclqijy-go-1.23.2/share/go/src/net/http/httptest/server.go:71 +0xdc
net/http/httptest.NewUnstartedServer(...)
        /nix/store/sc8n76lwg70fxcwnkxhqqfvfpvclqijy-go-1.23.2/share/go/src/net/http/httptest/server.go:119
net/http/httptest.NewTLSServer({0x105987ba0, 0x140006fc1d0})
        /nix/store/sc8n76lwg70fxcwnkxhqqfvfpvclqijy-go-1.23.2/share/go/src/net/http/httptest/server.go:191 +0x28
github.com/livepeer/go-livepeer/server.TestAIWorkerResults_BadRequestType(0x14000b0dba0)
        /private/tmp/nix-build-livepeer-0.8.0.drv-0/source/server/ai_http_test.go:89 +0x168
testing.tRunner(0x14000b0dba0, 0x10597bb60)
        /nix/store/sc8n76lwg70fxcwnkxhqqfvfpvclqijy-go-1.23.2/share/go/src/testing/testing.go:1690 +0xe4
created by testing.(*T).Run in goroutine 1
        /nix/store/sc8n76lwg70fxcwnkxhqqfvfpvclqijy-go-1.23.2/share/go/src/testing/testing.go:1743 +0x314
FAIL    github.com/livepeer/go-livepeer/server  0.551s
FAIL

@khaneliman
Copy link
Contributor

Does __darwinAllowLocalNetworking = true; resolve it?

@emilazy
Copy link
Member

emilazy commented Nov 9, 2024

I do not think we should introduce yet more FFmpeg forks that will inevitably break when we update the FFmpeg derivation. The Hydra build failure looks related to FFmpeg 7.0, not 7.1, so if this is working with their FFmpeg 7.0 fork then I expect it would likely work with our FFmpeg 7.1.

@stepbrobd
Copy link
Member

Does __darwinAllowLocalNetworking = true; resolve it?

The test passes after adding this

@Bot-wxt1221
Copy link
Member Author

Bot-wxt1221 commented Nov 9, 2024

@emilazy The newest livepeer need their self ffmpeg. These are patch from upstream livepeer/FFmpeg#32. Another solution is just apply patch. The function avfilter_compare_sign_bypath and so on is only available here

@emilazy
Copy link
Member

emilazy commented Nov 9, 2024

The patches they apply to FFmpeg don’t seem too substantial. What error message do you get building with the normal FFmpeg package?

Sorry, missed your edit when writing this comment. Applying the patches seems better than using the fork directly, but I think it’s still quite likely to break down the line.

@Bot-wxt1221
Copy link
Member Author

Not test.

@Atemu
Copy link
Member

Atemu commented Nov 11, 2024

Ah, I recommended it without regard for what this app is. Headed ffmpeg is only useful if you're going to be using it inside of a graphical environment where you need e.g. ffplay or x11 capture support.

@Bot-wxt1221
Copy link
Member Author

@Atemu So now I switch to ffmpeg-headless and can be compiled.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM

@ofborg build ffmpeg-livepeer livepeer

pkgs/by-name/ff/ffmpeg-livepeer/package.nix Show resolved Hide resolved
pkgs/by-name/ff/ffmpeg-livepeer/package.nix Show resolved Hide resolved
@Bot-wxt1221
Copy link
Member Author

@ofborg build livepeer ffmpeg-livepeer

@Atemu
Copy link
Member

Atemu commented Nov 11, 2024

(FYI: You don't need to rebase atop of master all the time, only when there's a conflict.)

@Bot-wxt1221
Copy link
Member Author

Bot-wxt1221 commented Nov 11, 2024

@ofborg build livepeer ffmpeg-livepeer

@Bot-wxt1221 Bot-wxt1221 added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 11, 2024
@Bot-wxt1221 Bot-wxt1221 removed the request for review from a team November 11, 2024 08:16
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM.

pkgs/by-name/ff/ffmpeg-livepeer/package.nix Outdated Show resolved Hide resolved
buildInputs =
old.buildInputs or [ ]
++ lib.optionals stdenv.hostPlatform.isDarwin [
apple-sdk_11
];
Copy link
Member

Choose a reason for hiding this comment

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

This will becomme unnescessary after the current staging-next gets merged

Copy link
Contributor

Choose a reason for hiding this comment

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

The current staging-next is for 24.11, which still has the 10.12 SDK as the default on x86_64-darwin. The work to make the 11.3 SDK the default is still in progress and hasn’t been merged yet.

Copy link
Contributor

@reckenrode reckenrode Nov 11, 2024

Choose a reason for hiding this comment

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

Oh, right. ffmpeg is using the 15.0 SDK on stanging-next. My previous reply is correct in general but not relevant for ffmpeg.

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove this and let this PR target staging-next then?

Copy link
Member Author

@Bot-wxt1221 Bot-wxt1221 Nov 11, 2024

Choose a reason for hiding this comment

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

I don't agree. It's now OK to merge. We should remove it when staging-next is merged.

Copy link
Member

@jopejoe1 jopejoe1 Nov 11, 2024

Choose a reason for hiding this comment

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

According to release schedule (#339153) staging-next is planned to be merged into master on the 12th of November (tomorrow)

Copy link
Member

Choose a reason for hiding this comment

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

It’s probably not going to be tomorrow because of events and happenings. We started a bit late because of builders catching fire. Probably a few more days, but I think it’s fine to wait until the merge rather than adding overrideAttrs hacks.

Copy link
Member

Choose a reason for hiding this comment

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

Or, yes, just target this at staging-next directly and then we can merge it.

Copy link
Member Author

@Bot-wxt1221 Bot-wxt1221 Nov 11, 2024

Choose a reason for hiding this comment

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

@emilazy Done. Now I remove apple-sdk_11.

pkgs/by-name/ff/ffmpeg-livepeer/package.nix Outdated Show resolved Hide resolved
@Bot-wxt1221 Bot-wxt1221 changed the base branch from master to staging-next November 11, 2024 13:39
@Bot-wxt1221 Bot-wxt1221 force-pushed the livepeer branch 2 times, most recently from 2e67253 to e739e26 Compare November 11, 2024 13:41
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 11, 2024
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

@ofborg build livepeer ffmpeg-livepeer

pkgs/by-name/ff/ffmpeg-livepeer/package.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Nov 12, 2024
@Atemu
Copy link
Member

Atemu commented Nov 12, 2024

@ofborg build livepeer ffmpeg-livepeer

@wegank wegank merged commit 7ae1889 into NixOS:staging-next Nov 13, 2024
30 of 32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.kind: ZHF Fixes Fixes during the Zero Hydra Failures (ZHF) campaign 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants