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

[osmanip] Add new port #30016

Merged
merged 78 commits into from
Mar 8, 2023
Merged

[osmanip] Add new port #30016

merged 78 commits into from
Mar 8, 2023

Conversation

JustWhit3
Copy link
Contributor

@JustWhit3 JustWhit3 commented Mar 4, 2023

Describe the pull request

  • What does your PR fix?

    This PR adds port to osmanip.

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

    Linux, MacOS, Windows. 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

@dg0yt
Copy link
Contributor

dg0yt commented Mar 6, 2023

The suggested osmanip change is good (with regard to the CONFIG, REQUIRED and PRIVATE keywords), but what is really missing is

include(CMakeFindDependencyMacro)
find_dependency(arsenalgear CONFIG)

early in osmanip's cmake/Config.cmake.in.

@JustWhit3
Copy link
Contributor Author

JustWhit3 commented Mar 6, 2023

Hello,

@dg0yt I added your updates, now it should be fixed.

@JonLiu1993 If I add this line:

find_package( arsenalgear CONFIG REQUIRED ):

I get an error:

CMake Error at CMakeLists.txt:47 (find_package):
  Could not find a package configuration file provided by "arsenalgear" with
  any of the following names:

    arsenalgearConfig.cmake
    arsenalgear-config.cmake

  Add the installation prefix of "arsenalgear" to CMAKE_PREFIX_PATH or set
  "arsenalgear_DIR" to a directory containing one of the above files.  If
  "arsenalgear" provides a separate development package or SDK, be sure it
  has been installed.

But If I simply add:

find_package( arsenalgear CONFIG )

It works.

@JonLiu1993
Copy link
Member

@JustWhit3, Very strange, I can successfully compile with Find_package (Osmanip Config Required):
CMakeLists:

cmake_minimum_required (VERSION 3.8)

project(test)

find_package(osmanip CONFIG REQUIRED)
find_package(arsenalgear CONFIG REQUIRED)

add_executable (main "CMakeProject42.cpp" "CMakeProject42.h")

target_link_libraries(main PRIVATE osmanip::osmanip)

1> CMake generation started for configuration: 'x64-Debug'.
1> Command line: "C:\WINDOWS\system32\cmd.exe" /c "%SYSTEMROOT%\System32\chcp.com 65001 >NUL && "C:\PROGRAM FILES (X86)\MICROSOFT VISUAL STUDIO\2019\ENTERPRISE\COMMON7\IDE\COMMONEXTENSIONS\MICROSOFT\CMAKE\CMake\bin\cmake.exe"  -G "Visual Studio 16 2019" -A x64  -DCMAKE_CONFIGURATION_TYPES:STRING="Debug" -DCMAKE_INSTALL_PREFIX:PATH="C:\Users\test\source\repos\CMakeProject42\out\install\x64-Debug" -DCMAKE_TOOLCHAIN_FILE=F:/Feature-test/osmanip/vcpkg/scripts/buildsystems/vcpkg.cmake "C:\Users\test\source\repos\CMakeProject42" 2>&1"
1> Working directory: C:\Users\test\source\repos\CMakeProject42\out\build\x64-Debug
1> [CMake] -- Selecting Windows SDK version 10.0.22000.0 to target Windows 10.0.19045.
1> [CMake] -- The C compiler identification is MSVC 19.29.30148.0
1> [CMake] -- The CXX compiler identification is MSVC 19.29.30148.0
1> [CMake] -- Detecting C compiler ABI info
1> [CMake] -- Detecting C compiler ABI info - done
1> [CMake] -- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe - skipped
1> [CMake] -- Detecting C compile features
1> [CMake] -- Detecting C compile features - done
1> [CMake] -- Detecting CXX compiler ABI info
1> [CMake] -- Detecting CXX compiler ABI info - done
1> [CMake] -- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio/2019/Enterprise/VC/Tools/MSVC/14.29.30133/bin/Hostx64/x64/cl.exe - skipped
1> [CMake] -- Detecting CXX compile features
1> [CMake] -- Detecting CXX compile features - done
1> [CMake] -- Configuring done
1> [CMake] -- Generating done
1> [CMake] -- Build files have been written to: C:/Users/test/source/repos/CMakeProject42/out/build/x64-Debug
1> Extracted CMake variables.
1> Extracted source files and headers.
1> Extracted code model.
1> Extracted toolchain configurations.
1> Extracted includes paths.
1> CMake generation finished.

@JustWhit3
Copy link
Contributor Author

@JonLiu1993 I put that line of code exactly here into the osmanip CMakeLists.txt file. However, is it important to have REQUIRED in order to let it work? It seems all fine without it.

@JonLiu1993
Copy link
Member

JonLiu1993 commented Mar 6, 2023

@JonLiu1993 I put that line of code exactly here into the osmanip CMakeLists.txt file. However, is it important to have REQUIRED in order to let it work? It seems all fine without it.

You have added to cmake/config.cmake.in

Include (CmakefindDependencyMacro)
Find_Dependency (ARSENALGEAR Config)

It will automatically modify the files in CMAKELISTS. What you have to do now is to make a patch manually modify the usage in CMakeLists to ensure that users can use it for the time being

Find_package (ARSENALGEAR)
target_link_libraries (OSMANIP PUBLIC ARSENALGEAR :: ARSENALGEAR)

After the next stable version is released, this patch is no longer needed

@JustWhit3
Copy link
Contributor Author

@JonLiu1993 I put that line of code exactly here into the osmanip CMakeLists.txt file. However, is it important to have REQUIRED in order to let it work? It seems all fine without it.

You have added to cmake/config.cmake.in

Include (CmakefindDependencyMacro)
Find_Dependency (ARSENALGEAR Config)

It will automatically modify the files in CMAKELISTS. What you have to do now is to make a patch S manually modify the usage in CMAKELISTS to ensure that users can use it for the time being

Find_package (ARSENALGEAR)
target_link_libraries (OSMANIP PUBLIC ARSENALGEAR :: ARSENALGEAR)

After the next stable version is released, this patch is no longer needed

Updated. Now it should be correct.

@JonLiu1993
Copy link
Member

@JustWhit3, Sorry, maybe my statement is not clear enough, the only thing you need to do now is to make a patch, the patch content is to https://github.com/JustWhit3/osmanip/blob/4c810a579eba784af0e86c1bec10cb33832a8e63/CMakeLists.txt#L47-L48

    find_package(arsenalgear)
    target_link_libraries( osmanip PUBLIC arsenalgear::arsenalgear )

change into

     find_package(arsenalgear CONFIG REQUIRED)
     target_link_libraries(main PRIVATE arsenalgear::arsenalgear)

@dg0yt
Copy link
Contributor

dg0yt commented Mar 7, 2023

AFAICS upstream source has capabilities to fetch arsenalgear. This may hide the lack to properly use vcpkg's arsenalgear (REQUIRED). This structure should be improved.

https://github.com/JustWhit3/osmanip/blob/fd70ee63c32f2b4d05f9092cb3b938af6928d867/CMakeLists.txt#L26-L28

https://github.com/JustWhit3/osmanip/blob/fd70ee63c32f2b4d05f9092cb3b938af6928d867/deps/arsenalgear/CMakeLists.txt#L18-L31

@JustWhit3
Copy link
Contributor Author

@JonLiu1993 Ok I added a new patch and now it should be correct.

@dg0yt So is it right what I did? I sincerly didn't completely understand your statement, sorry.

ports/osmanip/portfile.cmake Outdated Show resolved Hide resolved
@JonLiu1993
Copy link
Member

Usage tested successfully by osmanip:x64-windows:

    # this is heuristically generated, and may not be correct
    find_package(osmanip CONFIG REQUIRED)
    target_link_libraries(main PRIVATE osmanip::osmanip)

JonLiu1993
JonLiu1993 previously approved these changes Mar 8, 2023
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Mar 8, 2023
@dan-shaw dan-shaw merged commit b10e61a into microsoft:master Mar 8, 2023
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.

4 participants