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.

Sign up for GitHub

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

Jump to bottom

[c4core, ryml] Add windows arm/arm64 support #18316

Merged
merged 2 commits into from Jul 16, 2021
Merged

[c4core, ryml] Add windows arm/arm64 support #18316

merged 2 commits into from Jul 16, 2021

Conversation

ysc3839
Copy link
Contributor

@ysc3839 ysc3839 commented Jun 7, 2021 •
edited

Describe the pull request

  • What does your PR fix?

    Add arm/arm64 support for c4core and rapidyaml.

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

    windows x86, x64, arm, arm64. Yes

  • Does your PR follow the maintainer guide?

    I think 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/

@ysc3839 ysc3839 mentioned this pull request Jun 7, 2021
Closed
@NancyLi1013 NancyLi1013 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jun 8, 2021
NancyLi1013
NancyLi1013 suggested changes Jun 8, 2021
View reviewed changes
ports/c4core/CONTROL Outdated Show resolved Hide resolved
ports/c4core/portfile.cmake Show resolved Hide resolved
scripts/ci.baseline.txt Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

Hi @ysc3839
Thanks for your PR.
This PR also includes the version update. Related PR #18230. I have a question why do you not use the latest commit?

@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 8, 2021

@NancyLi1013 Sorry, I didn't noticed PR #18230. I use this commit because latest rapidyaml references this commit as submodule. https://github.com/biojppm/rapidyaml/tree/master/ext

@NancyLi1013
Copy link
Contributor

Thanks for your clarification. @ysc3839

The failures on CI pipeline are like this, can you look into this?

D:\buildtrees\ryml\src\7de9381706-233508cf9e.clean\src\c4/yml/tree.hpp(675): error C2039: 'contains': is not a member of 'c4::basic_substring<char>'
D:\installed\x86-windows\include\c4/substr_fwd.hpp(11): note: see declaration of 'c4::basic_substring<char>'
D:\buildtrees\ryml\src\7de9381706-233508cf9e.clean\src\c4/yml/tree.hpp(777): error C2039: 'contains': is not a member of 'c4::basic_substring<char>'
D:\installed\x86-windows\include\c4/substr_fwd.hpp(11): note: see declaration of 'c4::basic_substring<char>'
D:\buildtrees\ryml\src\7de9381706-233508cf9e.clean\src\c4/yml/tree.hpp(778): error C2039: 'contains': is not a member of 'c4::basic_substring<char>'
D:\installed\x86-windows\include\c4/substr_fwd.hpp(11): note: see declaration of 'c4::basic_substring<char>'
D:\buildtrees\ryml\src\7de9381706-233508cf9e.clean\src\c4/yml/tree.hpp(782): error C2039: 'contains': is not a member of 'c4::basic_substring<char>'

@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 9, 2021

@NancyLi1013 rapidyaml needs update to latest version. I created another PR #18317.

@NancyLi1013
Copy link
Contributor

@NancyLi1013 rapidyaml needs update to latest version. I created another PR #18317.

I noticed that you commented #18317 also requires this PR here #18317 (comment).

In this case, I suggest to update these two ports in one PR. Otherwise, CI test will not pass.

@ysc3839 ysc3839 changed the title [c4core] Add windows arm/arm64 support [c4core, ryml] Add windows arm/arm64 support Jun 10, 2021
@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 10, 2021

@NancyLi1013 I just merge changes for rapidyaml into this branch. Please review.

NancyLi1013
NancyLi1013 suggested changes Jun 11, 2021
View reviewed changes
ports/ryml/CONTROL Outdated Show resolved Hide resolved
ports/ryml/portfile.cmake Show resolved Hide resolved
@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 11, 2021

@NancyLi1013 Fixed, please review.

@NancyLi1013
Copy link
Contributor

Please run ./vcpkg x-add-version --overwrite-version c4core ryml to update versions.

@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 11, 2021

@NancyLi1013 Fixed, please review.

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@NancyLi1013
Copy link
Contributor

Could you please merge master to resolve the conflicts? @ysc3839

@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 17, 2021

@NancyLi1013 Updated, please review.

@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 22, 2021

Fixed version error.

@NancyLi1013
Copy link
Contributor

Missing versions/baselin.json file. You can runvpkg x-add-version --overwrite-version --allto update.

@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 23, 2021

@NancyLi1013 After running vcpkg x-add-version --overwrite-version --all, the versions/baseline.json keep unchanged.

NancyLi1013
NancyLi1013 suggested changes Jun 24, 2021
View reviewed changes
ports/c4core/vcpkg.json Outdated Show resolved Hide resolved
ports/ryml/vcpkg.json Outdated Show resolved Hide resolved
@NancyLi1013
Copy link
Contributor

@NancyLi1013 After running vcpkg x-add-version --overwrite-version --all, the versions/baseline.json keep unchanged.

Since you didn't update the version in CONTROL files. I have submitted the review suggestions, please update version first, then run the command. Sorry, I ignored this point before.

@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 24, 2021

@NancyLi1013 Fixed.

@NancyLi1013 NancyLi1013 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Jun 24, 2021
@NancyLi1013
Copy link
Contributor

LGTM now, thanks for your PR @ysc3839.

@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 25, 2021

@NancyLi1013 rapidyaml has merged my PR for ARM build issue. I will update this PR to remove the patches.

@NancyLi1013 NancyLi1013 removed the info:reviewed Pull Request changes follow basic guidelines label Jun 25, 2021
@NancyLi1013
Copy link
Contributor

@NancyLi1013 rapidyaml has merged my PR for ARM build issue. I will update this PR to remove the patches.

Thanks for your information. That's great.

@biojppm biojppm mentioned this pull request Jun 26, 2021
Closed
@ysc3839
Copy link
Contributor Author

ysc3839 commented Jun 29, 2021

@NancyLi1013 Updated and rebased, please review.

@NancyLi1013 NancyLi1013 added the category:port-update The issue is with a library, which is requesting update new revision label Jun 30, 2021
@NancyLi1013
Copy link
Contributor

LGTM, thanks again for your work. @ysc3839

@NancyLi1013 NancyLi1013 added the info:reviewed Pull Request changes follow basic guidelines label Jun 30, 2021
@ysc3839
Copy link
Contributor Author

ysc3839 commented Jul 14, 2021

@NancyLi1013 Is there any updates of this PR?

@NancyLi1013
Copy link
Contributor

@ras0219-msft , @vicroms Could you please help merge this PR?

Thanks.

@BillyONeal
Copy link
Member

Sorry for the delay, thanks for the help!

@BillyONeal BillyONeal merged commit d90f71a into microsoft:master Jul 16, 2021
@ysc3839 ysc3839 deleted the c4core branch July 16, 2021 08:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelGoulding MichaelGoulding MichaelGoulding left review comments

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist category:port-update The issue is with a library, which is requesting update new revision info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ysc3839 @NancyLi1013 @BillyONeal @MichaelGoulding