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

[faker-cxx] add new port #38583

Draft
wants to merge 19 commits into
base: master
Choose a base branch
from
Draft

[faker-cxx] add new port #38583

wants to merge 19 commits into from
+125 −0

Conversation

mostsignificant
Copy link

  • 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 adding-usage for context.
  • 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.

@mostsignificant
Copy link
Author

@microsoft-github-policy-service agree

@mostsignificant mostsignificant mentioned this pull request May 5, 2024
Open
@JonLiu1993 JonLiu1993 self-assigned this May 6, 2024
@JonLiu1993 JonLiu1993 changed the title Faker cxx [faker-cxx] add new port May 6, 2024
@JonLiu1993 JonLiu1993 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label May 6, 2024
dg0yt
dg0yt reviewed May 7, 2024
View reviewed changes
ports/faker-cxx/portfile.cmake Outdated
@@ -0,0 +1,31 @@
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a general limitation or just missing Windows DLL exports?

Copy link
Author

Choose a reason for hiding this comment

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

2nd - I initially did not find any in the original project's CMakeFiles ... maybe at a later point

Copy link
Contributor

Choose a reason for hiding this comment

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

But then please

Suggested change
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
if(VCPKG_TARGET_IS_WINDOWS)
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
endif()

ports/faker-cxx/usage Outdated
Comment on lines 3 to 4
find_package(faker-cxx CONFIG REQUIRED)
target_link_libraries(main PRIVATE faker-cxx::faker-cxx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
find_package(faker-cxx CONFIG REQUIRED)
target_link_libraries(main PRIVATE faker-cxx::faker-cxx)
find_package(faker-cxx CONFIG REQUIRED)
target_link_libraries(main PRIVATE faker-cxx::faker-cxx)

with new line at the end.
Or just the heuristical tool output if it is good enough,

ports/faker-cxx/portfile.cmake Outdated Show resolved Hide resolved
ports/faker-cxx/CMakeLists.txt.patch
- set(FMT_INCLUDE_DIR "${CMAKE_SOURCE_DIR}/externals/fmt/include")
- target_link_libraries(${LIBRARY_NAME} PRIVATE fmt)
+ find_package(fmt CONFIG REQUIRED)
+ target_link_libraries(${LIBRARY_NAME} PRIVATE fmt::fmt)
Copy link
Contributor

Choose a reason for hiding this comment

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

This will require find_dependency in exported config. Is it present?

Copy link
Author

Choose a reason for hiding this comment

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

Unfortunately I am that proficient in CMake - for Apple clang builds, fmt will be included via vcpkg, too. But can you give me an example from another project on what I might be missing here?

Copy link
Contributor

Choose a reason for hiding this comment

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

for Apple clang builds, fmt will be included via vcpkg, too.

I don't understand what this means.

can you give me an example from another project on what I might be missing here?

It is hard to find a minimal example but maybe soci comes close enough.
Making soci use unofficial::sqlite3::sqlite3 here:

vcpkg/ports/soci/dependencies.diff

Lines 43 to 52 in f4456c1

--- a/cmake/dependencies/SQLite3.cmake
+++ b/cmake/dependencies/SQLite3.cmake
@@ -1,5 +1,6 @@
set(SQLITE3_FIND_QUIETLY TRUE)
-find_package(SQLite3)
+find_package(SQLITE3 NAMES unofficial-sqlite3 CONFIG REQUIRED)
+set(SQLITE3_LIBRARIES unofficial::sqlite3::sqlite3)
boost_external_report(SQLite3 INCLUDE_DIR LIBRARIES)

makes $<LINK_ONLY:unofficial::sqlite3::sqlite3> appearing in the INTERFACE_LINK_LIBRARIES of the exported CMake config, so downstream usage will need to know what this target means. And that's why the same patch carries a change to the config file template:

vcpkg/ports/soci/dependencies.diff

Lines 55 to 68 in f4456c1

--- a/cmake/resources/SOCIConfig.cmake.in
+++ b/cmake/resources/SOCIConfig.cmake.in
@@ -1,3 +1,11 @@
@PACKAGE_INIT@
+include(CMakeFindDependencyMacro)
+if("@WITH_MYSQL@")
+ find_dependency(unofficial-libmysql)
+endif()
+if("@WITH_SQLITE3@")
+ find_dependency(unofficial-sqlite3)
+endif()
+
include(${CMAKE_CURRENT_LIST_DIR}/SOCITargets.cmake)

ports/faker-cxx/portfile.cmake Outdated Show resolved Hide resolved
JonLiu1993
JonLiu1993 reviewed May 7, 2024
View reviewed changes
ports/faker-cxx/usage Outdated Show resolved Hide resolved
mostsignificant and others added 2 commits May 9, 2024 14:52
@mostsignificant @JonLiu1993 @dg0yt
Apply suggestions from code review
2fad384 
usage new lines, vcpkg copyright function, config path fix, head_ref and ref

Co-authored-by: JonLiu1993 <63675417+JonLiu1993@users.noreply.github.com>
Co-authored-by: Kai Pastor <dg0yt@darc.de>
android build clang++ ranges fix
f234052
dg0yt
dg0yt reviewed May 9, 2024
View reviewed changes
ports/faker-cxx/CMakeLists.txt.patch Outdated Show resolved Hide resolved
ports/faker-cxx/CMakeLists.txt.patch Outdated
+ set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -std=c++20 -stdlib=libc++ -fexperimental-library")
else ()
set(CMAKE_CXX_FLAGS
"${CMAKE_CXX_FLAGS} -Wall -Wextra -Wpedantic -Wconversion -Wformat -Werror"
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW -Werror is nice for controlled environments (upstream development), but not appropriate for individual builds over a write range of environments.

Copy link
Author

Choose a reason for hiding this comment

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

Okay, I removed it via the patch file. However, I might submit a PR for the original project to remove this, too and only enable it in their own CI/CD

dg0yt
dg0yt reviewed May 13, 2024
View reviewed changes
versions/f-/faker-cxx.json Outdated Show resolved Hide resolved
ports/faker-cxx/vcpkg.json Outdated Show resolved Hide resolved
ports/faker-cxx/usage Outdated
Comment on lines 3 to 4
find_package(faker-cxx CONFIG REQUIRED)
target_link_libraries(main PRIVATE faker-cxx::faker-cxx)
Copy link
Contributor

Choose a reason for hiding this comment

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

For common formatting:

Suggested change
find_package(faker-cxx CONFIG REQUIRED)
target_link_libraries(main PRIVATE faker-cxx::faker-cxx)
find_package(faker-cxx CONFIG REQUIRED)
target_link_libraries(main PRIVATE faker-cxx::faker-cxx)

Copy link
Author

Choose a reason for hiding this comment

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

I saw some ports using 4-space intends - is it 2-space or 4-space?

Copy link
Contributor

Choose a reason for hiding this comment

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

There is no clear guideline, but...
It used to be 4 spaces. Then the tool's heuristical method was changed to use 2 spaces. Most of the existing usage files haven't been updated.

ports/faker-cxx/portfile.cmake Outdated
Comment on lines 23 to 26
vcpkg_cmake_config_fixup(
PACKAGE_NAME "faker-cxx"
CONFIG_PATH lib/cmake/faker-cxx
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
vcpkg_cmake_config_fixup(
PACKAGE_NAME "faker-cxx"
CONFIG_PATH lib/cmake/faker-cxx
)
vcpkg_cmake_config_fixup(CONFIG_PATH lib/cmake/faker-cxx)

ports/faker-cxx/portfile.cmake Outdated
Comment on lines 10 to 12
"CMakeLists.txt.patch"
"FormatHelper.h.patch"
"Helper.h.patch"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"CMakeLists.txt.patch"
"FormatHelper.h.patch"
"Helper.h.patch"
CMakeLists.txt.patch
FormatHelper.h.patch
Helper.h.patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dg0yt dg0yt dg0yt left review comments

@JonLiu1993 JonLiu1993 JonLiu1993 left review comments

Assignees

@JonLiu1993 JonLiu1993

Labels
category:new-port The issue is requesting a new library to be added; consider making a PR!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mostsignificant @dg0yt @JonLiu1993