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

[libmupdf] add ocr feature #32084

Merged
merged 7 commits into from
Jul 5, 2023
Merged

Conversation

simon987
Copy link
Contributor

New libmupdf versions have optional support for OCR with leptonica and tesseract. In the upstream project, it is enabled when both libraries are available, using the OCR_DISABLED flag:

https://github.com/ArtifexSoftware/mupdf/blob/c5d3b65c8a3e1923cd816e8cb0eed726937a289d/include/mupdf/fitz/config.h#L216-L220

I added the feature in ports/libmupdf/vcpkg.json and updated the ports/libmupdf/CMakeLists.txt file. Changing OCR_FEATURE_IS_ENABLED from 0 to 1 does work on my machine but I can't figure out how to "link" this variable with the [ocr] feature flag (is there a is_feature_enabled() function or similar?).

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@FrankXie05 FrankXie05 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jun 19, 2023
@dg0yt
Copy link
Contributor

dg0yt commented Jun 19, 2023

Changing OCR_FEATURE_IS_ENABLED from 0 to 1 does work on my machine but I can't figure out how to "link" this variable with the [ocr] feature flag (is there a is_feature_enabled() function or similar?).

This port is unusual, it has the usual portfile (CMake script mode) and it adds a CMake build system to the upstream sources (CMake project mode). Keep this in mind. Cf. https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#portfiles-are-run-in-script-mode

In script mode, the features are available in variable FEATURES. So the portfile can react in various ways. In particular, it needs to feed settings into the build system. For CMake build systems, there is vcpkg_check_features to support the common case of setting "input variables".

So, to the CMakeLists.txt, you could add:

option(ENABLE_OCR "Build with OCR" ON)

In the portfile you could use the maintainer functions:

vcpkg_check_features(
    OUT_FEATURE_OPTIONS FEATURE_OPTIONS
    FEATURES
        ocr ENABLE_OCR
)
...
vcpkg_cmake_configure(
    ...
    OPTIONS
        ${FEATURE_OPTIONS}
)

@simon987 simon987 changed the title [libmupdf] add ocr feature (help needed) [libmupdf] add ocr feature Jun 19, 2023
@simon987
Copy link
Contributor Author

Thanks a lot @dg0yt!

@simon987 simon987 marked this pull request as ready for review June 19, 2023 22:26
FrankXie05
FrankXie05 previously approved these changes Jun 28, 2023
@FrankXie05 FrankXie05 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 Jun 28, 2023
@FrankXie05
Copy link
Contributor

All features are tested successfully in the following triplet:

  • x86-windows
  • x64-windows
  • x64-windows-stataic

"git-tree": "c1684ff15eed2c86ba792de4e92d21b2ea58c188",
"git-tree": "d4ef30a15c2854debf3972dab602df82eb736ee7",
Copy link
Member

@vicroms vicroms Jul 1, 2023

Choose a reason for hiding this comment

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

Need to increase the port-version in vcpkg.json and then run vcpkg x-add-version to correctly update the versions database.

Please mark PR as "Ready for review" once you have fixed this.

@vicroms vicroms marked this pull request as draft July 1, 2023 00:44
@simon987
Copy link
Contributor Author

simon987 commented Jul 1, 2023

@vicroms I was told to delete the port-version field the last time :( I tried to re-add it at version 2 but I think that broke the CI, could you help me with that?

@simon987 simon987 marked this pull request as ready for review July 1, 2023 13:37
@dg0yt
Copy link
Contributor

dg0yt commented Jul 1, 2023

I was told to delete the port-version field the last time :( I tried to re-add it at version 2 but I think that broke the CI, could you help me with that?

port-version is relative to version[-...]. If you bump the version, the port version is reset. If the version is unchanged, the port-version must be incremented by one.

@@ -1,12 +1,17 @@
{
"name": "libmupdf",
"version": "1.22.1",
"port-version": 2,
Copy link
Contributor

Choose a reason for hiding this comment

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

This jumps from (implicit) 0 to 2. But we need 1 now.

ports/mongoose/portfile.cmake Outdated Show resolved Hide resolved
@simon987
Copy link
Contributor Author

simon987 commented Jul 1, 2023

 port-version is relative to version[-...]. If you bump the version, the port version is reset. If the version is unchanged, the port-version must be incremented by one.

I see, this makes sense now. Thank you!

versions/l-/libmupdf.json Outdated Show resolved Hide resolved
@vicroms vicroms merged commit d06a922 into microsoft:master Jul 5, 2023
15 checks passed
@simon987 simon987 deleted the libmupdf-add-ocr branch July 6, 2023 22:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants