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] Update to 6.0 #30135

Merged
merged 21 commits into from
Sep 26, 2023
Merged

[ffmpeg] Update to 6.0 #30135

merged 21 commits into from
Sep 26, 2023

Conversation

Sibras
Copy link
Contributor

@Sibras Sibras commented Mar 11, 2023

[ffnvcodec] Update to 11.1.5.2
[mfx-dispatch] Fix pkgconfig
[[freerdp] Update to 2.11.1]

@Sibras
Copy link
Contributor Author

Sibras commented Mar 12, 2023

Unfortunately all the CI failures are due to 'ignition-common3'. There doesnt seem to be a fix for this as all the updated versions of the formally called ignition libraries are now called gazebo (see https://github.com/microsoft/vcpkg/pull/28656/files). So until the old ignition ports are removed and replaced with the new gazebo ones (and hopefully remove the annoying multiple different versions of the ignition ports) then this PR is blocked.

@Adela0814 Adela0814 added the category:port-update The issue is with a library, which is requesting update new revision label Mar 13, 2023
@Adela0814
Copy link
Contributor

Convert this PR to draft since there is no progress. Please ping us if this PR is ready for review again.

@Adela0814 Adela0814 marked this pull request as draft March 16, 2023 10:04
@Adela0814
Copy link
Contributor

Closing this PR since it seems that no progress is being made. Please ping us to reopen if work is still being done.

@Adela0814 Adela0814 closed this Mar 24, 2023
@talregev
Copy link
Contributor

@Adela0814 You should open this PR and add the label depend on other PR, because there is a PR that block this PR:
#28656

@Adela0814 Adela0814 added the depends:different-pr This PR or Issue depends on a PR which has been filed label Mar 29, 2023
@Adela0814 Adela0814 reopened this Mar 29, 2023
@reitowo
Copy link
Contributor

reitowo commented Apr 11, 2023

Any progress?

@talregev
Copy link
Contributor

Any progress?

This PR is block by my PR that is already long time on vcpkg team review.

@reitowo
Copy link
Contributor

reitowo commented Apr 11, 2023

We should really think about separating major version to ffmpeg6 and avoid this blocking. It is frustrating to wait for team review.

@talregev
Copy link
Contributor

We should really think about separating major version to ffmpeg6 and avoid this blocking. It is frustrating to wait for team review.

Please review my PR and comment there: #28656

@Neumann-A
Copy link
Contributor

Is there some migration guide from ffmpeg 4 -> 5 or 5->6. ffmpeg removed a bunch of stuff and made it private and I am wondering if there official migration guides somewhere

@BillyONeal
Copy link
Member

See discussion from @vicroms, @JavierMatosD, @dan-shaw, and I over in #28656 (review) . Repeating the important parts here:

[...] upstream no longer cares about maintaining ignition-common3, and continuing to index that in the curated registry potentially blocks other unrelated updates like ffmpeg6 #30135

Potential outcomes:

  • No, the vcpkg port names use ignition- and we should pick only one despite side-by-side support upstream.
  • Delist the older ignition ports.
  • Baseline older ignition ports.
  • Maintain patches forever.

Result:

Speaking personally I think we should delist the ignition- names in this PR.

@traversaro
Copy link
Contributor

traversaro commented Apr 28, 2023

Contact @traversaro for opinions on the above potential outcomes because they are the original contributor of the ignition ports.

Thanks for mentioning me!

Click here for an OT explanation on why I am not updating anymore ignition-* packages on vcpkg that I originally contributed to.

First of all, just a bit of background on why I am not updating anymore the ignition-/gz- packages on vcpkg: as mentioned in #24630 (comment), I worked originally on bringing ignition-* libraries to vcpkg to have a clean way of installing them on Windows.

However, in the scientific computing context we need a package manager that can install:

  1. C/C++ libraries
  2. Program/tools written in C++, that depend on C/C++ libraries (1.)
  3. Python packages, that depend on on C/C++ libraries (1.)

Over time, I understood that the vcpkg goal is only to support the packaging of 1. (I remember a really clear in-depth explanation on this by @BillyONeal on some issue/discussion, but I can't find it at the moment), so I moved our packaging efforts to other packages managers that support 1., 2. and 3. (in our case conda/conda-forge, but for example also spack targets 1., 2. and 3. .

Back when I contributed ignition-* ports to vcpkg, the idea for packaging them as a different port for each major version was the following:

  • Permit to install several ignition libraries side-by-side, as explicitly permitted by their structure and how it is possible in other package managers that package them (such as apt, homebrew, conda-forge)
  • Clearly, that does not mean that older version needed to be contained in vcpkg forever: Ignition/Gazebo libraries have a clear support policy window, documented in https://github.com/gazebosim/docs/blob/master/tools/versions.md . My idea was that it could make sense to keep a given version in vcpkg just as long as upstream was supporting it (in the case of ign-common3, the EOL date is 2025-01-25), and then it could make sense to remove it.

Clearly, this was just my idea back when I was actively working in mantaining ignition-* ports in vcpkg. Now, I totally understand that it could make sense to delist ignition-* even before their EOL data ports if nobody is willing to invest time to actually mantain them.

However, just for this specific case of ffmpeg6, ign-common3 has gained support for ffmpeg6 in gazebosim/gz-common#504, so if someone is willing to mantain the ign-common3 in vcpkg, it is possible to either use that patch or wait for a new release of ign-common3 .

cc a few people that may (or may not, I am not sure) be interested in this discussion: @j-rivero @Ace314159 @ahoarau @autoantwort

@Neumann-A
Copy link
Contributor

Over time, I understood that the vcpkg goal is only to support the packaging of 1.

I think 2. is currently on @BillyONeal ToDo list.

  1. really needs a solution. How does condo/spack solve that problem on windows? I probably just need to see an expanded directory tree to figure out how to apply that for vcpkg.

@traversaro
Copy link
Contributor

  1. really needs a solution. How does condo/spack solve that problem on windows? I probably just need to see an expanded directory tree to figure out how to apply that for vcpkg.

Probably a bit OT, but basically both python and each Python package on conda-forge and spack are separate packages, and then python is patched/configured to look for packages in a given directory in the install path.

@autoantwort
Copy link
Contributor

Backport gazebosim/gz-common#504 to the ignition-common3 version packaged in vcpkg

I have created Sibras#1

@reitowo
Copy link
Contributor

reitowo commented Sep 17, 2023

I suggest all ports that have breaking changes across major version number should have seperate ports as vcpkg is not able to lock to specific version well, it is also not easy to maintain LTS of previous major version of the same port. It is funny to expect a 6 month delay/block when ffmpeg 7 rolls out.

@Sibras
Copy link
Contributor Author

Sibras commented Sep 17, 2023

Backport gazebosim/gz-common#504 to the ignition-common3 version packaged in vcpkg

I have created Sibras#1

That fixed it, CI passes now

@Sibras Sibras marked this pull request as ready for review September 17, 2023 14:31
@talregev
Copy link
Contributor

@Adela0814 Please review it again

@talregev
Copy link
Contributor

@Osyotr

@talregev
Copy link
Contributor

@Sibras I updated ignition-common3 ports.
There is no need to update it here aka changes with the patch.
There are conflicts on this PR also because of that.

@Adela0814
Copy link
Contributor

@Sibras Please fix the conflicts.

@Sibras
Copy link
Contributor Author

Sibras commented Sep 23, 2023

@Sibras Please fix the conflicts.

Done

@talregev
Copy link
Contributor

@Adela0814
Ready for review

@Adela0814 Adela0814 added requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function info:reviewed Pull Request changes follow basic guidelines and removed requires:all-feature-testing vcpkg install port[all features supported by that port] needs to be demonstrated to function labels Sep 25, 2023
Adela0814
Adela0814 previously approved these changes Sep 25, 2023
@Adela0814
Copy link
Contributor

All feature tested on x64-windows.

@vicroms
Copy link
Member

vicroms commented Sep 26, 2023

Sorry! could you resolve the merge conflicts again? I tried to solve the conflicts and push the resolution to your branch, but I don't seem to have permissions to push the changes.

@Sibras
Copy link
Contributor Author

Sibras commented Sep 26, 2023

Sorry! could you resolve the merge conflicts again? I tried to solve the conflicts and push the resolution to your branch, but I don't seem to have permissions to push the changes.

Done

@vicroms vicroms merged commit 1b4d69f into microsoft:master Sep 26, 2023
15 checks passed
@Sibras Sibras deleted the ffmpeg branch April 6, 2024 09:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.