Repository navigation
Adding windows compatibility - #96
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The Windows workflow does not run on pull requests, and the PR includes a large unrelated inactive archive.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Adds initial Windows compatibility by normalizing MSYS/MinGW/Cygwin OS detection and introducing a cross-platform FFmpeg test workflow.
Changes:
- Maps Windows POSIX environments to
windowsin the Makefile. - Adds Linux, macOS, and Windows FFmpeg testing.
- Adds unrelated inactive archival code under
_old/_old.
| File | Description |
|---|---|
Makefile |
Normalizes Windows OS detection. |
go.mod |
Marks YAML dependency indirect. |
.github/workflows/test_ffmpeg.yaml |
Adds cross-platform FFmpeg tests. |
_old/_old/source.go |
Archives source management. |
_old/_old/source_test.go |
Archives source tests. |
_old/_old/segment_audio.go |
Archives audio segmentation. |
_old/_old/segment_audio_test.go |
Archives segmentation test. |
_old/_old/metadata.go |
Archives metadata extraction. |
_old/_old/metadata_test.go |
Archives metadata tests. |
_old/_old/format.go |
Archives format listing. |
_old/_old/format_test.go |
Archives format tests. |
_old/_old/filter.go |
Archives filter listing. |
_old/_old/filter_test.go |
Archives filter tests. |
_old/_old/codec.go |
Archives codec listing. |
_old/_old/codec_test.go |
Archives codec tests. |
_old/_old/audiofingerprint.go |
Archives fingerprint generation. |
_old/_old/audiofingerprint_test.go |
Archives fingerprint test. |
_old/_old/audiofingerprint_stub.go |
Archives disabled-build stub. |
_old/_old/audiofingerprint_lookup.go |
Archives fingerprint lookup. |
_old/_old/_testcard/testcard.go |
Archives test-card source. |
_old/_old/_testcard/testcard_test.go |
Archives test-card tests. |
_old/_old/_schema/stream.go |
Archives stream schema. |
_old/_old/_schema/source.go |
Archives source interface. |
_old/_old/_schema/segment_audio.go |
Archives segmentation request. |
_old/_old/_schema/probe.go |
Archives probe schema. |
_old/_old/_schema/metadata.go |
Archives metadata schema. |
_old/_old/_schema/metadata_test.go |
Archives schema tests. |
_old/_old/_schema/format.go |
Archives format schema. |
_old/_old/_schema/filter.go |
Archives filter schema. |
_old/_old/_schema/codec.go |
Archives codec schema. |
_old/_old/_schema/audiofingerprint.go |
Archives fingerprint schema. |
_old/_old/_cmd/walk.go |
Archives filesystem walking. |
_old/_old/_cmd/templater.go |
Archives output templating. |
_old/_old/_cmd/server.go |
Archives server command. |
_old/_old/_cmd/cli.go |
Archives CLI composition. |
_old/_old/_cmd/cli_other.go |
Archives capability commands. |
_old/_old/_cmd/cli_metadata.go |
Archives metadata command. |
_old/_old/_cmd/cli_artwork.go |
Archives artwork command. |
_old/_old/_cmd/audiofingerprint_stub.go |
Archives CLI build stub. |
_old/_old/_cmd/audiofingerprint_chromaprint.go |
Archives fingerprint commands. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
workflow_dispatch requires the workflow file to exist on the default branch before it can be triggered via API on another branch. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… existing CI ffmpeg's ./configure runs pkg-config with --static, which also resolves libxml2.pc's Requires.private (liblzma), unlike the Makefile's own plain --exists check used to decide whether to pass --enable-libxml2. Mirrors the package set already installed by on_pull_request_merge.yaml. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
reader imports metadata, which blank-imports pkg/raw to register RAW format support, which pulls in sys/libraw via cgo. writer and frame don't have this dependency, so this is the only extra target needed. Also adds zlib on Windows/MSYS2, since the Makefile's libraw target patches libraw.pc to require -lz -lm. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Each test-* target only listed go-dep/nothing, relying on the top-level test target to build every C library up front in the right order. But invoking test-ffmpeg (etc.) standalone - as the new Windows/macOS/Linux CI matrix does - failed because the Go packages under test transitively import more cgo sys packages than the target's name suggests: - test-ffmpeg needs libraw too (reader -> metadata -> pkg/raw blank import -> sys/libraw, for MIME type registration) - test-exif needs libheif too (pkg/exif imports sys/libheif directly) - test-heif needs libexif too (pkg/heif imports pkg/exif) - test-metadata needs ffmpeg+libexif+libheif+libraw (metadata/image imports pkg/exif, pkg/heif and pkg/raw; metadata/audio and metadata/video import sys/ffmpeg80) - test-gomedia needs all five (task blank-imports every metadata/* subpackage; gomedia imports pkg/chromaprint) - test-chromaprint needs the chromaprint target explicitly .PHONY targets are deduplicated within a single make invocation, so `make test` is unaffected - each library is still only built once. Simplifies the new test.yaml workflow back to just `make test-ffmpeg`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
go.mod already requires go 1.26.0. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Copied from go-server's config, plus a grpc group (google.golang.org/grpc* and grpc-ecosystem/*) alongside the existing opentelemetry one, since both dependency families repeatedly split across many modules per release and otherwise flood PRs one module at a time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Surfaced by running CI on macOS for the first time (previously ubuntu-only): avfoundation's output-sink enumeration on a headless runner reports NumDevices() > 0 but with a nil entry in the list, likely a placeholder for "no real audio output hardware". Calling .String() on it doesn't panic - json.Marshal special-cases nil pointers that implement MarshalJSON by emitting literal "null" without invoking the method - which is what the test's assert.Contains(str, "device") choked on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…peg80 These older per-version FFmpeg bindings were stale and unreferenced outside the dead _old/ tree; sys/ffmpeg80 (selected via SYS_VERSION) is the only one actually maintained and tested. Removing them also resolves github.com//issues/95 (lost out-parameter in AVFormat_alloc_output_context2, only present in the two removed versions). Also fixes the allocator mismatch reported in #92: AVFilterInOut_alloc filled .name with C.CString (libc malloc), but avfilter_inout_free releases it via av_freep, matching FFmpeg's own allocator. Harmless where av_free resolves to libc free, but a real heap error elsewhere (Windows' _aligned_free, or whenever FFmpeg and the cgo runtime link different C runtimes). Now uses av_strdup/C.free, mirroring the existing idiom in avcodec_subtitle.go. Bumps go.mod to go 1.26.0 and upgrades dependencies, including google.golang.org/grpc to 1.84.0 (fixes 2 high + 1 medium Dependabot alerts, all patched by 1.83.1/1.83.2) and the opentelemetry modules. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Adds cmake across platforms (needed by chromaprint and libheif's CMake-based builds; already preinstalled on ubuntu-latest, but not guaranteed on macos-latest or in the MSYS2 MINGW64 environment). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
libraw's RAW-format parsing uses htonl/ntohl for byte-swapping, which on Windows live in Winsock (ws2_32) rather than libc. The link command already included -lws2_32, but before -lraw on the line - GNU ld resolves static libs left-to-right and won't revisit an earlier library for a later one's undefined symbols, so it never got picked up. Appending it to libraw.pc's own Libs line (like the existing -lz -lm) guarantees it lands right after -lraw regardless of overall link order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
chromaprint-build ran a bare `make` after cmake configure, assuming the Unix Makefiles generator. On MSYS2 (Windows), cmake picks Ninja instead since ninja is on PATH alongside the mingw-w64 toolchain, so no Makefile is generated and the build fails with "No targets specified and no makefile found." Switched to `cmake --build`/`--install`, generator -agnostic, matching the existing libheif-build/install targets. Also add libde265/aom/dav1d (brew on macOS, mingw-w64 packages on Windows) so libheif actually has a decoder - surfaced on macOS as every heif test failing with "Decoder plugin generated an error: Unspecified" once the CI matrix expanded to the full make test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
profile/manager's tests spin up a real Postgres via go-pg's testcontainers-based test helper (go-pg/pkg/test.Main), which always needs a Docker daemon - no env-var fallback to an existing connection. ubuntu-latest has Docker built in; macos-latest and windows-latest don't reliably, hence "rootless Docker not found" on macOS once the matrix expanded to the full make test. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous sed matched the literal substring "-lraw -lstdc++" and inserted after it. That happened to work on Linux/macOS but silently matched nothing on Windows - libraw's own ./configure doesn't generate that exact adjacent substring there - so neither -lz/-lm nor the new -lws2_32 fix were ever actually applied, and the htonl link error persisted despite the previous fix. Appends to the end of the Libs: line instead, which doesn't depend on the line's existing content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
docker/setup-docker-action took 7+ minutes to provision a working Docker daemon on macOS/Windows runners, versus ubuntu-latest's built-in one. Since go-pg/pkg/test.Main has no escape hatch from its testcontainer, added one locally: profile/test/test.go's Main now checks GOMEDIA_TEST_DATABASE_URL and, if set, connects directly via pg.WithURL instead of starting a container - otherwise falls back to the existing container-based path unchanged (so Linux/local dev keep working exactly as before). test.yaml now provisions a native Postgres on macOS (Homebrew) and Windows (the runner's preinstalled service), setting that env var - same approach as go-omodie's test.yml. profile/manager doesn't depend on the custom Docker image's extra server settings (pg_stat_statements, wal_level=logical), so a vanilla instance is sufficient. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
extensionContentTypes entries (.m4a, .jpg, .jpeg) are deliberately curated because byte-sniffing gets them wrong or ambiguous (e.g. .m4a sniffs as video/mp4), so the extension is authoritative for them - no need to read the stream at all, which also avoids consuming the reader's first bytes for callers who need to read it again afterward. Extensions outside that curated map (including the broader stdlib mime.TypeByExtension table) still sniff first, same as before - e.g. .ts is genuinely ambiguous (TypeScript source vs. MPEG transport stream) and must not be fast-pathed. This also makes the old video/mp4 sniff-override branch dead code, since .m4a now returns before sniffing ever happens - removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Windows always pre-sets an OS=Windows_NT environment variable (a classic DOS/Windows convention). make's OS ?= ... treats any already-defined variable - including one inherited from the process environment - as "already set" and skips the right-hand side entirely, so the uname-based detection never ran on the actual Windows runner. $(OS) silently stayed "Windows_NT" for the whole build, meaning every ifeq/filter against $(OS) elsewhere in this Makefile - including yesterday's -lws2_32 fix for the htonl link error - never actually fired there, despite working in every local dry-run on this (non-Windows) machine. First attempt at this fix (OS := to clear it, relying on OS ?= afterwards) was itself wrong: a variable assigned to empty is still "already defined" as far as a later ?= is concerned, so OS stayed permanently empty. Directly computing the uname-based value inside the Windows_NT branch avoids that trap. Also adds a build/ directory cache (keyed on the Makefile's contents) to test.yaml, since Windows CI runs were taking 20+ minutes rebuilding all five C libraries from source every time. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Encode() returned immediately on any send_frame error, including EAGAIN - which per FFmpeg's documented send/receive API means "the encoder's internal queue is full, drain pending packets via receive_packet and retry," not a hard failure. The existing code only drained packets after a successful send, never as a retry path. This was presumably always wrong, but apparently never triggered in CI until now - Windows' encoder buffering (likely thread-count/queue -depth related) hits this path where Linux/macOS evidently don't, surfacing as "Encode: Resource temporarily unavailable" across nearly every reader/writer test that encodes a fixture file. Factored the drain loop into drainPackets so both the retry path and the existing post-send drain share it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
AVError.IsErrno(v syscall.Errno) computed AVERROR(v) using whatever
numeric value Go's syscall package assigned v, then compared that
against the real AVERROR code libavcodec/libavformat returned. That
works by accident on Linux and macOS, where Go's syscall.EAGAIN etc.
happen to be sourced from the same system <errno.h> the C library was
compiled against. It can never work on Windows: Go deliberately
defines its windows syscall.EAGAIN (and most other POSIX-named errno
constants) as APPLICATION_ERROR + small-offset - a synthetic value
chosen specifically so it can't collide with real Windows error
codes - which has no relation to the real EAGAIN=11 that mingw's own
<errno.h> defines and that mingw-compiled FFmpeg actually returns.
IsErrno(syscall.EAGAIN) therefore always silently returns false on
Windows, regardless of what FFmpeg actually returned.
This directly undermines the EAGAIN-retry fix just added to
writer/encoder.go, and affects every other IsErrno call site: decode's
send/receive loop, the demuxer's read loop, and the avdevice ENOSYS
("not implemented") checks - the same device-enumeration code whose
tests were patched earlier today to tolerate exactly that error text
on Windows, for what turns out to be this same underlying reason.
The existing IsErrno unit tests passed despite all this because they
constructed their test AVError from -int(syscall.EPERM) etc. - the
same Go-side value IsErrno then compared against, so they only proved
Go's own negation was self-consistent and never exercised the real
C-boundary mismatch.
Fixed by comparing against the compiling C toolchain's own <errno.h>
macros (C.EAGAIN/C.EINVAL/C.ENOSYS) directly, which is correct by
construction on every platform: libavcodec/libavformat and this cgo
file are necessarily compiled by the same toolchain against the same
headers. Replaced the single generic IsErrno(syscall.Errno) with
IsEAGAIN/IsEINVAL/IsENOSYS (the only three values actually used
anywhere in this codebase), and updated every call site, including
pkg/ffmpeg/filter.go.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
v4 is node20, which GitHub now force-runs on node24 anyway with a deprecation warning each run. v6's action.yml declares node24 itself, same path/key/restore-keys inputs we use. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Every caller - including the real production one, task/encoder/task.go
- constructs the output URL as &url.URL{Path: path} for a local file.
On Windows, a path's drive letter ("C:\...") is itself ambiguous with
a URI scheme, so net/url's String() both prepends "./" to disambiguate
it and percent-escapes every backslash, turning
"C:\Users\...\sample.mp4" into "./C:%5CUsers%5C...%5Csample.mp4".
FFmpeg's file-writing backend doesn't decode that back into the
intended path, so the write silently succeeds against the wrong
location - and reopening the real path afterward fails with "No such
file or directory" / ERROR_FILE_NOT_FOUND, which is what nearly every
reader test hit once get this far on Windows (the two fixes earlier
today for EAGAIN handling and the errno-matching bug).
Scoped to local files only (empty or "file" scheme) - a real network
URL (http://, rtmp://, srt://, ...) still needs the full String()
form, since the scheme/host carry meaning there. On Unix, url.Path and
url.String() already coincided for a plain path with no scheme, so
this is a no-op change in behavior there.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reader.Close() (and open()'s error path) called AVFormat_free_context on an input context opened via avformat_open_input (through AVFormat_open_url or AVFormat_open_reader) - but that only frees the AVFormatContext struct. It does not close the underlying file/protocol avformat_open_input opened internally, which is specifically what avformat_close_input exists to do (it's the documented, required counterpart). The leaked handle is invisible on Linux/macOS, where a still-open file can be freely deleted or renamed, but not on Windows: every reader test's t.TempDir() cleanup failed with "The process cannot access the file because it is being used by another process", once the EAGAIN, errno-matching, and writer.Create path fixes earlier today got far enough for these tests to actually open and read what they'd written. Fixing this safely also required AVFormat_open_reader (used by NewReader's custom io.Reader path): it set ctx.pb directly but never set AVFMT_FLAG_CUSTOM_IO, unlike AVFormat_open_writer's equivalent. Without that flag, avformat_close_input assumes it owns pb and avio_close()s it itself - double-freeing alongside Reader's own separate AVFormat_avio_context_free call. Mirrors AVFormat_open_writer, which already sets this flag correctly. Applied the same two fixes to pkg/ffmpeg/reader.go, an older/parallel reader implementation with the identical pattern - and, while there, fixed open()'s error path there to also free r.avio, which it was leaking unconditionally (not a Windows-only issue - NewReader's avio was never freed on this path on any platform). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…dows
chromaprint.h unconditionally decorates every function with
__declspec(dllimport) on Windows unless this macro is defined, which
only makes sense when linking against a DLL's import library. The
Makefile's chromaprint target builds and installs a static archive
(-DBUILD_SHARED_LIBS=0), so the linker went looking for __imp_-
prefixed symbols that don't exist there - undefined reference at
link time for every chromaprint_* function, surfaced once Windows CI
got past test-ffmpeg to test-chromaprint.
Harmless to define on every platform: chromaprint.h only consults it
in its Windows branch (non-Windows already uses
__attribute__((visibility("default")))), confirmed by chromaprint
tests still passing locally on macOS.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Without --disable-nls, libexif's ./configure enables gettext-based translation support whenever it finds libintl available (as MSYS2 does), generating calls to libintl_bindtextdomain/dgettext/snprintf - requiring -lintl at link time, which nothing here provides. go-media reads EXIF tags as structured data, not translated human-readable strings, so there's no need for this. Doesn't affect Linux (glibc has gettext built in, so no separate libintl, and no libintl_-prefixed symbols either) or macOS (Homebrew's gettext is keg-only and off configure's default search path, so NLS was already silently disabled there) - only Windows, where MSYS2 actually has gettext/libintl on the default search path. Verified locally: nm on the rebuilt libexif.a now shows zero libintl_ references (previously inherited this problem silently without surfacing, since nothing here links libexif.a directly without also linking libintl otherwise). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fakeTask.Run does almost no work before blocking on done, and the
test closed it immediately after Start returned - so on a fast or
virtualized runner, Started and Finished could read the exact same
timestamp, failing the Duration() > 0 assertion ("0s is not greater
than 0s", observed on Windows CI). A 1ms sleep before closing done
guarantees real elapsed time regardless of platform, without weakening
the assertion to >= 0 (which would stop catching a genuine bug where
Duration() always returns 0).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same bug as reader.Reader and pkg/ffmpeg.Reader (fixed earlier today): AVFormat_free_context only frees the AVFormatContext struct, not the underlying file/protocol handle that AVFormat_open_url/ AVFormat_open_reader opened internally via avformat_open_input - invisible on POSIX, fatal on Windows once a test's t.TempDir() tries to remove a still-open file. Found by grepping every remaining AVFormat_free_context call site: profile/schema/stream_test.go's openStreams helper (opens a demuxer directly since reader.Reader doesn't expose raw *ff.AVStream), and two of sys/ffmpeg80's own demux/context tests. The other AVFormat_free_ context call sites checked out fine - either a context that was only ever alloc_context'd (nothing to close), or avformat_mux.go's own output-context error path where avio_open itself failed before any file was opened. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sys/chromaprint's own #cgo LDFLAGS: -lstdc++ was redundant - the Makefile already sets CGO_LDFLAGS="-lstdc++ ..." globally, and libchromaprint.pc's own Libs line already has -lstdc++ positioned correctly after -lchromaprint. Harmless on its own, but once many packages needing libstdc++ link into a single binary (first hit by gomedia/httpclient, which pulls in ffmpeg+chromaprint+libexif+libraw+ libheif together), cgo appears to deduplicate the identical -lstdc++ string across packages and keep this directive's position over pkg-config's - too early relative to -lchromaprint for Windows' strict single-pass linker to resolve chromaprint's C++ internals (std::string::resize, used by fingerprint_compressor.cpp/base64.cpp). Also fixes gomedia/task's TestManager_RunCompletes: identical Duration()-reads-0s flake as task/manager's TestManager_StartCompletes (fixed earlier today), in a separate/parallel Manager implementation that happens to have the same bug independently - same fix, guarantee 1ms of real elapsed time before the task completes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Previous attempt (removing the redundant per-file -lstdc++ directive, on the theory that cgo was deduplicating it over pkg-config's correctly-ordered one) didn't fix it: still undefined reference to std::string symbols in gomedia/httpclient, just from a different -lstdc++ occurrence now textually adjacent in the final link line (apparently libheif's own chain, via libde265.pc). Each package's cgo flags get concatenated in whatever order Go's tooling assembles them across a many-package binary, which isn't under this package's control and isn't reliably predictable from here - confirmed by watching the exact failure shift between attempts. -Wl,--start-group/--end-group sidesteps the ordering question entirely by making the linker re-scan the group until every symbol inside resolves. GNU ld only (Linux, and Windows via MSYS2/mingw) - Apple's ld64 doesn't support these flags, but doesn't need them either, since its symbol resolution isn't single-pass like GNU ld's. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
fmt.Sprintf("file://%s", abs) breaks on Windows: filepath.Abs returns
a backslash-separated path with a drive letter, e.g.
D:\a\go-media\...\sample.mp3. Pasted directly after "file://", the
drive letter's colon parses as a host:port separator - net/url
rejected it server-side with "invalid port ... after host" before
ProbeSource ever got to open the file. Same class of bug as
writer.Create's path-mangling fix earlier today, opposite direction:
there, a Windows path went into a URL struct's String() and came out
wrong; here, one went directly into a string with no URL encoding at
all.
fileURL builds a proper url.URL{Scheme: "file", Path: ...} instead,
verified by hand with manually slash-converted paths (filepath.ToSlash
is a no-op on this darwin machine - it only converts the running OS's
own separator, so it can't be used to simulate Windows' backslash
handling locally): the path also needs a leading "/" prepended (not
just slash conversion) for String() to produce the standard triple-
slash "file:///D:/..." form, since the earlier shape still parses the
drive letter as a host the exact same way.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
FFmpeg's file protocol only strips the literal "file:" prefix (libavformat/file.c: av_strstart(url, "file:", &url)) - no further URI decoding at all. RFC 8089's standard Windows form, file:///C:/path (an empty authority, then a path that still carries the leading "/" before the drive letter), parses in Go to Path="/C:/path" - passed straight through, that leading slash makes an invalid Windows path, so ProbeSource failed with "No such file or directory" for any real, standards-compliant file:// URL a caller might send. POSIX paths happened to work by accident: a POSIX path's extra leading slashes collapse to one on open() (implementation-defined per POSIX, but POSIX.1-2017 XBD 4.13 permits it and every mainstream POSIX kernel does it), which Windows has no equivalent leniency for. fileProtocolPath normalizes a parsed "file" URL into the exact string FFmpeg's naive stripping needs - stripping that leading slash only when what follows really does look like a drive letter, and passing Opaque through unchanged for the (already-correct) non-"//" form some callers may send directly. gomedia/httpclient's TestProbeSource_File already needed a real url.URL-based fix today for a different reason (string-pasting a Windows path broke url.Parse itself, before this even came into play) - now that production normalizes the standard form, switched the test back to constructing one properly rather than speaking FFmpeg's internal dialect directly, so it actually exercises this fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
test.yaml now triggers on push/pull_request to main (plus workflow_dispatch for manual runs), same pattern as go-omodie's test.yml, replacing the temporary push-to-this-branch trigger that only existed because workflow_dispatch can't target a non-default branch until a workflow file exists on main. Removes on_pull_request_merge.yaml: it only ran make test on ubuntu-latest across a couple of Go versions, which test.yaml now fully covers (and far more - all three OSes, the complete native build chain) in one place. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Replaces on_publish.yaml, which only built Linux amd64/arm64 via the Makefile's docker/docker-push targets. docker.yaml adopts go-omodie's structure instead (docker/build-push-action + docker/metadata-action for Linux, a separate native-build-then-package job for Windows, a manifest job combining all three into one multi-platform tag). Renamed Dockerfile -> Dockerfile.linux (matching go-omodie's naming) and added Dockerfile.windows, which only packages an already-built gomedia.exe - it's built natively on a windows-2022 runner via the same MSYS2 setup as test.yaml (the cmd target doesn't itself depend on the library-build targets, so ffmpeg/chromaprint/libexif/libraw/ libheif are built explicitly first), since replicating that entire native build chain inside a Windows container would be a much bigger, separate undertaking. Two judgment calls made without being able to test either on a real Windows container host: - servercore, not nanoserver, as the Windows base image: nanoserver's minimal API surface suits a trivial CGO_ENABLED=0 binary (go-omodie's case) but is a much less certain fit for one linking OpenSSL, zlib, and several C/C++ codec libraries via cgo. - gomedia.exe links libstdc++/libgcc/libwinpthread dynamically (this project's own libraries are static, but MinGW's own C++/threading runtime isn't), so those three DLLs are copied alongside the .exe for the image to package together, rather than changing CGO_LDFLAGS for static linking - which would touch the same CGO_ENV test.yaml already relies on for its newly-green Windows run, risking a regression there to fix something untested here instead. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
File URL handling loses or malforms Windows UNC and drive paths, while MIME detection can misclassify arbitrary JPEG-named content.
3 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
docker.yaml now builds and publishes images directly via docker/build-push-action, not through make docker/docker-push/ docker-version - those targets, DOCKER_TAG/DOCKER_REPO/DOCKER_SOURCE/ DOCKER_REGISTRY, and the docker-dep check are all unused now. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Extracted FileProtocolPath to the root package (fileurl.go) so both
writer.Create and ProbeSourceTask's file-URL handling share one
implementation, instead of fixing the same bug in only one place
again - today's recurring pattern. Two real gaps the review caught in
the version that only lived in probe.go:
- A non-empty, non-"localhost" Host (a UNC path, file://server/share/
file) was silently dropped instead of being reassembled as
"//server/share/file", which FFmpeg's file protocol expects. Also
applies to writer.Create, which had the same drive-letter/UNC gap
writer_test.go's bare &url.URL{Path: ...} construction never
exercised (no scheme, so the explicit "file" scheme branch with
u.Path's RFC 8089 artifacts was never reached by any existing test).
- metadata/mime.go's extensionContentTypes entries conflated two
different things under one "Preferred" flag: .jpg/.jpeg are only
there to pick a preferred extension for ExtensionByType's reverse
mapping, while .m4a is there because byte-sniffing is actually wrong
for it (sniffs as video/mp4). Today's earlier "skip the read for a
curated extension" optimization treated all three as equally
authoritative, so a mislabeled or corrupted *.jpg would report
image/jpeg without ever being sniffed. Added a separate
Authoritative field, true only for .m4a; .jpg/.jpeg still sniff
first, same as before that optimization existed.
Also added a concurrency group to test.yaml, cancelling a superseded
run rather than letting it race a newer one - found while
investigating why Windows CI "doesn't seem to cache": the build
-directory cache is keyed on the Makefile's contents (changed on
nearly every commit today), and when two runs did share a key, the
loser's save silently failed ("another job may be creating this
cache"). Not added to docker.yaml: cancelling a publish mid-way is a
different risk than cancelling a test run.
A fourth review comment (test.yaml needs a pull_request trigger) was
already resolved by an earlier commit before this review ran against
the PR's current state.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces several improvements and updates to the build system, documentation, and CI workflows, with a strong focus on better Windows support, dependency management, and test reliability. The most significant changes include adding a cross-platform test workflow, refining Makefile logic for Windows compatibility, and improving dependency tracking via Dependabot.
CI/CD and Testing Enhancements:
test.yaml) to run tests on Linux, macOS, and Windows, with appropriate environment setup and package installation for each platform. This ensures better reliability and coverage across all supported OSes.on_pull_request_merge.yamlworkflow to test only on Go 1.26, removing Go 1.25 from the test matrix.Dependency and Build System Improvements:
dependabot.ymlconfiguration to automate monthly updates for Go modules and GitHub Actions, with grouping for OpenTelemetry and gRPC dependencies.OSvariable towindowsfor MSYS2, MinGW, and Cygwin environments, improving cross-platform compatibility.librawbuild to append-lws2_32to linker flags on Windows, ensuring proper linking for byte-swapping functions.Documentation Updates:
.github/copilot-instructions.mdandREADME.mdto reflect that onlysys/ffmpeg80/is supported by default, removing references to older FFmpeg versions. [1] [2] [3]