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

[flycv] new port flycv 1.2 #31986

Closed
wants to merge 24 commits into from
Closed

[flycv] new port flycv 1.2 #31986

wants to merge 24 commits into from

Conversation

sdcb
Copy link
Contributor

@sdcb sdcb commented Jun 14, 2023

  • 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.

ports/flycv/int64_and_install.patch Outdated Show resolved Hide resolved
ports/flycv/portfile.cmake Outdated Show resolved Hide resolved
@jimwang118 jimwang118 added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Jun 15, 2023
@jimwang118
Copy link
Contributor

Note: I will be converting your PR to draft status. When you respond, please revert to "ready for review". That way, I can be aware that you've responded since you can't modify the tags.

@jimwang118 jimwang118 marked this pull request as draft June 15, 2023 08:15
ports/flycv/int64_and_install.patch Outdated Show resolved Hide resolved
ports/flycv/int64_and_install.patch Outdated Show resolved Hide resolved
@sdcb
Copy link
Contributor Author

sdcb commented Jun 16, 2023

@dg0yt @jimwang118 Thank you all, vcpkg managed 3rd-party dependencies have already working, but still one question to ask, how to write usage file? I'm not familiar with cmake before, should I write(like libpng):

The package flycv is compatible with built-in CMake targets:

    find_package(FLYCV REQUIRED)
    target_link_libraries(main PRIVATE FLYCV::FLYCV)

Or (like libjpeg-turbo)

flycv provides CMake targets for the flycv C/C++ API:

    find_package(flycv CONFIG REQUIRED)
    target_link_libraries(main PRIVATE FLYCV::FLYCV)

How can I tell the different? Which one are correct in this case?

@sdcb sdcb marked this pull request as ready for review June 16, 2023 06:22
ports/flycv/portfile.cmake Outdated Show resolved Hide resolved
@sdcb
Copy link
Contributor Author

sdcb commented Jun 19, 2023

There is no problem with compiling now. How can I use this library after compiling?

This part actually I don't know for cmake, but it works in Visual Studio, I also wants to check with you how to write usage file? I'm not familiar with cmake before, should I write(like libpng):

The package flycv is compatible with built-in CMake targets:

    find_package(FLYCV REQUIRED)
    target_link_libraries(main PRIVATE FLYCV::FLYCV)

Or (like libjpeg-turbo)

flycv provides CMake targets for the flycv C/C++ API:

    find_package(flycv CONFIG REQUIRED)
    target_link_libraries(main PRIVATE FLYCV::FLYCV)

How can I tell the different? Which one are correct in this case?

@jimwang118
Copy link
Contributor

There is no problem with compiling now. How can I use this library after compiling?

This part actually I don't know for cmake, but it works in Visual Studio, I also wants to check with you how to write usage file? I'm not familiar with cmake before, should I write(like libpng):

The package flycv is compatible with built-in CMake targets:

    find_package(FLYCV REQUIRED)
    target_link_libraries(main PRIVATE FLYCV::FLYCV)

Or (like libjpeg-turbo)

flycv provides CMake targets for the flycv C/C++ API:

    find_package(flycv CONFIG REQUIRED)
    target_link_libraries(main PRIVATE FLYCV::FLYCV)

How can I tell the different? Which one are correct in this case?

This represents two usage modes of find_package(), one is the default module mode and the other is the config mode. You can check the cmake official website to understand this. For generating usage, you can check cmake's install syntax to complete the export usage.

@sdcb
Copy link
Contributor Author

sdcb commented Jun 19, 2023

After checked these details, but still no, I can't write a cmake usage file because still not make sense to me, upstream document also seems lack of these details: https://github.com/PaddlePaddle/FlyCV/wiki/Quick-Start

Can we merge without a usage since .lib/.dll already compiled and Visual Studio is working fine?

@jimwang118 jimwang118 added the info:reviewed Pull Request changes follow basic guidelines label Jun 20, 2023
@sdcb
Copy link
Contributor Author

sdcb commented Jun 21, 2023

@jimwang118 @dg0yt I already added usage, please check and review.

@jimwang118 jimwang118 removed the info:reviewed Pull Request changes follow basic guidelines label Jun 21, 2023
@jimwang118
Copy link
Contributor

@jimwang118 @dg0yt I already added usage, please check and review.

The following error occurs when using the added usage.

Error		CMake Error at F:/flycv/scripts/buildsystems/vcpkg.cmake:853 (_find_package):
  By not providing "FindFLYCV.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "FLYCV", but
  CMake did not find one.

  Could not find a package configuration file provided by "FLYCV" with any of
  the following names:

    FLYCVConfig.cmake
    flycv-config.cmake

  Add the installation prefix of "FLYCV" to CMAKE_PREFIX_PATH or set
  "FLYCV_DIR" to a directory containing one of the above files.  If "FLYCV"
  provides a separate development package or SDK, be sure it has been
  installed.		F:/flycv/scripts/buildsystems/vcpkg.cmake	853	

@jimwang118 jimwang118 marked this pull request as draft June 21, 2023 07:01
@jimwang118
Copy link
Contributor

@sdcb Thanks for your PR, Is work still being done for this PR?

@sdcb
Copy link
Contributor Author

sdcb commented Jul 31, 2023

@sdcb Thanks for your PR, Is work still being done for this PR?

Actually no, I don't know how to write usage, I will delete the wrong usage file, request merge without usage.

@sdcb sdcb marked this pull request as ready for review July 31, 2023 01:15
@jimwang118
Copy link
Contributor

The following error occurs when testing locally.
image

@jimwang118 jimwang118 marked this pull request as draft July 31, 2023 01:56
@sdcb
Copy link
Contributor Author

sdcb commented Jul 31, 2023

Hey @jimwang118 It seems I don't have such a line in your screenshot: file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/share")

it's test good in my side:

PS C:\Users\ZhouJie> vcpkg install flycv:x64-windows --editable
Computing installation plan...
The following packages will be built and installed:
    flycv:x64-windows -> 1.2.0
Installing 1/1 flycv:x64-windows...
Building flycv:x64-windows...
-- Using cached PaddlePaddle-FlyCV-release_-v1.2.0.tar.gz.
-- Using source at C:/_/3rd/vcpkg/buildtrees/flycv/src/se_-v1.2.0-b36dbca405
-- Configuring x64-windows
-- Building x64-windows-dbg
-- Building x64-windows-rel
-- Using cached mingw-w64-i686-pkgconf-1~1.8.0-2-any.pkg.tar.zst.
-- Using cached msys2-msys2-runtime-3.4.6-1-x86_64.pkg.tar.zst.
-- Using msys root at C:/_/3rd/vcpkg/downloads/tools/msys2/6f3fa1a12ef85a6f
-- Installing: C:/_/3rd/vcpkg/packages/flycv_x64-windows/share/flycv/copyright
-- Performing post-build validation
Elapsed time to handle flycv:x64-windows: 1.2 min
Total install time: 1.2 min

@sdcb sdcb marked this pull request as ready for review July 31, 2023 03:40
@jimwang118
Copy link
Contributor

file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/share")
This line is not in your latest submitted portfile.cmake, you are checking the following.

@sdcb
Copy link
Contributor Author

sdcb commented Jul 31, 2023

file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/share") This line is not in your latest submitted portfile.cmake, you are checking the following.

Sorry I don't get it, are you talking about an issue in an old version, but current version is good?

I think my best concern is will you merge without the usage file? (Because I tested in Visual Studio it's works perfectly)

@dan-shaw
Copy link
Contributor

I think we need a working usage file for CMake users. @jimwang118 can you help?

@dan-shaw dan-shaw marked this pull request as draft August 24, 2023 23:36
@jimwang118
Copy link
Contributor

I think we need a working usage file for CMake users. @jimwang118 can you help?

The local compilation test has the following problems. I can help write cmake usage.

warning: /debug/share should not exist. Please reorganize any important files, then use
file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/debug/share")
warning: There should be no absolute paths, such as the following, in an installed package:
    F:\flycv\packages\flycv_x64-windows
    F:\flycv\installed
    F:\flycv\buildtrees\flycv
    F:\flycv\downloads
Absolute paths were found in the following files:
    F:\flycv\packages\flycv_x64-windows\debug\share\flycv\flycv_sharedConfig-debug.cmake
    F:\flycv\packages\flycv_x64-windows\share\flycv\flycv_sharedConfig-release.cmake
error: Found 2 post-build check problem(s). To submit these ports to curated catalogs, please first correct the portfile: F:\flycv\ports\flycv\portfile.cmake

@jimwang118
Copy link
Contributor

Thanks for your PR, Is work still being done for this PR?

@sdcb
Copy link
Contributor Author

sdcb commented Nov 17, 2023

@jimwang118 sadly nop, hmm maybe you should close this PR😂?

@jimwang118
Copy link
Contributor

@jimwang118 sadly nop, hmm maybe you should close this PR😂?

Then you can just close the PR.

@sdcb sdcb closed this Nov 18, 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!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants