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

[pcre] fix build pcre undeclared identifier strtoq on arm-android #24757

Merged
merged 3 commits into from
May 19, 2022
Merged

[pcre] fix build pcre undeclared identifier strtoq on arm-android #24757

merged 3 commits into from
May 19, 2022

Conversation

tqcq
Copy link
Contributor

@tqcq tqcq commented May 17, 2022

  • replace CHECK_FUNCTION_EXISTS by CHECK_SYMBOL_EXISTS

Describe the pull request

  • What does your PR fix?

    • fix build pcre on arm-android error. (replace CheckFunctionExists by CheckSymbolExists.)
    • #17600
  • Which triplets are supported/not supported? Have you updated the CI baseline?

    unchanged, No

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

* replace CHECK_FUNCTION_EXISTS by CHECK_SYMBOL_EXISTS
@ghost
Copy link

ghost commented May 17, 2022

CLA assistant check
All CLA requirements met.

@tqcq
Copy link
Contributor Author

tqcq commented May 18, 2022

Hello @BillyONeal , I am trying to fix a pcre build problem, can you help me review these codes.

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 you should check the license field.

If you feel able to do so, please consider adding a "license" field to the following files:

  • ports/pcre/vcpkg.json

Valid values for the license field can be found in the documentation

@Adela0814 Adela0814 added the category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. label May 18, 2022
@BillyONeal
Copy link
Member

Has this patch been submitted upstream? (Can you point to the submission?)

Otherwise this looks good to me.

@tqcq
Copy link
Contributor Author

tqcq commented May 19, 2022

Has this patch been submitted upstream?

This is just a patch for version 8.45. It was not submitted upstream.

The older, but still widely deployed PCRE library, originally released in 1997, is at version 8.45. This version of PCRE is now at end of life, and is no longer being actively maintained. Version 8.45 is expected to be the final release of the older PCRE library, and new projects should use PCRE2 instead. (Reference https://www.pcre.org/)

@tqcq
Copy link
Contributor Author

tqcq commented May 19, 2022

Can you point to the submission?)

CHECK_FUNCTION_EXISTS: symbol links (defined)
CHECK_SYMBOL_EXISTS: symbol available (declared and defined)

Therefore, CHECK_SYMBOL_EXISTS should be used here to ensure that the symbol exists

@BillyONeal
Copy link
Member

Therefore, CHECK_SYMBOL_EXISTS should be used here to ensure that the symbol exists

I'm not saying it's a bad patch, but when it's a patch that isn't specific to something vcpkg does we generally try to make sure upstream knows about it.

(1) This means that when they issue a new version they may have this fixed which allows us to remove the patch.
(2) This ensures that we don't apply patches upstream doesn't disagree with.

I'm less worried about (2) given that this is a minor build system detection change which generally isn't going to change the outcome.

@BillyONeal
Copy link
Member

Ah, I see, upstream says there will never be another release in this series so notifying them about a build system fix is largely irrelevant, OK.

Maybe we should consider updating the port to 10.x but that's of course unrelated to this change.

@BillyONeal BillyONeal merged commit f27af04 into microsoft:master May 19, 2022
@BillyONeal
Copy link
Member

Thanks for the fix!

@Neumann-A
Copy link
Contributor

Maybe we should consider updating the port to 10.x but that's of course unrelated to this change.

there is already port pcre2 which is 10.x

@Adela0814 Adela0814 added the info:reviewed Pull Request changes follow basic guidelines label May 19, 2022
@tqcq tqcq deleted the p/tqcq/fix_pcre_build_on_arm-android branch May 19, 2022 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:community-triplet A PR or issue related to community triplets not officially validated by the vcpkg team. info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants