Skip to content

[ffmpeg] no longer hardcode version strings in FindFFMPEG script#17236

Merged
strega-nil merged 9 commits intomicrosoft:masterfrom
mcmtroffaes:feature/ffmpeg-version-regex
Apr 14, 2021
Merged

[ffmpeg] no longer hardcode version strings in FindFFMPEG script#17236
strega-nil merged 9 commits intomicrosoft:masterfrom
mcmtroffaes:feature/ffmpeg-version-regex

Conversation

@mcmtroffaes
Copy link
Contributor

Describe the pull request

  • What does your PR fix? The version of ffmpeg and the versions of its components are currently hardcoded in the FindFFMPEG script. This patch instead extracts the versions from the source files, ensuring that they do not need to be updated by hand, and also ensuring that --head builds have the correct version. I've manually verified that versions in the installed FindFFMPEG script are identical before and after patch.

  • Which triplets are supported/not supported? Have you updated the CI baseline? No changes.

  • Does your PR follow the maintainer guide? Yes, to the best of my knowledge.

cc @genevanmeter (see #16882 (comment))

@genevanmeter
Copy link
Contributor

Thanks this is a proper solution.

@mcmtroffaes
Copy link
Contributor Author

Great, many thanks for giving it a look and for the quick response!

Comment on lines +721 to +728
extract_version_from_component(COMPONENT libavutil)
extract_version_from_component(COMPONENT libavcodec)
extract_version_from_component(COMPONENT libavdevice)
extract_version_from_component(COMPONENT libavfilter)
extract_version_from_component(COMPONENT libavformat)
extract_version_from_component(COMPONENT libavresample)
extract_version_from_component(COMPONENT libswresample)
extract_version_from_component(COMPONENT libswscale)
Copy link
Member

Choose a reason for hiding this comment

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

Just to be consistent with other functions in vcpkg I would make the output variable explicit.

extract_version_from_component(OUT_VARIABLE LIBAVUTIL_VERSION COMPONENT libavutil)

However the current design is convenient and since this is a one of for this port I'm not against merging as is.
Pinging @strega-nil for review of CMake guidelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I completely agree it is best practice to make the output variable explicit. I guess the function could be made more generic to work for other ports too, but for the moment I'll update it as per your suggestion. Let me add a commit to this effect.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the suggested changes in 754423c

@JackBoosY JackBoosY added the category:port-bug The issue is with a library, which is something the port should already support label Apr 13, 2021
@mcmtroffaes mcmtroffaes marked this pull request as draft April 13, 2021 08:10
@mcmtroffaes mcmtroffaes marked this pull request as ready for review April 13, 2021 08:21
Copy link
Contributor

@strega-nil strega-nil left a comment

Choose a reason for hiding this comment

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

I really like this, but I've made changes to the cmake functions to be closer to how we want them to be written in the scripts tree (not necessary, but I thought it'd be good anyways)

@JackBoosY
Copy link
Contributor

CMake Error at ports/ffmpeg/portfile.cmake:682 (file):
  file failed to open for reading (No such file or directory):

    C:/a/1/s/
Call Stack (most recent call first):
  ports/ffmpeg/portfile.cmake:712 (extract_regex_from_file)
  scripts/ports.cmake:142 (include)

@mcmtroffaes
Copy link
Contributor Author

Thanks @strega-nil LGTM! I'll see if I can reproduce that regression failure.

@mcmtroffaes
Copy link
Contributor Author

I've fixed a few small typos, it passes locally for me, should be hopefully good to go now.

@JackBoosY
Copy link
Contributor

depends on #17277

@autoantwort autoantwort mentioned this pull request Apr 14, 2021
@strega-nil
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil strega-nil merged commit b1e352c into microsoft:master Apr 14, 2021
@mcmtroffaes mcmtroffaes deleted the feature/ffmpeg-version-regex branch April 14, 2021 21:37
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants