Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions .claude/settings.local.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
{
"permissions": {
"allow": [
"Bash(echo:*)",
"WebFetch(domain:github.com)",
"Bash(gh pr view:*)"
]
}
}
7 changes: 6 additions & 1 deletion .github/workflows/test-integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions .github/workflows/workflow-release.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
84 changes: 84 additions & 0 deletions Code Review - 2026-02-25.md
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 0 additions & 3 deletions ports/microlog/portfile.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -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")
24 changes: 0 additions & 24 deletions ports/microlog/usage

This file was deleted.

5 changes: 1 addition & 4 deletions scripts/build_vcpkg_port.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down Expand Up @@ -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"
163 changes: 163 additions & 0 deletions scripts/publish_vcpkg_branch.ps1
Original file line number Diff line number Diff line change
@@ -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<version>, 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"
27 changes: 18 additions & 9 deletions tests/integration/cpm/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -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)

Expand Down
10 changes: 7 additions & 3 deletions tests/integration/cpm/test_cpm.ps1
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down