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] Block "tensorflow" and others for the "all" feature on non-x64. #22984

Merged
merged 2 commits into from
Feb 8, 2022

Conversation

BillyONeal
Copy link
Member

In #21980 , we removed all the blocks for tensorflow which were merely replicating that upstream's "supports" expression. That is the correct behavior: if upstream adds support for a thing, it should start being tested downstream. However, the "all" and "all-nonfree" features of ffmpeg attempt to turn on what is really "all supported" rather than "all", so the feature-dependency needs to be guarded.

Note that the ffmpeg[tensorflow] feature remains unguarded! It is ffmpeg[all]'s dependency on ffmpeg[tensorflow] that is guarded.

In microsoft#21980 , we removed all the blocks for tensorflow which were merely replicating that upstream's "supports" expression. That is the correct behavior: if upstream adds support for a thing, it should start being tested downstream. However, the "all" and "all-nonfree" features of ffmpeg attempt to turn on what is really "all supported" rather than "all", so the feature-dependency needs to be guarded.

Note that the ffmpeg[tensorflow] feature remains unguarded! It is ffmpeg[all]'s *dependency* on ffmpeg[tensorflow] that is guarded.
@BillyONeal BillyONeal added category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. labels Feb 7, 2022
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/ffmpeg/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

@cenit
Copy link
Contributor

cenit commented Feb 7, 2022

what about converting control file of vcpkg-ci-ffmpeg at the same time in this PR?

@cenit
Copy link
Contributor

cenit commented Feb 7, 2022

also, very important,
vcpkg-ci-ffmpeg is STILL a cascade failure in these triplets:

x64_uwp
arm64_windows
arm_uwp

@Sibras
Copy link
Contributor

Sibras commented Feb 8, 2022

The patch should actually be updated to be
!(x86 | arm | uwp) & !static

Not sure why it wasnt kept at that to begin with

@JackBoosY
Copy link
Contributor

Please note that due to the association between ci-opencv and ci-ffmpeg, we can only solve the problem by de-testing the feature tensorflow in ci-ffmpeg.

@BillyONeal
Copy link
Member Author

The patch should actually be updated to be !(x86 | arm | uwp) & !static

Tensorflow itself phrases it in the positive ("only x64 is supported"), not "these architectures are not supported".

Not sure why it wasnt kept at that to begin with

Because we shouldn't be needing to replicate the "supports" results for every downstream port in the tree.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

You have modified or added at least one vcpkg.json where a "license" field is missing.
If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/ffmpeg/vcpkg.json

Valid values for the license field are listed at https://spdx.org/licenses/

@BillyONeal
Copy link
Member Author

vcpkg-ci-ffmpeg:arm64-windows is still unhappy but the others are fixed; I think this is good enough to ensure CI is testing something. I still think overall we should be having a validation step that fails the CI run if one of these test ports cascades.

@BillyONeal BillyONeal changed the title [ffmpeg] Block "tensorflow" for the "all" feature on non-x64. [ffmpeg] Block "tensorflow" and others for the "all" feature on non-x64. Feb 8, 2022
Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

This LGTM, but shouldn't fixing arm64 be as simple as modifying these platform clauses a bit more?

@BillyONeal
Copy link
Member Author

This LGTM, but shouldn't fixing arm64 be as simple as modifying these platform clauses a bit more?

The issue is that it's hard for me to test because arm64 isn't working for anything on my machine, and I already spent like 4 hours trying to figure out why and can't really afford to invest further here.

@BillyONeal BillyONeal merged commit 439bf40 into microsoft:master Feb 8, 2022
@BillyONeal BillyONeal deleted the ffmpeg branch February 8, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants