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

[libgourou] Add new port and its dep #28037

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

Conversation

SamuelMarks
Copy link
Contributor

Describe the pull request

New port

  • What does your PR fix?

N/A

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

Linux, macOS

Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

Yes

If you are still working on the PR, open it as a Draft: https://github.blog/2019-02-14-introducing-draft-pull-requests/

github-actions[bot]
github-actions bot previously approved these changes Nov 28, 2022
github-actions[bot]
github-actions bot previously approved these changes Nov 28, 2022
github-actions[bot]
github-actions bot previously approved these changes Nov 28, 2022
@Cheney-W Cheney-W added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Nov 28, 2022
@Cheney-W
Copy link
Contributor

Please use vcpkg_install_copyright instead of file(INSTALL ...) for copyright file.

Other cl failures can be found in the log here: https://dev.azure.com/vcpkg/public/_build/results?buildId=81584&view=artifacts&pathAsName=false&type=publishedArtifacts

…ll ; [ports/libgourou/portfile.cmake] { [{cmake/modules,utils/CMakeLists.txt}] Use regular OpenSSL `find_package` mechanism ; [utils/*.cpp] Use `std::cout` once over multiple lines }
github-actions[bot]
github-actions bot previously approved these changes Nov 28, 2022
…nstall_copyright`; [ports/updfparser/portfile.cmake] Implementing testing and disable build with tests for vcpkg
github-actions[bot]
github-actions bot previously approved these changes Nov 28, 2022
…`find_package` and `target_link_libraries` libcurl } ; [ports/{libgourou,updfparser}/vcpkg.json] Specify lack of Windows support
github-actions[bot]
github-actions bot previously approved these changes Nov 28, 2022
github-actions[bot]
github-actions bot previously approved these changes Nov 28, 2022
@SamuelMarks
Copy link
Contributor Author

@Cheney-W That should do the trick.

@@ -0,0 +1,23 @@
{
"name": "libgourou",
"version": "0.8.0",
Copy link
Contributor

Choose a reason for hiding this comment

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

The upstream repository on GitHub doesn't have any releases or tags, so where did this version come from?
I see that you are actually using the latest commit, so if there is no release or tags available, you should use "version-date": "2022-11-28" here (date of submission of this pr).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

The upstream repository on GitHub doesn't have any releases or tags, so where did this version come from?

@Cheney-W IIUC you are looking at an unofficial fork, but upstream is there:

See the tags on the left https://indefero.soutade.fr/p/libgourou/source/tree/master/

So shouldn't this port download from real upstream?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been messaging the maintainer we can wait to see if he accepts my patches I guess?- He accepted some but not all so I think this might work, but happy to try again if you push it.

@Cheney-W
Copy link
Contributor

I tried to test the usage on Linux machine with following CMakelist.txt:

cmake_minimum_required(VERSION 3.10)
project(test-project)
add_executable(main main.cpp)
find_package(libgourou CONFIG REQUIRED)
target_link_libraries(main PRIVATE gourou gourou_utils gourou_include gourou_compiler_flags)

and I got below errors:

CMake Error at offscale/scripts/buildsystems/vcpkg.cmake:582 (_add_executable):
  Target "main" links to target "pugixml::static" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:3 (add_executable)

CMake Error at offscale/scripts/buildsystems/vcpkg.cmake:582 (_add_executable):
  Target "main" links to target "pugixml::pugixml" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:3 (add_executable)

CMake Error at offscale/scripts/buildsystems/vcpkg.cmake:582 (_add_executable):
  Target "main" links to target "libzip::zip" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:3 (add_executable)

CMake Error at offscale/scripts/buildsystems/vcpkg.cmake:582 (_add_executable):
  Target "main" links to target "OpenSSL::SSL" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:3 (add_executable)

CMake Error at offscale/scripts/buildsystems/vcpkg.cmake:582 (_add_executable):
  Target "main" links to target "CURL::libcurl" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?
Call Stack (most recent call first):
  CMakeLists.txt:3 (add_executable)

For fixing above errors, I think you should add below contents into the https://github.com/SamuelMarks/libgourou/blob/cmake/cmake/Config.cmake.in file:

include(CMakeFindDependencyMacro)

find_dependency(CURL)
find_dependency(libzip)
find_dependency(OpenSSL)
find_dependency(pugixml)

@SamuelMarks
Copy link
Contributor Author

@Cheney-W Darn, it's been open for 2 years and no activity for 3 months (whence someone had built solution on their fork): https://gitlab.kitware.com/cmake/cmake/-/issues/20511

I'll make the changes you suggested & thanks

…party dependencies with `find_dependency` to Config }
@Cheney-W
Copy link
Contributor

Cheney-W commented Dec 6, 2022

@SamuelMarks Could you please apply my suggestions?

@SamuelMarks
Copy link
Contributor Author

@Cheney-W I thought I did a week ago? - Or did I miss something?

@Cheney-W
Copy link
Contributor

Cheney-W commented Dec 7, 2022

I added some comments in vcpkg.json file before, about the "version" and "supports", could you please take a look?

@Cheney-W
Copy link
Contributor

@SamuelMarks Are you still working on this PR?

github-actions[bot]
github-actions bot previously approved these changes Dec 19, 2022
Comment on lines +7 to +12
},
{
"git-tree": "c1ce55a66a9b2cbd947c9574ae85e543ae5df418",
"version": "0.8.0",
"port-version": 0
}
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
},
{
"git-tree": "c1ce55a66a9b2cbd947c9574ae85e543ae5df418",
"version": "0.8.0",
"port-version": 0
}
}

@Cheney-W Cheney-W marked this pull request as draft December 28, 2022 07:14
@Cheney-W
Copy link
Contributor

Cheney-W commented Jan 6, 2023

@SamuelMarks Are you still working on this PR?

@SamuelMarks
Copy link
Contributor Author

Yes, I thought maybe I'd add Windows support first then send it through

Otherwise it's basically ready at is

@Cheney-W
Copy link
Contributor

Cheney-W commented Jan 9, 2023

OK, please apply my suggestions by the way when you add new changes.

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.

None yet

3 participants