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

[popsift] fix copy tools #17954

Merged
merged 8 commits into from
May 21, 2021
Merged

[popsift] fix copy tools #17954

merged 8 commits into from
May 21, 2021

Conversation

simogasp
Copy link
Contributor

@simogasp simogasp commented May 15, 2021 •
edited
Loading

  • What does your PR fix?

Popsift was not installing the dlls in bin folder because the bin folder was just moved into the tools directory.
Properly using vcpkg_copy_tools() instead of managing the file manually fixes the problem.
Incidentally, it adds the now required FEATURES to vcpkg_check_feature()

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

    all, Yes

  • 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

ras0219
ras0219 approved these changes May 17, 2021
View reviewed changes
ports/popsift/portfile.cmake Outdated Show resolved Hide resolved
NancyLi1013
NancyLi1013 suggested changes May 18, 2021
View reviewed changes
ports/popsift/portfile.cmake Outdated Show resolved Hide resolved
ports/popsift/portfile.cmake Show resolved Hide resolved
ports/popsift/portfile.cmake Outdated Show resolved Hide resolved
@NancyLi1013 NancyLi1013 added category:port-bug The issue is with a library, which is something the port should already support requires:author-response labels May 18, 2021
@simogasp simogasp requested a review from NancyLi1013 May 18, 2021 17:24
@simogasp
Copy link
Contributor Author

Could you re-launch the CI? I don't think the failure is coming from this PR

@NancyLi1013
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@simogasp
Copy link
Contributor Author

simogasp commented May 19, 2021 •
edited
Loading

/azp run

Thanks, but this time is another different pkg that is failing :-(

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

LGTM now, thanks for your fixing @simogasp.

@dan-shaw dan-shaw merged commit 094e58f into microsoft:master May 21, 2021
@simogasp simogasp deleted the popsift/fixTools branch May 21, 2021 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ras0219 ras0219 ras0219 approved these changes

@NancyLi1013 NancyLi1013 NancyLi1013 approved these changes

Assignees

@NancyLi1013 NancyLi1013

Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@simogasp @NancyLi1013 @ras0219 @dan-shaw