Skip to content

[libfyaml] new port, [appstream] update to 1.1.2#51490

Draft
brunvonlope wants to merge 2 commits intomicrosoft:masterfrom
brunvonlope:bruno/libfyaml-test
Draft

[libfyaml] new port, [appstream] update to 1.1.2#51490
brunvonlope wants to merge 2 commits intomicrosoft:masterfrom
brunvonlope:bruno/libfyaml-test

Conversation

@brunvonlope
Copy link
Copy Markdown
Contributor

@brunvonlope brunvonlope commented May 1, 2026

Supersedes #49474


  • Changes comply with the maintainer guide.
  • SHA512s are updated for each updated download.
  • The "supports" clause reflects platforms that may be fixed by this new version, or no changes were necessary.
  • Any fixed CI baseline and CI feature baseline entries are removed from that file, or no entries needed to be changed.
  • All patch files in the port are applied and succeed.
  • 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.

@brunvonlope brunvonlope force-pushed the bruno/libfyaml-test branch 3 times, most recently from a2cc5c0 to a38f3a7 Compare May 1, 2026 15:23
@BillyONeal BillyONeal marked this pull request as draft May 1, 2026 18:35
@BillyONeal
Copy link
Copy Markdown
Member

Drafting due to build failures in the new port.

@brunvonlope brunvonlope force-pushed the bruno/libfyaml-test branch 4 times, most recently from 70c3ef2 to 778012a Compare May 1, 2026 20:34
@brunvonlope brunvonlope force-pushed the bruno/libfyaml-test branch 6 times, most recently from 3e113bd to 91a61d4 Compare May 1, 2026 22:25
@brunvonlope brunvonlope force-pushed the bruno/libfyaml-test branch 3 times, most recently from 492d07b to f41ce33 Compare May 1, 2026 23:57
@brunvonlope brunvonlope marked this pull request as ready for review May 2, 2026 00:43
@brunvonlope
Copy link
Copy Markdown
Contributor Author

@BillyONeal May be in decent shape for review now :)

Comment thread ports/appstream/portfile.cmake Outdated
Comment on lines +13 to +14
set(VCPKG_C_FLAGS "${VCPKG_C_FLAGS} -Wno-error=incompatible-pointer-types")
set(VCPKG_CXX_FLAGS "${VCPKG_CXX_FLAGS} -Wno-error=incompatible-pointer-types")
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.

Usually we want to get rid of -Werror.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could you rephrase? I did not get it

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.

AFAIU these lines disable -Wno-error for a particular warning. I see that as a symptom of -Werror being used during the port build. (Correct me if I'm wrong.)

In general, it is not desired to have -Werror in port builds. That flags is a helpful tool in controlled environments (i.e. upstream CI, or actual developer). But it is breaking builds too easily when toolchains, targets and time vary over a wide range.

So I suggest to check the buildsystem and logs for signs of -Werror, and to disable that flag.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

appstream uses a lot of -Werror on the main meson.build. I tried passing -Wno-error VCPKG_C*_FLAGS and with meson build options but no success.

The only one that makes to build is disabling -Wno-error=incompatible-pointer-type specifially.

Comment thread ports/libfyaml/portfile.cmake Outdated
Comment thread ports/libfyaml/pthread.diff Outdated
Comment thread ports/libfyaml/pthread.diff Outdated
Comment thread ports/libfyaml/pthread.diff Outdated
Comment thread ports/libfyaml/pthread.diff Outdated
Comment thread ports/libfyaml/portfile.cmake Outdated
@brunvonlope brunvonlope force-pushed the bruno/libfyaml-test branch 5 times, most recently from 20a4646 to dc5a7d2 Compare May 4, 2026 22:33
Needed for appstream port
@brunvonlope brunvonlope force-pushed the bruno/libfyaml-test branch from 48b015e to 483ad2b Compare May 5, 2026 11:05
Copy link
Copy Markdown
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I believe GPT 5.4 is correct about this:

The main review findings are bundled third-party code with likely incomplete license metadata and host-dependent optional components that are not explicitly controlled by the port.

Declared Metadata

  • License: MITports\libfyaml\vcpkg.json:6

Build Invocation Summary

  • Primary build helper(s): vcpkg_cmake_configure, vcpkg_cmake_install, vcpkg_fixup_pkgconfig, vcpkg_copy_tools, vcpkg_cmake_config_fixupports\libfyaml\portfile.cmake:31-44
  • Key options: -DBUILD_TESTING=OFFports\libfyaml\portfile.cmake:31-35

Vendored Dependencies

  • Bundled component: xxHash

    • Evidence: buildtrees\libfyaml\src\...\src\xxhash\xxhash.c; compiled into fyaml via CMakeLists.txt:869; BSD-2-Clause notice in src\xxhash\xxhash.h:9-35
    • Status: installed as part of the shipped library
    • Assessment: bundled third-party code is used in the library, but it is not modeled in ports\libfyaml\vcpkg.json
  • Bundled component: Windows getopt

    • Evidence: CMakeLists.txt:1246-1249 adds src\getopt\getopt.c on Windows; fy-tool is installed via CMakeLists.txt:1684-1690 and copied by ports\libfyaml\portfile.cmake:42; BSD-3-Clause text in src\getopt\LICENSE.getopt
    • Status: installed indirectly in the shipped fy-tool.exe
    • Assessment: bundled Windows-only dependency, also not modeled in metadata
  • Bundled component: BLAKE3

    • Evidence: CMakeLists.txt:877-907,897 compiles src\blake3\* into fyaml and installs include\libfyaml\libfyaml-blake3.h; installed package contains include\libfyaml\libfyaml-blake3.h
    • Status: installed as part of the shipped library and public headers
    • Assessment: bundled code is definitely shipped; its third-party notice coverage should be reviewed

Optional Dependency Risks

  • Dependency / feature: libyaml

    • Evidence: upstream auto-detects it with find_package(yaml QUIET CONFIG) / pkg_check_modules(LIBYAML yaml-0.1)buildtrees\libfyaml\src\...\CMakeLists.txt:210-227
    • Why it is risky: ports\libfyaml\vcpkg.json does not declare yaml, and ports\libfyaml\portfile.cmake does not explicitly disable or gate it. If present on the host, it changes the build by enabling libfyaml-parserCMakeLists.txt:1300-1335
    • Suggested packaging change: explicitly disable this path or add a feature/dependency for it
  • Dependency / feature: Sphinx

    • Evidence: upstream does find_program(SPHINX_EXECUTABLE) and conditionally installs generated manpages — CMakeLists.txt:1592-1654; otherwise it installs a canned fallback manpage — CMakeLists.txt:1665-1680
    • Why it is risky: installed content varies with host tooling; builders with sphinx-build can install extra manpages that are absent otherwise
    • Suggested packaging change: patch docs off, or force the canned-manpage path for reproducible packaging

License / Installed Content Findings

  • Finding: declared MIT appears incomplete for the shipped contents

    • Declared license: MIT
    • Observed installed content: packages\libfyaml_x64-windows\bin\fyaml.dll, packages\libfyaml_x64-windows\tools\libfyaml\fy-tool.exe, packages\libfyaml_x64-windows\share\libfyaml\copyright
    • Evidence: installed copyright is only upstream MIT text; bundled xxHash carries BSD-2-Clause text (src\xxhash\xxhash.h:9-35), and bundled Windows getopt carries BSD-3-Clause text (src\getopt\LICENSE.getopt)
    • Assessment: the port likely needs a broader SPDX expression and/or bundled third-party notices in the installed copyright
  • Finding: bundled BLAKE3 code is shipped but not obviously covered by installed notices

    • Declared license: MIT
    • Observed installed content: packages\libfyaml_x64-windows\include\libfyaml\libfyaml-blake3.h
    • Evidence: src\blake3\* is compiled into fyaml and the public BLAKE3 header is installed — CMakeLists.txt:877-907,897
    • Assessment: verify upstream licensing for the src\blake3 subtree and include any required attribution in the package metadata/notices

Other Port Review Suggestions

  • Suggestion: consider feature-gating the CLI tool
    • Evidence: fy-tool is always installed by upstream (CMakeLists.txt:1684-1690) and always copied by the port (ports\libfyaml\portfile.cmake:42)
    • Rationale: if the intended default is “library-only”, a tools feature would reduce package surface and avoid always shipping the extra executable

Recommended Follow-ups

  1. Audit the bundled xxhash, getopt, and blake3 subtrees and update license / installed notices accordingly.
  2. Make libyaml detection explicit: either disable it or model it as a feature with a declared dependency.
  3. Remove host-tool variance from docs packaging by explicitly disabling Sphinx-generated docs or forcing the canned-manpage path.

@BillyONeal BillyONeal marked this pull request as draft May 5, 2026 23:32
@brunvonlope brunvonlope force-pushed the bruno/libfyaml-test branch from 483ad2b to 8c5df4b Compare May 6, 2026 01:23
@brunvonlope
Copy link
Copy Markdown
Contributor Author

@BillyONeal I appreciate the effort to make a review with the chatbot but would prefer a human, non overly verbose, and actionable review

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants