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

[promise-cpp] Add new port #20340

Merged
merged 10 commits into from
Sep 30, 2021
Merged

Conversation

chausner
Copy link
Contributor

@chausner chausner commented Sep 25, 2021

Describe the pull request

  • What does your PR fix?

    Adds new port promise-cpp

  • 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

@chausner chausner changed the title [promise-cpp] Update to 1.0.3 [promise-cpp] Add new port Sep 25, 2021
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.

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for promise-cpp but no changes to version or port version.
-- Version: 1.0.3
-- Old SHA: fbd1822f72da4ff1934d6395ddd918d68f9c0c0a
-- New SHA: ef3fb5249eed37360ceb9b3b366037ad2ed2c865
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Sep 26, 2021
ports/promise-cpp/portfile.cmake Outdated Show resolved Hide resolved
ports/promise-cpp/vcpkg.json Outdated Show resolved Hide resolved
@JonLiu1993
Copy link
Member

This is error log:
install-x64-osx-dbg-out.log

@chausner
Copy link
Contributor Author

chausner commented Sep 26, 2021

I updated the port for the new major version 2.1.0 which was released yesterday. Unfortunately, the CMake install targets have apparently be removed from CMakeLists.txt so I now simply copy the headers into include.

The suggestion by vcpkg is not ideal, though:

The package promise-cpp is header only and can be used from CMake via:

find_path(PROMISE_CPP_INCLUDE_DIRS "add_ons.hpp")
target_include_directories(main PRIVATE ${PROMISE_CPP_INCLUDE_DIRS})

add_ons.hpp is not the primary header file. Is there any way to change what header file it recommends?

@dg0yt
Copy link
Contributor

dg0yt commented Sep 26, 2021

Is there any way to change what header file it recommends?

Add a usage file with the rectified usage instructions.

@chausner
Copy link
Contributor Author

Is there any way to change what header file it recommends?

Add a usage file with the rectified usage instructions.

Thanks, added a usage file.

@JonLiu1993 JonLiu1993 added info:reviewed Pull Request changes follow basic guidelines and removed requires:author-response labels Sep 27, 2021
@BillyONeal
Copy link
Member

I'm concerned that this port takes likely to conflict names:

  • any.hpp
  • add_ons.hpp
  • call_traits.hpp

Would you consider it acceptable to install into a subidrectory like <promise-cpp/promise.hpp>?

@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Sep 28, 2021
@chausner chausner marked this pull request as draft September 28, 2021 23:10
@chausner
Copy link
Contributor Author

Would you consider it acceptable to install into a subidrectory like <promise-cpp/promise.hpp>?

I noticed there is also another issue that is preventing the port from being usable right now: https://github.com/xhawk18/promise-cpp/blob/5e533a1c2beb363ca6b87f7f002007388349366c/include/promise.hpp#L262

I will open an issue and ask the maintainer if they can address these issues in the library itself so we don't have to work around them with patches.

@BillyONeal
Copy link
Member

I noticed there is also another issue that is preventing the port from being usable right now: https://github.com/xhawk18/promise-cpp/blob/5e533a1c2beb363ca6b87f7f002007388349366c/include/promise.hpp#L262

Ouch! Best of luck!

@BillyONeal BillyONeal added depends:upstream-changes Waiting on a change to the upstream project and removed requires:author-response labels Sep 29, 2021
@chausner chausner marked this pull request as ready for review September 29, 2021 18:03
@chausner
Copy link
Contributor Author

@BillyONeal This should be ready now.

Copy link
Contributor

@PhoebeHui PhoebeHui left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for your contribution!

@PhoebeHui PhoebeHui added info:reviewed Pull Request changes follow basic guidelines and removed depends:upstream-changes Waiting on a change to the upstream project labels Sep 30, 2021
@BillyONeal BillyONeal merged commit 3434e5c into microsoft:master Sep 30, 2021
@BillyONeal
Copy link
Member

Thanks for running that down with upstream!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:new-port The issue is requesting a new library to be added; consider making a PR! info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants