ccextractor icon indicating copy to clipboard operation
ccextractor copied to clipboard

[BUG] ccextractor 0.94 release cannot build with gpac 2.0.0

Open chenrui333 opened this issue 2 years ago • 19 comments

Problem

ccextractor 0.94 release cannot build with gpac 2.0.0 CCExtractor version: 0.94

build error:

/usr/local/Cellar/gpac/2.0.0/include/gpac/version.h:72:1: error: unknown type name 'u32'
u32 gf_gpac_abi_major();
^
/usr/local/Cellar/gpac/2.0.0/include/gpac/version.h:76:1: error: unknown type name 'u32'
u32 gf_gpac_abi_minor();
^
/usr/local/Cellar/gpac/2.0.0/include/gpac/version.h:80:1: error: unknown type name 'u32'
u32 gf_gpac_abi_micro();
^
3 errors generated.

In raising this issue, I confirm the following:

  • [x] I have read and understood the contributors guide.
  • [x] I have checked that the bug-fix I am reporting can be replicated, or that the feature I am suggesting isn't already present.
  • [x] I have checked that the issue I'm posting isn't already reported.
  • [x] I have checked that the issue I'm porting isn't already solved and no duplicates exist in closed issues and in opened issues
  • [ ] I have checked the pull requests tab for existing solutions/implementations to my issue/suggestion.
  • [ ] I have used the latest available version of CCExtractor to verify this issue exists.
  • [ ] I have ticked all the boxes in this section and to prove it I'm deleting the section completely to remove boilerplate text.

Necessary information

  • Is this a regression (i.e. did it work before)? Yes
  • What platform did you use? MacOS

Additional information

  • relates to https://github.com/Homebrew/homebrew-core/pull/97413

chenrui333 avatar Mar 21 '22 23:03 chenrui333

I fail to see how a gpac error is related to this project (aside from it being a dependency of CCExtractor)?

canihavesomecoffee avatar Mar 22 '22 06:03 canihavesomecoffee

Hi 👋 another Homebrew maintainer here. I understand that CCExtractor vendors its own dependencies (e.g. for gpac, at https://github.com/CCExtractor/ccextractor/tree/master/src/thirdparty/gpacmp4) and that we as packagers are going beyond those boundaries to ship CCExtractor with external dependencies linked rather than baked in.

I was wondering if someone has tried building CCExtractor with the new gpac release to see if it is supposed to work, and whether there's a plan to ship a newer version of gpac with CCExtractor (and make whatever changes may be needed in CCExtractor itself to support that).

We carry patches downstream to use our own gpac when building CCExtractor; this seems to be an error with the compilation unit in CCExtractor that includes the gpac headers not declaring the type u32 (I guess this wasn't needed in the current gpac 1.0.1). So long as we're carrying the patch downstream, we may as well bear the responsibility of adding the right includes/CFLAGS to make it work (the patch is already enormous anyways).

alebcay avatar May 17 '22 16:05 alebcay

@alebcay Why do you go out of your way to "unvendor"? What do you get from it other that doing a lot of work and possibly breaking stuff? :-)

cfsmp3 avatar May 17 '22 16:05 cfsmp3

One example is generally upstream fixes for uncommon platforms. For instance, CCExtractor failed to build on macOS for Apple Silicon machines for the longest time because the bundled libpng was failing to build on the new ARM64 architecture.

The folks at libpng fixed it pretty quickly, because someone reported the issue to them. We proceeded to pick up the new version and were shipping it without issue. Meanwhile, CCExtractor builds (outside of Homebrew) remained broken on Apple Silicon machines until it finally started using the new libpng version.

In other words - as an upstream, your maintenance burden (especially for platform support like the Apple Silicon scenario) greatly increases when you vendor your dependencies. As a downstream, we're basically picking up the burden now since we own the patch.

What do we get out of it? In situations like what I described above, we get a successful build + working application at all on certain architectures.

alebcay avatar May 17 '22 17:05 alebcay

In any case, I don't expect my rationale to sway the direction of the CCExtractor project (I'm sure you have valid reasons for doing things the way you do). Similarly, I don't expect the Homebrew project direction to really change either from any discussion in this thread.

I'd like to point out that Homebrew isn't unique in this policy of unvendoring. For instance, Debian and Termux also carry patches to force the use of system libs when packaging CCExtractor. There's also a handful more like FreeBSD Ports and Parabola that have opted to not update past 0.85 because that's when some of the flexibility in the CMake script was removed.

Thus my focus remains on the possibility of gpac 2.0.0 support in CCExtractor.

alebcay avatar May 17 '22 17:05 alebcay

In any case, I don't expect my rationale to sway the direction of the CCExtractor project (I'm sure you have valid reasons for doing things the way you do).

Windows.

I'd like to point out that Homebrew isn't unique in this policy of unvendoring. For instance, Debian and [Termux

Debian specifically wants to fight the world with fewer soldiers that are needed :-) A lost battle with Rust, by the way.

Anyway we're happy to take patches of course, but I don't see ourselves not vendoring unless someone solves the DLL nightmare for us.

cfsmp3 avatar May 25 '22 22:05 cfsmp3

but I don't see ourselves not vendoring unless someone solves the DLL nightmare for us.

Yeah, I understand the Windows situation. Again, not asking to stop vendoring but at least offer build-time flags to not use the vendored libraries. I realize that even just providing that kind of flexibility requires additional complexity in the build system.

For what it's worth, the Debian folks seem to have a patch for building ccextractor with GPAC 2.0.0. Apart from dropping in a new vendored version, looks like the required change in the ccextractor source is just a one-liner. I can have a go at contributing the relevant changes if there's interest.

alebcay avatar May 26 '22 12:05 alebcay

We always welcome PR's :)

Getting the correct DLL's for the Windows build might be a bit tougher, but with the automated builds that should be easy enough to be able to quickly test out.

canihavesomecoffee avatar May 26 '22 16:05 canihavesomecoffee

Why do you go out of your way to "unvendor"? What do you get from it other that doing a lot of work and possibly breaking stuff? :-)

Your bundled version of GPAC is currently vulnerable to at least 50 CVEs and nobody appears aware of this or motivated to resolve the problem.

https://www.cvedetails.com/vulnerability-list/vendor_id-18664/product_id-47755/Gpac-Gpac.html

When you vendor a library you also become responsible for its security maintenance. Using a "system" version of the library frees you from this responsibility.

From NixOS' point of view, our choice is either to "unvendor" the package or to mark it with all your GPAC version's knownVulerabilities, making it uninstallable by default.

FWIW even GPAC upstream appears to be a bit of a security catastrophe, if it's simple to switch it for a different dependency it might be worth consideration.

risicle avatar Jan 28 '23 17:01 risicle

From NixOS' point of view, our choice is either to "unvendor" the package or to mark it with all your GPAC version's knownVulerabilities, making it uninstallable by default.

CCExtractor needs to build also on Windows and OSX. Windows specifically is a nightmare without vendoring. Or at least it used to be, it's possible things are different now.

Our reality is that most of our users run Windows, even if us developers run Linux. We already go out of our way to build on Windows even if we personally don't use it...

FWIW even GPAC upstream appears to be a bit of a security catastrophe, if it's simple to switch it for a different dependency it might be worth consideration.

I suspect GPAC considers their real life usage to be "safe because the input stream comes from trustworthy places". I don't know, to be honest.

cfsmp3 avatar Jan 29 '23 18:01 cfsmp3

I get the impression the GPAC folks are from a telephony/streaming background so I'd hope they weren't just assuming trusted input. I think what's happened is they've only recently hooked it up to OSS-Fuzz and they're now getting a decade's worth of accumulated flaws to address. Either way, it's quite hard at the moment to maintain a version that's both stable and secure.

Being able to offer windows users a bundled version doesn't have to be mutually exclusive from being able to accommodate/understand that many distributors have a need to minimize vendored & outdated dependencies.

From a distributors point of view, being pragmatic and understanding what assumptions different projects make about trusted inputs etc is both expensive (in terms of effort) and potentially dangerous. Dangerous not just because it could lead to one of us making an overly-lax judgement and exposing users to a vulnerability, but also because it leaves us open to accusations from those in the community who don't have the time to evaluate the nuance of the situation.

So this is the situation that drives distributors in the direction of being rather "absolutist" on security vulnerabilities. It's the simplest way (perhaps the only practical way) to handle the problem - to eliminate as many known CVEs as sensibly possible, wherever they're found.

risicle avatar Jan 29 '23 21:01 risicle

Being able to offer windows users a bundled version doesn't have to be mutually exclusive from being able to accommodate/understand that many distributors have a need to minimize vendored & outdated dependencies.

We'll be happy to merge PRs :-)

cfsmp3 avatar Jan 30 '23 19:01 cfsmp3

Didn't get to updating this thread earlier, but I had a go at replacing the vendored GPAC in this source tree. While the API is a one-line change from 1.x to 2.x, replacing the vendored copy here and getting it into the expected directory layout is a huge diff and wouldn't compile so I gave up.

In any case, the Homebrew formula (package) is now disabled since we are now packaging gpac 2.x. I continue to maintain a copy of the ccextractor formula at https://github.com/alebcay/homebrew-ccextractor but it's on a best-effort basis from myself now (and with no CI set up yet).

alebcay avatar Jan 30 '23 19:01 alebcay

Didn't get to updating this thread earlier, but I had a go at replacing the vendored GPAC in this source tree. While the API is a one-line change from 1.x to 2.x, replacing the vendored copy here and getting it into the expected directory layout is a huge diff and wouldn't compile so I gave up.

We added it as a takehome task for GSoC '23 applicants.

Hopefully someone will tackle it soon.

cfsmp3 avatar Jan 30 '23 20:01 cfsmp3

Here is the pull request for it: https://github.com/CCExtractor/ccextractor/pull/1526

sachinrao8312 avatar Apr 03 '23 16:04 sachinrao8312

Here is the pull request for it: #1526

Windows build doesn't work... can you update your PR?

cfsmp3 avatar Apr 03 '23 16:04 cfsmp3

@prateekmedia in case you missed this, this issue might be helpful for your GPAC efforts.

cfsmp3 avatar May 22 '23 16:05 cfsmp3

This one is already resolved in my PR, I think it was due to some setup.h or config.h missing header of the file

prateekmedia avatar May 22 '23 16:05 prateekmedia

#1546 and #1535 fixes this

prateekmedia avatar Jul 15 '23 10:07 prateekmedia