diff --git a/.claude/settings.local.json b/.claude/settings.local.json new file mode 100644 index 0000000..386b5d3 --- /dev/null +++ b/.claude/settings.local.json @@ -0,0 +1,9 @@ +{ + "permissions": { + "allow": [ + "Bash(echo:*)", + "WebFetch(domain:github.com)", + "Bash(gh pr view:*)" + ] + } +} diff --git a/.github/workflows/test-integration.yml b/.github/workflows/test-integration.yml index ca4df7f..e8b37a9 100644 --- a/.github/workflows/test-integration.yml +++ b/.github/workflows/test-integration.yml @@ -26,7 +26,12 @@ jobs: sudo apt install -y meson ninja-build - name: Set VCPKG_ROOT - run: echo "VCPKG_ROOT=$(dirname $(realpath $(which vcpkg)))" >> $GITHUB_ENV + run: | + if [ -n "$VCPKG_INSTALLATION_ROOT" ]; then + echo "VCPKG_ROOT=$VCPKG_INSTALLATION_ROOT" >> $GITHUB_ENV + else + echo "VCPKG_ROOT=$(dirname $(realpath $(which vcpkg)))" >> $GITHUB_ENV + fi - name: Test Meson Package run: pwsh -File ./tests/integration/packaging/test_meson.ps1 diff --git a/.github/workflows/workflow-release.yml b/.github/workflows/workflow-release.yml index 4d2ccb2..54c545c 100644 --- a/.github/workflows/workflow-release.yml +++ b/.github/workflows/workflow-release.yml @@ -20,3 +20,21 @@ jobs: needs: [test-integration] name: Create Release uses: ./.github/workflows/create-release.yml + + publish-vcpkg-branch: + needs: [release] + name: Publish vcpkg Branch + runs-on: ubuntu-latest + steps: + - name: Checkout repository + uses: actions/checkout@v5 + + - name: Configure git identity + run: | + git config --global user.name "github-actions[bot]" + git config --global user.email "github-actions[bot]@users.noreply.github.com" + # Embed token so git push to an-dr/vcpkg works without interactive auth + git config --global url."https://x-access-token:${{ secrets.VCPKG_PR_TOKEN }}@github.com/".insteadOf "https://github.com/" + + - name: Publish vcpkg branch + run: pwsh -File ./scripts/publish_vcpkg_branch.ps1 diff --git a/Code Review - 2026-02-25.md b/Code Review - 2026-02-25.md new file mode 100644 index 0000000..ec10ffe --- /dev/null +++ b/Code Review - 2026-02-25.md @@ -0,0 +1,84 @@ +# Code Review - 2026-02-25 + +Branch: `fix/winarm64_vs_processing` → `main` +Commit: `829dfcc Fix va handling on Windows on ARM causing crashes in some configurations` +Scope: `src/ulog.c` — 20 lines changed + +--- + +## 🎯 Summary + +Three targeted fixes for `va_list` misuse that caused crashes on Windows ARM64 (MSVC). The changes cover `ulog_log`, `ulog_event_get_message`, and `ulog_event_to_cstr`. Code is correct and focused; no unnecessary scope creep. + +--- + +## ⚠️ Critical Issues + +None. + +--- + +## 🔧 High Priority + +None. + +--- + +## 🚀 Modernization Opportunities + +Not applicable — this is C with platform-portability constraints. + +--- + +## 💡 Improvements + +**[src/ulog.c:335, 1773]** — Comment slightly overstates cost predictability + +The comment `"~4-5 word moves"` is inaccurate when optional features are compiled in. With `ULOG_HAS_TOPICS`, `ULOG_HAS_TIME`, and `ULOG_HAS_SOURCE_LOCATION` all enabled, `sizeof(ulog_event)` grows to ~8–10 words (2 `int`s, 3–4 pointers, plus `va_list`). The claim is true for a minimal build but misleading for a full one. + +- Better: `"memcpy of a small stack struct; vsnprintf below dominates"` — or just drop the word count. + +**[src/ulog.c:336, 1774]** — Zero-init before `memcpy` is redundant + +```c +ulog_event ev_copy = {0}; +memcpy(&ev_copy, ev, sizeof(ulog_event)); +``` + +`memcpy` overwrites every byte anyway, so the `= {0}` buys nothing. It's not wrong, but it creates a false impression that the zeroing matters for correctness. The only byte that is not covered by `memcpy` but needs initialization is `va_list` — and that is handled by `va_copy` immediately after. + +If the intent is defensive clarity, a comment explaining why would be better than silent double-initialization. + +--- + +## 📝 Technical Debt + +**[src/ulog.c — va_list in struct]** — Root fragility: `va_list` stored inside a struct + +Storing a `va_list` inside `ulog_event` is the underlying source of all three fixes. The C standard permits it, but it creates a recurring friction point: + +- Struct copy of a `va_list`-containing struct is always risky (hence the `memcpy` change). +- `va_start` directly into a struct member is non-portable (hence the intermediate `args` change). +- Every new call site must remember to `va_copy`/`va_end` correctly. + +Future work: Consider storing the formatted string directly in `ulog_event` (fixed-size buffer or pointer to caller-owned buffer) instead of keeping the raw `va_list` alive. This would eliminate all `va_list` lifecycle concerns from the event struct at the cost of one early `vsnprintf` call. + +--- + +## ✅ Positives + +- **`ulog_log` fix is textbook-correct.** Using an intermediate `va_list`, copying with `va_copy`, then immediately calling `va_end` on the source is exactly the right pattern. It also properly calls `va_end(args)` which the original pattern sidestepped. + +- **Consistent fix across all three sites.** Both `ulog_event_get_message` and `ulog_event_to_cstr` received the identical treatment, preventing latent bugs in the other paths. + +- **No behavior change for correct platforms.** The fix is purely mechanical; there is no logic change, making regression risk minimal. + +- **Branch name and commit message are accurate and descriptive.** Easy to bisect later. + +--- + +## 🏁 Verdict + +- [x] Approved — ship it + +The fixes are correct, minimal, and well-scoped. The improvements noted above are cosmetic (comment wording, redundant zero-init) and do not block merge. The technical debt item is a pre-existing design choice, not introduced by this PR. diff --git a/ports/microlog/portfile.cmake b/ports/microlog/portfile.cmake index 0effdb1..e28d6c0 100644 --- a/ports/microlog/portfile.cmake +++ b/ports/microlog/portfile.cmake @@ -21,7 +21,4 @@ file(REMOVE_RECURSE "${CURRENT_PACKAGES_DIR}/lib") # Remove the LICENSE that cmake installed to prefix root (replaced by vcpkg_install_copyright) file(REMOVE "${CURRENT_PACKAGES_DIR}/LICENSE") -# Install the usage file -file(INSTALL "${CMAKE_CURRENT_LIST_DIR}/usage" DESTINATION "${CURRENT_PACKAGES_DIR}/share/${PORT}") - vcpkg_install_copyright(FILE_LIST "${SOURCE_PATH}/LICENSE") diff --git a/ports/microlog/usage b/ports/microlog/usage deleted file mode 100644 index 54e9706..0000000 --- a/ports/microlog/usage +++ /dev/null @@ -1,24 +0,0 @@ -microlog provides CMake targets: - - find_package(microlog CONFIG REQUIRED) - target_link_libraries(main PRIVATE microlog::microlog) - -The library compiles ulog.c into your project at build time. -Optional compile definitions (pass via target_compile_definitions): - - ULOG_BUILD_COLOR=1 # ANSI color output - ULOG_BUILD_TIME=1 # timestamp in each log line - ULOG_BUILD_SOURCE_LOCATION=0 # disable file:line prefix (default: 1) - ULOG_BUILD_LEVEL_SHORT=1 # short level tags: D/I/W/E (default: DEBUG/INFO/…) - ULOG_BUILD_PREFIX_SIZE=64 # enable custom per-output prefix (bytes; 0=off) - ULOG_BUILD_EXTRA_OUTPUTS=4 # number of extra output handlers (0=off) - ULOG_BUILD_WARN_NOT_ENABLED=0 # suppress stub warnings for disabled features (default: 1) - ULOG_BUILD_TOPICS_MODE= # enable topics - ULOG_BUILD_TOPICS_MODE_OFF/ ULOG_BUILD_TOPICS_MODE_STATIC / ULOG_BUILD_TOPICS_MODE_DYNAMIC - ULOG_BUILD_TOPICS_STATIC_NUM=8 # number of static topics (requires TOPICS_MODE=STATIC) - ULOG_BUILD_DYNAMIC_CONFIG=1 # runtime feature toggle API (overrides other flags) - ULOG_BUILD_DISABLED=1 # replace all log calls with ((void)0) no-ops - ULOG_BUILD_CONFIG_HEADER_ENABLED=1 # load settings from ulog_config.h instead of defines - -# Full documentation: https://github.com/an-dr/microlog -# Check optional library extentions here: https://github.com/an-dr/microlog/tree/main/extensions diff --git a/scripts/build_vcpkg_port.ps1 b/scripts/build_vcpkg_port.ps1 index aaf3ee9..d361ead 100644 --- a/scripts/build_vcpkg_port.ps1 +++ b/scripts/build_vcpkg_port.ps1 @@ -5,7 +5,7 @@ .DESCRIPTION Downloads the GitHub source tarball for a given tag, computes its SHA512, - and writes a ready-to-use portfile.cmake + vcpkg.json + usage into dist/vcpkg-port/. + and writes a ready-to-use portfile.cmake + vcpkg.json into dist/vcpkg-port/. Can be run locally or from CI. @@ -87,7 +87,4 @@ $OverlayJson = Get-Content (Join-Path $RepoRoot "ports/microlog/vcpkg.json") -Ra $RegistryJson = $OverlayJson -replace '"version":\s*"[^"]*"', "`"version`": `"$Version`"" Set-Content -Path (Join-Path $OutDir "vcpkg.json") -Value $RegistryJson -NoNewline -# usage file (copy from overlay port verbatim) -Copy-Item (Join-Path $RepoRoot "ports/microlog/usage") (Join-Path $OutDir "usage") - Write-Host "Generated port in: $OutDir" diff --git a/scripts/publish_vcpkg_branch.ps1 b/scripts/publish_vcpkg_branch.ps1 new file mode 100644 index 0000000..b71301a --- /dev/null +++ b/scripts/publish_vcpkg_branch.ps1 @@ -0,0 +1,163 @@ + +<# +.SYNOPSIS + Publishes the microlog port update to the an-dr/vcpkg fork. + +.DESCRIPTION + Builds the release port artifacts, clones an-dr/vcpkg, creates a branch + named microlog/v, copies the port files, updates the version + database, commits, and pushes. No PR is opened - submit it manually. + + Requires: + - git available on PATH with push access to an-dr/vcpkg + (set GH_TOKEN or configure credentials before running) + - vcpkg bootstrapped in the cloned repo (done automatically) + +.PARAMETER Tag + The git tag of the microlog release (e.g. "v7.0.2"). + Defaults to the version in the version file. + +.PARAMETER WorkDir + Directory where the vcpkg fork will be cloned. + Defaults to a temp directory. + +.EXAMPLE + .\scripts\submit_vcpkg_pr.ps1 + .\scripts\submit_vcpkg_pr.ps1 -Tag v7.0.2 +#> + +param( + [string]$Tag, + [string]$WorkDir +) + +$ErrorActionPreference = "Stop" +Set-Location "$PSScriptRoot/.." +$RepoRoot = (Resolve-Path ".").Path + +# ── Resolve tag / version ───────────────────────────────────────────────────── +if (-not $Tag) { + $Version = (Get-Content (Join-Path $RepoRoot "version") -Raw).Trim() + $Tag = "v$Version" +} +$Version = $Tag.TrimStart("v") + +$ForkRepo = "an-dr/vcpkg" +$Branch = "microlog/v$Version" +$CommitMsg = "[microlog] Add version $Version" + +Write-Host "Tag: $Tag" +Write-Host "Version: $Version" +Write-Host "Fork: $ForkRepo" +Write-Host "Branch: $Branch" + +# ── Step 1: Build port artifacts ───────────────────────────────────────────── +Write-Host "" +Write-Host "==> Building port artifacts..." +& (Join-Path $PSScriptRoot "build_vcpkg_port.ps1") -Tag $Tag +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + +$PortSrc = Join-Path $RepoRoot "dist/vcpkg-port" + +# ── Step 2: Sync fork master with upstream ──────────────────────────────────── +# Uses the GitHub merge-upstream API so the fork is up-to-date before we branch. +# Requires GH_TOKEN (or git credential) with push access to $ForkRepo. +Write-Host "" +Write-Host "==> Syncing $ForkRepo master with microsoft/vcpkg..." +$SyncHeaders = @{ "Accept" = "application/vnd.github+json" } +if ($env:GH_TOKEN) { + $SyncHeaders["Authorization"] = "Bearer $env:GH_TOKEN" +} +$SyncBody = '{"branch":"master"}' +try { + $SyncResp = Invoke-RestMethod ` + -Method Post ` + -Uri "https://api.github.com/repos/$ForkRepo/merge-upstream" ` + -Headers $SyncHeaders ` + -ContentType "application/json" ` + -Body $SyncBody + Write-Host "Sync result: $($SyncResp.message)" +} catch { + # A 409 means master is already up-to-date - not a failure. + if ($_.Exception.Response.StatusCode.value__ -eq 409) { + Write-Host "Fork master already up-to-date." + } else { + throw + } +} + +# ── Step 3: Clone the (now-synced) fork ────────────────────────────────────── +if (-not $WorkDir) { + $WorkDir = Join-Path ([System.IO.Path]::GetTempPath()) "vcpkg-publish-$Version" +} +$VcpkgClone = Join-Path $WorkDir "vcpkg" + +Write-Host "" +Write-Host "==> Cloning $ForkRepo into $VcpkgClone ..." +if (Test-Path $VcpkgClone) { + Remove-Item $VcpkgClone -Recurse -Force +} +New-Item -ItemType Directory -Path $WorkDir -Force | Out-Null + +# Shallow clone - we only need the current tree, not full history. +git clone --depth 1 "https://github.com/$ForkRepo.git" $VcpkgClone +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + +# ── Step 4: Create branch ───────────────────────────────────────────────────── +Write-Host "" +Write-Host "==> Creating branch $Branch ..." +git -C $VcpkgClone checkout -b $Branch +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + +# ── Step 5: Copy port files ─────────────────────────────────────────────────── +$PortDst = Join-Path $VcpkgClone "ports/microlog" +Write-Host "" +Write-Host "==> Copying port to $PortDst ..." +if (Test-Path $PortDst) { Remove-Item $PortDst -Recurse -Force } +Copy-Item $PortSrc $PortDst -Recurse +git -C $VcpkgClone add "ports/microlog" + +# ── Step 6: Bootstrap vcpkg and update version database ─────────────────────── +Write-Host "" +Write-Host "==> Bootstrapping vcpkg..." +if ($IsWindows) { + $BootstrapScript = Join-Path $VcpkgClone "bootstrap-vcpkg.bat" + cmd /c $BootstrapScript -disableMetrics +} else { + $BootstrapScript = Join-Path $VcpkgClone "bootstrap-vcpkg.sh" + bash $BootstrapScript -disableMetrics +} +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + +$VcpkgExe = if ($IsWindows) { + Join-Path $VcpkgClone "vcpkg.exe" +} else { + Join-Path $VcpkgClone "vcpkg" +} + +Write-Host "" +Write-Host "==> Formatting manifest..." +& $VcpkgExe format-manifest (Join-Path $PortDst "vcpkg.json") +git -C $VcpkgClone add "ports/microlog" + +Write-Host "" +Write-Host "==> Updating version database..." +& $VcpkgExe x-add-version microlog --overwrite-version +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } +git -C $VcpkgClone add "versions/" + +# ── Step 7: Commit ──────────────────────────────────────────────────────────── +Write-Host "" +Write-Host "==> Committing..." +git -C $VcpkgClone commit -m $CommitMsg +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + +# ── Step 8: Push branch to fork ────────────────────────────────────────────── +Write-Host "" +Write-Host "==> Pushing branch to $ForkRepo ..." +git -C $VcpkgClone push origin $Branch --force-with-lease +if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE } + +Write-Host "" +Write-Host "[OK] Branch '$Branch' pushed to $ForkRepo." +Write-Host " Open a PR manually at: https://github.com/$ForkRepo/compare/$Branch" diff --git a/tests/integration/cpm/CMakeLists.txt b/tests/integration/cpm/CMakeLists.txt index 1f24f06..dba540a 100644 --- a/tests/integration/cpm/CMakeLists.txt +++ b/tests/integration/cpm/CMakeLists.txt @@ -1,16 +1,25 @@ cmake_minimum_required(VERSION 3.15) project(example_package) -execute_process( - COMMAND git rev-parse HEAD - WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} - OUTPUT_VARIABLE GIT_COMMIT_HASH - OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET) -message(STATUS "Current commit: ${GIT_COMMIT_HASH}") - include(CPM.cmake) -cpmaddpackage(NAME microlog GIT_REPOSITORY - https://github.com/an-dr/microlog.git GIT_TAG ${GIT_COMMIT_HASH}) + +# If MICROLOG_SOURCE_DIR is set, use the local source tree directly. +# This avoids fetching from a hardcoded upstream URL, so the test works +# correctly from forks and in local development without network access. +if(DEFINED MICROLOG_SOURCE_DIR) + message(STATUS "CPM: using local source at ${MICROLOG_SOURCE_DIR}") + cpmaddpackage(NAME microlog SOURCE_DIR "${MICROLOG_SOURCE_DIR}") +else() + execute_process( + COMMAND git rev-parse HEAD + WORKING_DIRECTORY ${CMAKE_SOURCE_DIR} + OUTPUT_VARIABLE GIT_COMMIT_HASH + OUTPUT_STRIP_TRAILING_WHITESPACE ERROR_QUIET) + message(STATUS "CPM: fetching microlog commit ${GIT_COMMIT_HASH}") + cpmaddpackage(NAME microlog + GIT_REPOSITORY https://github.com/an-dr/microlog.git + GIT_TAG ${GIT_COMMIT_HASH}) +endif() add_executable(example_package example.cpp) diff --git a/tests/integration/cpm/test_cpm.ps1 b/tests/integration/cpm/test_cpm.ps1 index 66396f2..868d70f 100644 --- a/tests/integration/cpm/test_cpm.ps1 +++ b/tests/integration/cpm/test_cpm.ps1 @@ -14,9 +14,13 @@ $ErrorActionPreference = "Stop" Push-Location $PSScriptRoot try { - - # Build the test package - cmake -B./build/cmake-cpm + + $RepoRoot = (Resolve-Path "$PSScriptRoot/../../..").Path + + # Build the test package. Pass the repo root so CMake can use the local + # source tree instead of fetching from a hardcoded upstream URL, which + # would fail when running from a fork. + cmake -B./build/cmake-cpm "-DMICROLOG_SOURCE_DIR=$RepoRoot" cmake --build ./build/cmake-cpm --verbose if ($LASTEXITCODE -ne 0) { Write-Host "Tests failed with exit code $LASTEXITCODE"