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

[pcre2] Convert JIT to feature #31294

Merged
merged 1 commit into from
May 11, 2023

Conversation

talregev
Copy link
Contributor

@talregev talregev commented May 6, 2023

Convert JIT to feature

  • 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.

@autoantwort
Copy link
Contributor

It is currently not failing?

@talregev
Copy link
Contributor Author

talregev commented May 6, 2023

It is currently not failing?

vcpkg ci doesn't check arm64-osx triplet.

@autoantwort
Copy link
Contributor

I have tested it locally

@talregev
Copy link
Contributor Author

talregev commented May 6, 2023

I have tested it locally

I tested it on osx on the github action ci. and it fail without this fix.

I also set these:

set(VCPKG_OSX_DEPLOYMENT_TARGET 10.13)
set(VCPKG_C_FLAGS -mmacosx-version-min=10.13)
set(VCPKG_CXX_FLAGS -mmacosx-version-min=10.13)

@autoantwort
Copy link
Contributor

Can you provide the error logs?

@talregev
Copy link
Contributor Author

talregev commented May 6, 2023

Can you provide the error logs?

I will.

@autoantwort
Copy link
Contributor

Ah I found it:

/Users/leanderSchulten/git_projekte/vcpkg2/buildtrees/pcre2/src/cre2-10.40-42fc341438.clean/src/sljit/sljitExecAllocator.c:156:2: error: "Must target Big Sur or newer"
#error "Must target Big Sur or newer"
 ^
1 error generated.

@autoantwort
Copy link
Contributor

But then I suggest to make it a feature instead of turning it off completely because it only works on current macs.

@talregev
Copy link
Contributor Author

talregev commented May 6, 2023

But then I suggest to make it a feature instead of turning it off completely because it only works on current macs.

I will try to convert it to a feature.

@talregev talregev changed the title [pcre2] fix jit off for osx arm64 [pcre2] convert jit to feature May 6, 2023
@talregev talregev changed the title [pcre2] convert jit to feature [pcre2] Convert JIT to feature May 6, 2023
ports/pcre2/vcpkg.json Outdated Show resolved Hide resolved
@talregev talregev force-pushed the TalR/pcre2_arm_osx branch 3 times, most recently from 20c58fc to 4e5f85b Compare May 6, 2023 20:21
@talregev
Copy link
Contributor Author

talregev commented May 6, 2023

@autoantwort Can you test my port and check I didn't make errors?

autoantwort
autoantwort previously approved these changes May 6, 2023
@talregev
Copy link
Contributor Author

talregev commented May 6, 2023

@autoantwort What about the android failing in the ci?

@autoantwort
Copy link
Contributor

I think it is a baseline regression not caused by this PR.

@Osyotr
Copy link
Contributor

Osyotr commented May 6, 2023

It think that until microsoft/vcpkg-tool#813 is merged, default-features should not contain features that have supports expression.
Workaround is to create platform-default-features feature that has platform expression (https://github.com/microsoft/vcpkg/pull/31291/files#diff-fa5764c3245e6fdc2a5bd9ad48405b4e6f2555a4bff9079d365bb4990e3b002a)

@Cheney-W Cheney-W added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 8, 2023
@Cheney-W
Copy link
Contributor

Cheney-W commented May 8, 2023

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Cheney-W
Cheney-W previously approved these changes May 8, 2023
@Cheney-W
Copy link
Contributor

Cheney-W commented May 8, 2023

Pipeline issue came from #31155. I'm trying to fix it.

@talregev
Copy link
Contributor Author

talregev commented May 8, 2023

@Cheney-W
There already fix that approved
#29767

I will merge the master after you will merge it.

@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label May 9, 2023
@dg0yt
Copy link
Contributor

dg0yt commented May 9, 2023

It think that until microsoft/vcpkg-tool#813 is merged, default-features should not contain features that have supports expression. Workaround is to create platform-default-features feature that has platform expression (https://github.com/microsoft/vcpkg/pull/31291/files#diff-fa5764c3245e6fdc2a5bd9ad48405b4e6f2555a4bff9079d365bb4990e3b002a)

This still must be done.

vicroms
vicroms previously requested changes May 9, 2023
ports/pcre2/vcpkg.json Outdated Show resolved Hide resolved
@Cheney-W Cheney-W removed the info:reviewed Pull Request changes follow basic guidelines label May 10, 2023
@Cheney-W Cheney-W marked this pull request as draft May 10, 2023 01:33
@Cheney-W
Copy link
Contributor

I have converted your PR to draft status. When you respond to Victor's suggestion, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

@talregev talregev dismissed stale reviews from autoantwort and Cheney-W via f9a83fc May 10, 2023 10:42
@talregev talregev requested review from vicroms and Cheney-W May 10, 2023 10:42
@talregev talregev marked this pull request as ready for review May 10, 2023 10:43
@talregev
Copy link
Contributor Author

@Cheney-W @vicroms
Ready for review.

@Cheney-W Cheney-W added the info:reviewed Pull Request changes follow basic guidelines label May 11, 2023
@JavierMatosD JavierMatosD merged commit d54600a into microsoft:master May 11, 2023
15 checks passed
@talregev talregev deleted the TalR/pcre2_arm_osx branch May 11, 2023 22:01
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

7 participants