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

[arg-router] Update to v1.2.0 #29869

Merged
merged 4 commits into from
Mar 2, 2023
Merged

Conversation

cmannett85
Copy link
Contributor

@cmannett85 cmannett85 commented Feb 26, 2023

arg_router v1.2.0

Bug fixes

  • Issue 254, passing no args to short_form_expander_t::pre_parse_phase(..) causes segfault
  • Issue 255, policy_unique_from_owner_parent_to_mode_or_root not working

Improvements

  • Corrected CMake package so you don't need to manually set the include directory anymore (Issue 257)
  • Node can be instantiated with bare compile-time strings and they are automatically mapped to the appropriate policy (Issue 256)
  • Improved help output formatting documentation (Issue 258)

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

@cmannett85 cmannett85 force-pushed the arg_router_v1.2.0 branch 2 times, most recently from 25b7af6 to 125dcfc Compare February 26, 2023 14:42
@cmannett85 cmannett85 marked this pull request as ready for review February 26, 2023 14:45
@MonicaLiu0311 MonicaLiu0311 added the category:port-update The issue is with a library, which is requesting update new revision label Feb 27, 2023
@MonicaLiu0311
Copy link
Contributor

usage:

The package arg-router is a header-only library and so is typically used like this:

    find_package(arg_router REQUIRED)
    target_link_libraries(my_exe PUBLIC Boost::boost arg_router)

When testing usage, the following error occurs:

CMake Error at CMakeFindUsage/CMakeLists.txt:22 (target_link_libraries):
  Target "CMakeFindUsage" links to:

    Boost::boost

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.	CMakeFindUsage	C:\Users\monica\source\repos\CMakeFindUsage\CMakeFindUsage/CMakeLists.txt	22	

CMakeFindUsage.cpp
#include <iostream>
#include "arg_router/arg_router.hpp"

using namespace std;

int main()
{
cout << "Hello CMake." << endl;
return 0;
}

CMakeLists.txt
cmake_minimum_required (VERSION 3.8)

set(CMAKE_TOOLCHAIN_FILE "E:/arg-router/scripts/buildsystems/vcpkg.cmake")

project ("CMakeFindUsage")

find_package(arg_router REQUIRED)

add_executable (CMakeFindUsage "CMakeFindUsage.cpp")

target_link_libraries(CMakeFindUsage PUBLIC Boost::boost arg_router)

@cmannett85
Copy link
Contributor Author

You're right, I blindly copied the line from a real project that had find_package(Boost) call in it. arg_router's CMake package is supposed to check the dependency itself but I'm still working on that - it doesn't matter in the context of vcpkg though as that dependency is managed via vcpkg.json so the required Boost headers are always there.

@cmannett85
Copy link
Contributor Author

@MonicaLiu0311 I've updated the patch but I can't remove the requires:author-response label.

@MonicaLiu0311
Copy link
Contributor

Using VS2022 to test the new usage, an error occurred during build, see the attachment for details.

output.txt

@cmannett85
Copy link
Contributor Author

cmannett85 commented Feb 28, 2023

Using VS2022 to test the new usage, an error occurred during build, see the attachment for details.

output.txt

@MonicaLiu0311 this library requires a minimum of C++17, MSVC defaults to C++14 so you'll need to add target_compile_features(CMakeFindUsage PUBLIC cxx_std_17) (or higher) to your CMakeLists.txt. I don't think it's appropriate for me to add toolchain related settings to the usage file, the minimum C++ version is specified in the library documentation, a link to which is in the usage file.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

Extra notes for upstream:

  • It would have been better if the exported target would use a namespace (arg_router::...). Cmake could recognize it is a target, and clearly and early notify the user if he forgot find_package().
  • It would be good if some variables could be turned into cache variables (aka input variables), in particular install locations e.g. for cmake config.

versions/a-/arg-router.json Outdated Show resolved Hide resolved
Bug fixes
* Issue 254, passing no args to short_form_expander_t::pre_parse_phase(..) causes segfault
* Issue 255, policy_unique_from_owner_parent_to_mode_or_root not working

Improvements
* Corrected CMake package so you don't need to manually set the include directory anymore (Issue 257)
* Node can be instantiated with bare compile-time strings and they are automatically mapped to the appropriate policy (Issue 256)
* Improved help output formatting documentation (Issue 258)
@cmannett85
Copy link
Contributor Author

  • It would have been better if the exported target would use a namespace (arg_router::...). Cmake could recognize it is a target, and clearly and early notify the user if he forgot find_package().

I avoided using a namespace simply because there is only one exported target from the package, but you're right, it doesn't fit the pattern of other packages so I'll update it for the next release (cmannett85/arg_router#275).

  • It would be good if some variables could be turned into cache variables (aka input variables), in particular install locations e.g. for cmake config.

In a 'normal' installation CMake would look in certain platform-specific locations for the package config files, so I never considered making this settable by the user. But obviously vcpkg changes this, so I'll add one in (cmannett85/arg_router#276).

Thanks for your feedback!

@cmannett85 cmannett85 requested a review from dg0yt March 1, 2023 08:44
@dg0yt
Copy link
Contributor

dg0yt commented Mar 1, 2023

In a 'normal' installation CMake would look in certain platform-specific locations for the package config files...

I always find this confusing: While the documentation marks some locations as being "meant for" some platform, the fine print states:

This is merely a convention, so all (W) and (U) directories are still searched on all platforms.

@cmannett85
Copy link
Contributor Author

cmannett85 commented Mar 1, 2023

This is merely a convention, so all (W) and (U) directories are still searched on all platforms.

Ha, so I could have had share/arg_router all along!

@MonicaLiu0311
Copy link
Contributor

MonicaLiu0311 commented Mar 1, 2023

The usage test passed (header files found):

The package arg-router is a header-only library and so is typically used like this:

    find_package(arg_router REQUIRED)
    include_directories(SYSTEM "${arg_router_INCLUDE_DIRS}")

After correcting the above usage:

The package arg-router is a header-only library and so is typically used like this:

    find_package(arg_router REQUIRED)
    target_link_libraries(my_exe PUBLIC arg_router)

@cmannett85
Copy link
Contributor Author

@MonicaLiu0311 the include_directories call is not necessary, as the package provides an exported target which already has the include directory as a property. So your test CMakeLists.txt should be:

cmake_minimum_required (VERSION 3.8)
set(CMAKE_TOOLCHAIN_FILE "E:/arg-router/scripts/buildsystems/vcpkg.cmake")

project ("CMakeFindUsage")
find_package(arg_router REQUIRED)

add_executable (CMakeFindUsage "CMakeFindUsage.cpp")
target_link_libraries(CMakeFindUsage PUBLIC arg_router)
target_compile_features(CMakeFindUsage PUBLIC cxx_std_17)

@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Mar 2, 2023
@dan-shaw dan-shaw merged commit e4040c6 into microsoft:master Mar 2, 2023
@cmannett85 cmannett85 deleted the arg_router_v1.2.0 branch March 2, 2023 21:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

4 participants