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

make libfdk-aac opt-in to avoid license issues #341

Merged
merged 3 commits into from
Sep 29, 2023
Merged

Conversation

grusell
Copy link
Contributor

@grusell grusell commented Sep 29, 2023

No description provided.

Signed-off-by: Gustav Grusell <gustav.grusell@eyevinn.se>
Dockerfile Outdated
@@ -770,8 +772,8 @@ RUN \
--disable-ffplay \
--enable-static \
--enable-gpl \
--enable-version3 \
--enable-nonfree \
--enable-version3 \
Copy link
Owner

Choose a reason for hiding this comment

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

weird indent? i guess we use auto formatters with different opinions? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah missed that, will fix.

@wader
Copy link
Owner

wader commented Sep 29, 2023

Thanks! i was thinking that maybe the CI PR build should build with --build-arg ENABLE_FDKAAC=1 to make sure it's not bit rotting?

@wader
Copy link
Owner

wader commented Sep 29, 2023

Maybe not a big deal but with this we will still always download/build fdkaac. I've done various experiments before to make all libs optional, and have a default to build al., But it's tricky do to in a way that you don't end up with ex frankenstein Dockerfile that is hard to have a overview of, i really like how clean and simple the current one is.

@grusell
Copy link
Contributor Author

grusell commented Sep 29, 2023

I agree it could make sense to build with libfdk-aac enabled in the CI to verify that that works. Do you want me to update this in the PR?

Yes, I thought it wasn't worth it to complicate things by making the actual build of the libfdk-aac opt-in, I assume it won't add much to the build time anyway.

@wader
Copy link
Owner

wader commented Sep 29, 2023

I agree it could make sense to build with libfdk-aac enabled in the CI to verify that that works. Do you want me to update this in the PR?

Yeap do that

Yes, I thought it wasn't worth it to complicate things by making the actual build of the libfdk-aac opt-in, I assume it won't add much to the build time anyway.

Agree, let's keep it simple for now and we can fix in later if something more generic is done

Signed-off-by: Gustav Grusell <gustav.grusell@eyevinn.se>
Signed-off-by: Gustav Grusell <gustav.grusell@eyevinn.se>
@grusell
Copy link
Contributor Author

grusell commented Sep 29, 2023

PR updated as discussed.

@wader
Copy link
Owner

wader commented Sep 29, 2023

Looks good. Ready to merge?

@wader wader merged commit b7dd7b6 into wader:master Sep 29, 2023
1 check passed
@wader
Copy link
Owner

wader commented Sep 29, 2023

Tack!

@wader wader mentioned this pull request Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants