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

[casbin-cpp] Add new port #29027

Closed
wants to merge 7 commits into from
Closed

[casbin-cpp] Add new port #29027

wants to merge 7 commits into from

Conversation

ex-purple
Copy link
Contributor

github-actions[bot]
github-actions bot previously approved these changes Jan 17, 2023
@LilyWangLL LilyWangLL added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jan 18, 2023
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: checked-in files for casbin-cpp have changed but the version was not updated
version: 1.52.0
old SHA: 8232a784d1db1a9794d849a34a80c43b383ebb87
new SHA: 528054dc212781d05ed65359f85359e67c14f376
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

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: checked-in files for casbin-cpp have changed but the version was not updated
version: 1.52.0
old SHA: 8232a784d1db1a9794d849a34a80c43b383ebb87
new SHA: 1fd2cfff3369f7ba7ebfc2e285aadce5a39296c3
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

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: checked-in files for casbin-cpp have changed but the version was not updated
version: 1.52.0
old SHA: 8232a784d1db1a9794d849a34a80c43b383ebb87
new SHA: 0b36cc1fbe5680bf7ae92a66c02241a747e21875
Did you remember to update the version or port version?
Use --overwrite-version to bypass this check
***No files were updated***

github-actions[bot]
github-actions bot previously approved these changes Jan 18, 2023
LilyWangLL
LilyWangLL previously approved these changes Jan 18, 2023
@ex-purple ex-purple requested a review from LilyWangLL January 18, 2023 22:58
@LilyWangLL LilyWangLL added the info:reviewed Pull Request changes follow basic guidelines label Jan 19, 2023
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

  • Changes comply with the maintainer guide
  • The name of the port matches an existing name for this component on https://repology.org/ if possible, and/or is strongly associated with that component on search engines.
  • Optional dependencies are resolved in exactly one way. For example, if the component is built with CMake, all find_package calls are REQUIRED, are satisfied by vcpkg.json's declared dependencies, or disabled with CMAKE_DISABLE_FIND_PACKAGE_Xxx
  • The versioning scheme in vcpkg.json matches what upstream says.
  • The license declaration in vcpkg.json matches what upstream says.
  • The installed as the "copyright" file matches what upstream says.
  • The source code of the component installed comes from an authoritative source.
  • The generated "usage text" is accurate. See docs/examples/adding-an-explicit-usage.md for context.

This seems wrong: upstream talks about needing to add a library in their readme but currently the usage text is:

casbin-cpp is header-only and can be used from CMake via:

    find_path(CASBIN_CPP_INCLUDE_DIRS "casbin/casbin.h")
    target_include_directories(main PRIVATE ${CASBIN_CPP_INCLUDE_DIRS})

Can you investigate? It looks like you might not be actually building the library?

  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is in the new port's versions file.
  • Only one version is added to each modified port's versions file.

@BillyONeal BillyONeal added requires:author-response and removed info:reviewed Pull Request changes follow basic guidelines labels Jan 20, 2023
@ex-purple
Copy link
Contributor Author

@BillyONeal you are right about a cmake usage. Oddly, casbin-cpp does not install the library and cmake config too.

Looks like this project orient to use FetchContent. Local installation looks broken. Patches are the only way to fix it.

@ex-purple
Copy link
Contributor Author

@BillyONeal I figured out what's wrong
Library and CMake configuration are installed in the CMake User Package Registry.

@BillyONeal
Copy link
Member

@BillyONeal I figured out what's wrong Library and CMake configuration are installed in the CMake User Package Registry.

I don't understand what this means. They are not being installed in anything deployed by vcpkg. If the port is expecting to touch something outside of the installed tree to do its installation, that will fail when the port is restored from the binary cache (where portfile.cmake does not run, only the reuslts the port installed to ${CURRENT_PACKAGES_DIR} are relevant then).

@ex-purple
Copy link
Contributor Author

I understand that this approach is incorrect for vpkg. To fix this, I will need to create a patch.

@ex-purple
Copy link
Contributor Author

Sorry, this port is no more interesting for me.

@BillyONeal
Copy link
Member

I'm sorry to hear that it didn't work out; thanks for the contribution nonetheless!

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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[New Port Request] <casbin-cpp>
4 participants