Skip to content

[picohttpparser] Add new port, devendor from [cinatra]#51743

Merged
vicroms merged 2 commits into
microsoft:masterfrom
vicroms:ports/picohttpparser
May 13, 2026
Merged

[picohttpparser] Add new port, devendor from [cinatra]#51743
vicroms merged 2 commits into
microsoft:masterfrom
vicroms:ports/picohttpparser

Conversation

@vicroms

@vicroms vicroms commented May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Required by #51671, created with the create-port skill in #51187.

Adds a port for https://github.com/h2o/picohttpparser.

The cinatra port vendors an extensively modified copy of picohttpparser.h but namespaced to avoid conflicts with upstream. Both versions have been tested using a test project and are installable side-by-side.

  • Changes comply with the maintainer guide.
  • The packaged project shows strong association with the chosen port name. Check this box if at least one of the following criteria is met:
    • The project is in Repology: https://repology.org/project/picohttpparser/versions
    • The project is amongst the first web search results for "picohttpparser" or "picohttpparser C++". Include a screenshot of the search engine results in the PR.
      image
    • The port name follows the 'GitHubOrg-GitHubRepo' form or equivalent Owner-Project form.
  • Optional dependencies of the build are all controlled by the port. A dependency is controlled if it is declared an unconditional dependency in vcpkg.json, or explicitly disabled through patches or build system arguments such as CMAKE_DISABLE_FIND_PACKAGE_Xxx or VCPKG_LOCK_FIND_PACKAGE
  • 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 brief and accurate. See adding-usage for context. Don't add a usage file if the automatically generated usage is correct.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Exactly one version is added in each modified versions file.

@vicroms

vicroms commented May 13, 2026

Copy link
Copy Markdown
Member Author

AI review:

Port Audit Report — picohttpparser

PR: #51743 | Triplet: x64-windows | Result: ✅ Installed successfully

Key Findings

  1. License (low): Upstream is dual-licensed MIT/Perl but only MIT is declared. This is acceptable (choosing MIT), but MIT OR Artistic-2.0 would be more precise.

  2. Copyright file is noisy: vcpkg_install_copyright uses picohttpparser.h (the full header with C code), since upstream has no standalone LICENSE file. Functionally correct but cosmetically messy.

  3. Shared library risk: ONLY_STATIC_LIBRARY is gated behind VCPKG_TARGET_IS_WINDOWS only. The library lacks visibility/dllexport macros. Shared builds likely work on Linux (ELF default visibility) but this should be verified — or make it unconditionally static.

  4. Verify version-date: 2026-04-06 should match the actual commit date of f4d94b48.

No Issues Found

  • Vendored deps: picotest submodule exists but is test-only and not built/installed ✅
  • Optional dependency risks: None — it's a zero-dependency single-file library ✅

@vicroms

vicroms commented May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

The cinatra port installs a vendored copy of this library in include/cinatra/picohttpparser.h. This is what Copilot had to say about that:

Regarding the cinatra devendoring task — I analyzed cinatra's vendored picohttpparser.h and it's heavily modified from upstream:

  • Wrapped in cinatra::detail namespace
  • Uses cinatra::http_header (with std::string_view members) instead of upstream's struct phr_header (with const char* + size_t)
  • phr_parse_request has extra parameters (has_connection, has_close, has_upgrade, has_query)
  • Custom SIMD paths (CINATRA_SSE, CINATRA_AVX2, CINATRA_ARM_OPT)
  • All functions are inline (header-only) vs upstream's compiled .c file

This is essentially a fork, not a simple vendored copy. Replacing it with our picohttpparser port would require significant API adapter work in cinatra

@vicroms

vicroms commented May 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

I asked Copilot to test both libraries linked to a test project:

Here's the summary:

✅ No linkage conflicts — both libraries coexist successfully when used from separate translation units and linked into the same binary.

⚠️ Include guard collision — both picohttpparser.h and cinatra/picohttpparser.h use #ifndef picohttpparser_h, so they cannot be #included in the same .cpp file. Whichever is included first wins; the other is silently skipped.

Why it works across translation units:

  • Cinatra's fork is entirely inline functions in cinatra::detail namespace — no exported symbols
  • Upstream picohttpparser is a compiled C library with C-linkage symbols (phr_parse_request, etc.)
  • Different namespaces + different linkage = no symbol collisions

EDIT: Patched cinatra to avoid the include guard collision in c998a79, tested both libraries can coexist using a test project.

@vicroms
[picohttpparser] Add new port
7c70318 
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@vicroms vicroms force-pushed the ports/picohttpparser branch from 3f9b27a to 7c70318 Compare May 13, 2026 22:01
@vicroms
[picohttpparser] Add new port; [cinatra] Fix picohttpparser include g…
c998a79 
…uard collision

- Add picohttpparser port (version-date 2026-04-06) with custom CMakeLists.txt
- Patch cinatra vendored picohttpparser.h to use unique include guard
  (cinatra_picohttpparser_h) to avoid collision with upstream library

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
BillyONeal
BillyONeal approved these changes May 13, 2026
View reviewed changes
Comment thread ports/cinatra/fix-picohttpparser-include-guard.patch
@@ -0,0 +1,27 @@
From dfdd983a4daed8a12c0283e579d8d2e31a2504fc Mon Sep 17 00:00:00 2001

@BillyONeal BillyONeal May 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we have usually tried to not include this email header part

@vicroms vicroms May 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This header was auto-generated by git format-patch -o to avoid Copilot doing whatever it does to line endings that corrupt the patch when we use git diff, but maybe it has gotten better about that lately?

Comment thread ports/picohttpparser/usage
picohttpparser provides CMake targets:

find_package(unofficial-picohttpparser CONFIG REQUIRED)
target_link_libraries(main PRIVATE unofficial::picohttpparser::picohttpparser)

@BillyONeal BillyONeal May 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Considering this thing appears to be header only and we aren't actually using this target in cinatra, should we even add it?

@vicroms vicroms May 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Is not header-only, the cinatra version was modified to be header-only but the upstream library has a separate .c file.

The usage file could suggest find_path + find_library instead of the unofficial targets.

@vicroms vicroms mentioned this pull request May 13, 2026
Merged
7 tasks
@BillyONeal BillyONeal changed the title [picohttpparser] Add new port [picohttpparser] Add new port, devendor from [cinatra] May 13, 2026
@vicroms vicroms merged commit 14da8d9 into microsoft:master May 13, 2026
16 checks passed
Hical61 added a commit to Hical61/vcpkg that referenced this pull request May 14, 2026
@Hical61
[hical61-hical] Update to version 2.6.1
db353b6 
- Update SHA512 for v2.6.1 release
- Add picohttpparser dependency (port merged in microsoft#51743)
- Pass -DHICAL_USE_SYSTEM_PICOHTTPPARSER=ON
- Remove boost-beast dependency (replaced by native HTTP/WebSocket stack)
dg0yt
dg0yt reviewed May 14, 2026
View reviewed changes

@dg0yt dg0yt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So it is not devendored in cinatra?

Comment thread ports/picohttpparser/portfile.cmake
Comment on lines +11 to +13
if(VCPKG_TARGET_IS_WINDOWS)
vcpkg_check_linkage(ONLY_STATIC_LIBRARY)
endif()

@dg0yt dg0yt May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I was looking for this at the top of the file.

Comment thread ports/picohttpparser/CMakeLists.txt
@@ -0,0 +1,25 @@
cmake_minimum_required(VERSION 3.14)

@dg0yt dg0yt May 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why do we add ports with 3.14 while building with 4.3?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@BillyONeal BillyONeal BillyONeal approved these changes
+1 more reviewer
@dg0yt dg0yt dg0yt left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vicroms @BillyONeal @dg0yt