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

[libcurl-simple-https] New port #22917

Merged
merged 46 commits into from
Feb 22, 2022
Merged

Conversation

SamuelMarks
Copy link
Contributor

Describe the pull request

Very simple HTTPS interface built atop libcurl

https://github.com/SamuelMarks/curl-simple-https

  • What does your PR fix?

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

All

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/

Possibly still a draft, need to see what your CI says

…y exports targets… and renames to dashes not underscore)
…kg x-add-version --all --overwrite-version` + manually remove yesterdays
@SamuelMarks
Copy link
Contributor Author

Actually don't merge this yet, getting weirdness on the find_package end. Without find_package it links fine with my package (cauthflow). With find_package(libcurl-simple-https CONFIG REQUIRED) I get:

Could not find a package configuration file provided by
  "libcurl-simple-https" with any of the following names:

    libcurl-simple-httpsConfig.cmake
    libcurl-simple-https-config.cmake

Even though:

vcpkg (project0) $ fd -HIFecmake 'libcurl-simple-httpsConfig'
buildtrees/libcurl-simple-https/x64-osx-dbg/libcurl-simple-httpsConfig.cmake
buildtrees/libcurl-simple-https/x64-osx-dbg/libcurl-simple-httpsConfigVersion.cmake
buildtrees/libcurl-simple-https/x64-osx-rel/libcurl-simple-httpsConfig.cmake
buildtrees/libcurl-simple-https/x64-osx-rel/libcurl-simple-httpsConfigVersion.cmake

(and it found my other vcpkg dependencies, it's just this package…)

@dg0yt
Copy link
Contributor

dg0yt commented Feb 5, 2022

set(CMAKE_FIND_DEBUG_MODE 1)

@JackBoosY JackBoosY added the category:new-port The issue is requesting a new library to be added; consider making a PR! label Feb 6, 2022
@SamuelMarks
Copy link
Contributor Author

@dg0yt Ok thanks… so with that enabled I was able to find that manifest mode is the issue. I have a vcpkg.json in my root.

With manifest mode enables I still get this:

The package libcurl-simple-https provides CMake targets:

    find_package(libcurl-simple-https CONFIG REQUIRED)
    target_link_libraries(main PRIVATE libcurl-simple-https libcurl-simple-https_compiler_flags)

(even double checked by ./vcpkg remove libcurl-simple-https first from my vcpkg root)

However fd -HIFecmake 'libcurl-simple-httpsConfig' shows nothing from my project root (only shows in my vcpkg root when I ./vcpkg install libcurl-simple-https)

What am I doing wrong? - E.g., this line of my CMakeLists.txt:

install(EXPORT "${LIBRARY_NAME}Targets" DESTINATION "share/${LIBRARY_NAME}")

@dg0yt
Copy link
Contributor

dg0yt commented Feb 6, 2022

I am unable to follow. Maybe it is too early to integrate into vcpkg.

@SamuelMarks
Copy link
Contributor Author

@dg0yt Possibly. Anyway this project is the simplest non-header-only C89 CMake project I have. So once this issue is resolved then you'll see a lot more [& working] ports being contributed from me.

There are two CMakeLists.txt one .c and one .h, along with a vcpkg.json:

|-- CMakeLists.txt
|-- libcurl-simple-https
|   |-- CMakeLists.txt
|   |-- curl_simple_https.c
|   `-- curl_simple_https.h
`-- vcpkg.json

Everything else is boilerplate, primarily from the official CMake tutorial.

Why is this failing? - Thanks for any insight

@dg0yt
Copy link
Contributor

dg0yt commented Feb 6, 2022

Anyway this project is the simplest non-header-only C89 CMake project I have. So once this issue is resolved then you'll see a lot more [& working] ports being contributed from me.

C89 seems to be quite the opposite direction to modern C++. I have no benefit from the fact that a package supports this old standard.
And ports which are working now are still a technical debt when updating their dependencies. (And I did work on port curl.)

Everything else is boilerplate, primarily from the official CMake tutorial.

No, it isn't. It does a lot of non-standard things. (Hint: Many relevant lines contain "${PROJECT). To be fair, many CMake projects are more complex than necessary.

along with a vcpkg.json

Assuming that we talk about building the port, remove it (with a command in the portfile).
Check the logs for what is build and installed, and where to.

@SamuelMarks
Copy link
Contributor Author

For reuse as boilerplate across my decent number of C projects, I took https://gitlab.kitware.com/cmake/cmake/-/blob/master/Help/guide/tutorial/Complete and:

Following your advice @dg0yt from the #22930 pull request I added the aforementioned INSTALL(EXPORT line.

I'm going to try without manifest mode and see how far I get…

Any other tips would be greatly appreciated

…kg x-add-version --all --overwrite-version` + manually remove yesterdays
…_file` with `INSTALL_DESTINATION` from `"lib/cmake/${PROJECT_NAME}"` to `"share/${PROJECT_NAME}"`
@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Feb 21, 2022
@vicroms vicroms merged commit daa7215 into microsoft:master Feb 22, 2022
@dg0yt
Copy link
Contributor

dg0yt commented Feb 23, 2022

Personally I think packages should have some relevance before being accepted into the main repo.

@sfhacker
Copy link

Perhaps someone has requested this addition.

@SamuelMarks
Copy link
Contributor Author

It's a convenient interface to libcurl?

Even this month this was posted https://daniel.haxx.se/blog/2022/02/02/curl-dash-dash-json

So it's not like they aren't thinking about it [officially]…

@mathisloge
Copy link
Contributor

Personally I think packages should have some relevance before being accepted into the main repo.

think so too. Such just-created projects are often abandoned by the maintainer and are then in the official repo and carry a lot of dept. Especially when you consider that everything always has to be built, projects that are possibly only used by one person or no one can disrupt an important library such as curl in the update process. IMHO those are good candidates for custom registries. #17540 (comment)

@SamuelMarks
Copy link
Contributor Author

First commit: Jul 23, 2021

Albeit not used by projects I'm not involved in, but this is a dependency for:

If there was some better metric for acceptance, happy to follow. E.g, a certain level of test and documentation coverage, cross-platform support, &etc.

@mathisloge
Copy link
Contributor

This is just my personal opinion and not specific to this project.
But all these projects you list belong to your own organisation and would work even if they were used in a custom registry, and this will not prevent anyone from using your projects either. and if, for example, you realise at some point that you no longer want to support them, that's no problem either, because they don't have to be maintained in the vcpkg universe.

But yeah. This is a decision of the vcpkg team.

@SamuelMarks
Copy link
Contributor Author

Yes you are right that these would all work just fine in a custom registry. But that's not my question. My question is what metrics do you use to decide betwixt the two?

The primary advantages of having them in vcpkg itself are: review by vcpkg staff; CI access; advertising (when I was looking for UI frameworks I used grep and ls on your ports tree to find them). So would be good not to lose these advantages.

Metrics which might be what you're looking for to decide whether to accept them into vcpkg are, from the top of my head:

  • Cross-platform support;
  • Cross-architecture support (esp. for Windows);
  • Open-source license (and which one);
  • Test coverage;
  • Documentation coverage.

Fluffier metrics like number of tweets, blogposts, GitHub stars and the like are all gameable… so would prefer to stick to more objective measures (assuming you'd be open to that).

@mathisloge
Copy link
Contributor

mathisloge commented Feb 23, 2022

I think it would be good to have a metric for survivablity.
E.g. How likely is it, that the project would be maintained in the future.
So:

  • founded support of companies
  • number of contributors
  • stars could very well be a guidance: if the maintainer sometime decide to not support the project, some of those stargazers might jump in. So the likelyhood of survival is a bit better

Test coverage etc. are not that indicative IMHO since those need to be analyzed very closly to say anything about the quality of the coverage.

But I do understand your argument of the visibilty. And there should be some discovery mechanism provided by vcpkg to list available custom registries.

@dg0yt
Copy link
Contributor

dg0yt commented Feb 24, 2022

The primary advantages of having them in vcpkg itself are: review by vcpkg staff; CI access; advertising

I do understand the advantages on your side. But I also see that your PRs consume limited review capacity, even solving "upstream" (i.e. your) problems with plain CMake project which are entirely unrelated to vcpkg. In the meantime I wait days and weeks for reviews, have to re-explain basics, and re-resolve merge conflicts. So your benefits come at a price.
And I all I want that there is a conciousness for these side effects.

BillyONeal added a commit to BillyONeal/vcpkg that referenced this pull request Feb 24, 2022
84: Pass StartDate and EndDate to Get-MergedPullRequests instead of special casing only Credentials
109+130: Fix calculating negative progress caused by seeing PRs in the future because GitHub returns times in UTC
268: Actually make filtering for infrastructure PRs functional
303: Fix extraction of port-version from vcpkg.json. This really should be handled by diffing baseline.json in the future.
374-382: Fix expression ordering and casting problem which prevented non-CONTROL ports from ever being considered "new"

Also skip emitting headers for empty blocks, update and sort current triplet list, and move infrastructure bits below
port modifications.

Example output:

vcpkg (2022.02.19 - 2022.02.22)
---
#### Total port count:
#### Total port count per triplet (tested):
|triplet|ports available|
|---|---|
|x86-windows|NUM|
|**x64-windows**|NUM|
|x64-windows-static|NUM|
|x64-windows-static-md|NUM|
|x64-uwp|NUM|
|arm64-windows|NUM|
|arm-uwp|NUM|
|**x64-osx**|NUM|
|**x64-linux**|NUM|
<details>
<summary><b>The following 2 ports have been added:</b></summary>

|port|version|
|---|---|
|[libcurl-simple-https](microsoft#22917
|[triton](microsoft#23111

</details>
<details>
<summary><b>The following 18 ports have been updated:</b></summary>

- polyhook2 `2022-02-06#0` -> `2022-02-21#0`
    - [(microsoft#23203)](microsoft#23203) [polyhook2] Update to latest (2022-02-21) (by @acidicoala)
- graphviz `2.49.1#1` -> `2.49.1#2`
    - [(microsoft#23148)](microsoft#23148) [graphviz] Fix tools (by @Ace314159)
- itk `5.1.0#7` -> `5.2.1#0`
    - [(microsoft#23158)](microsoft#23158) [ITK] update to v5.2.1 (by @Adela0814)
- qhull `8.0.2#2` -> `8.0.2#3`
    - [(microsoft#23129)](microsoft#23129) [qhull] Fix copyright, pc files, cmake usage (by @dg0yt)
- tgui `2021-04-19#2` -> `2021-04-19#3`
    - [(microsoft#23211)](microsoft#23211) [tgui] fix absolute paths (by @autoantwort)
- json-dto `0.3.0#0` -> `0.3.1#0`
    - [(microsoft#23224)](microsoft#23224) [json-dto] Update to 0.3.1 (by @eao197)
- leveldb `1.22#4` -> `1.22#5`
    - [(microsoft#23180)](microsoft#23180) [leveldb] Fix homepage (by @MarcoFalke)
- openxr-loader `1.0.22#0` -> `1.0.22#1`
    - [(microsoft#23191)](microsoft#23191) [openxr-loader] Fix build failure in world rebuild CI. (by @Hoikas)
- ngspice `35#1` -> `35#2`
    - [(microsoft#23151)](microsoft#23151) [ngspice] Fix error C2065 (by @Cheney-W)
- openmvg `2.0#1` -> `2.0#2`
    - [(microsoft#23114)](microsoft#23114) [cereal] Update to 1.3.1 (by @mapret)
- lazy-importer `2021-10-23#0` -> `2022-02-09#0`
    - [(microsoft#23192)](microsoft#23192) [lazy-importer] Update to 2022-02-09 (by @Thomas1664)
- gstreamer `1.19.2#3` -> `1.19.2#4`
    - [(microsoft#23125)](microsoft#23125) [gstreamer] Support arm-windows and add features (by @JackBoosY)
- charls `2.2.0#2` -> `2.3.4#0`
    - [(microsoft#23189)](microsoft#23189) [charls] Update to 2.3.4 (by @Thomas1664)
- arrow `7.0.0#0` -> `7.0.0#1`
    - [(microsoft#23188)](microsoft#23188) [arrow] add plasma support for non Windows platforms (by @fran6co)
- trantor `1.5.4#0` -> `1.5.5#0`
    - [(microsoft#23182)](microsoft#23182) [trantor] Update to 1.5.5 (by @an-tao)
- capstone `4.0.2#2` -> `4.0.2#3`
    - [(microsoft#23122)](microsoft#23122) [capstone] Use static runtime if capstone wants to be statically linked (by @illera88)
- gainput `1.0.0#4` -> `1.0.0#5`
    - [(microsoft#23219)](microsoft#23219) [gainput] Support Linux (by @JackBoosY)
- cereal `1.3.0#1` -> `1.3.1#0`
    - [(microsoft#23114)](microsoft#23114) [cereal] Update to 1.3.1 (by @mapret)

</details>
<details>
<summary>The following additional changes have been made to vcpkg's infrastructure:</summary>

- [(microsoft#23045)](microsoft#23045) [vcpkg docs] Update ko_KR translation (by @jnooree)
- [(microsoft#23181)](microsoft#23181) [vcpkg doc] Fixes typo in vcpkg_download_distfile.cmake (by @acd1034)

</details>
-- vcpkg team vcpkg@microsoft.com Thu, 24 February 00:29:57 -0800
ekilmer added a commit to ekilmer/vcpkg that referenced this pull request Feb 27, 2022
* master: (57 commits)
  [vcpkg-tools] update cmake and git (windows only) (microsoft#22985)
  Update vcpkg tool to 2022-02-24. (microsoft#23162)
  [vcpkg baseline] Move cspice headers (microsoft#23272)
  Fixed inaccurate Chinese words (microsoft#23179)
  [vcpkg] Add fixed changelog generator. (microsoft#23255)
  [authentication.md] Add Jenkins section (microsoft#23226)
  [vcpkg] Meson osx sysroot (microsoft#21772)
  [pkgconf] enable search for system libs on linux (microsoft#23010)
  [yasm/yasm-tool] Incorporate yasm-tool into yasm (microsoft#23218)
  [lapack-reference] Update to 3.10 (microsoft#23228)
  [skia] Arm64 for skia on osx (microsoft#23222)
  [libfido2] Update to 1.10.0 (microsoft#23241)
  [Tracy] Fixing issue where version 0.7.8 was pulling the wrong version (microsoft#23061)
  [libgpiod] Add new port. (microsoft#23221)
  [drogon] Update to 1.7.5 (microsoft#23227)
  [tinyexif] Remove from fail list. (microsoft#23163)
  [vcpkg docs][ES] Sync with English readme (microsoft#19834) (microsoft#22618)
  [vcpkg baseline][libao] Disable dlfcn check under windows (microsoft#23235)
  [OpenCV] upgrade to v4.5.5 (microsoft#22801)
  [libcurl-simple-https] New port (microsoft#22917)
  ...
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.

None yet

7 participants