Windows: don't build UNC paths from http/https URIs; enable GTest on Windows - #321
Open
bdeluca wants to merge 4 commits into
Open
Windows: don't build UNC paths from http/https URIs; enable GTest on Windows#321bdeluca wants to merge 4 commits into
bdeluca wants to merge 4 commits into
Conversation
BUILD_TESTING=ON could not configure under MSVC: macros.cmake did gtest
discovery through pkg-config, and gtest was not in vcpkg.json.
- Windows takes gtest from the vcpkg CONFIG package and publishes it
through ${GTEST_LDFLAGS}, the variable the test targets already link
against. Other platforms keep pkg_search_module untouched, so no
test CMakeLists needs editing.
- guard the GCC-only flags behind NOT MSVC, and add /bigobj for MSVC
test targets - CAF and GTest headers together exceed the default
object section limit
- add gtest to vcpkg.json
- put the DLL directories on PATH per test. Test executables are built
beside their sources while the DLLs land in bin/, and Windows has no
rpath, so without this every test exits 0xc0000135.
- cap the ctest timeout at 5s. Tests that work finish in well under a
second, and several hang rather than fail.
Four test fixes are needed before the suite will build and run on
Windows:
- lock_file_test: LockFile is ambiguous against the Win32 API of the
same name
- remote_session_file_test: RemoteSessionFile takes a std::string, and
fs::path only converts to one implicitly on POSIX, where
fs::path::string_type is std::string
- sequence_test: sequence.hpp aliases uid_t to DWORD so it needs
windows.h first - done in the test, not the header
- helpers_test: getenv("HOME") is null on Windows, so the old line was
undefined behaviour rather than a failure. Its absolute-path
assertions also used '/file.mov', which is drive-less and therefore
not absolute on Windows.
Signed-off-by: Ben de Luca <bdeluca@gmail.com>
uri_to_posix_path()'s comment said file://, but the code branched on uri.authority() alone, so it fired for any scheme carrying an authority: https://host.example.com/a/b.mp4 -> \host.example.com\a\b.mp4 Windows reads that as an SMB server, so every fs:: call on it opens TCP 445 to the host. Hosts that actively refuse the connection fail in milliseconds and go unnoticed; hosts that silently drop it - anything behind an egress firewall that DROPs rather than REJECTs outbound SMB, which is a common configuration - cost the full TCP SYN retransmit sequence. Measured with Test-Path against a UNC path: 127.0.0.1 0.07s (refuses) example.com 22.23s (drops) an external media host 26.09s (drops) It is not https specific either: any host outside the firewall stalls, over plain http just the same. ScanHelperActor does this on every media source created, and there is only one of it, so every other status, checksum and relink request queues behind it. Fixing it here rather than at the call sites covers every unguarded caller at once. Also fixes the repeated separator collapse, which the new tests caught: it looked for '//' but ran after the UNC branch had already rewritten the separators to backslashes, so it could never match the very input its own comment described. Adds a path matrix over http and https - the only non-file schemes xstudio takes media from - drive letter forms, UNC hosts, and round trips including reserved characters. A web uri yields its path component, pinned in the test: it cannot simply be empty, because is_file_supported() and media_actor both read the extension off it to choose a reader. Signed-off-by: Ben de Luca <bdeluca@gmail.com>
CAF's uri.path() drops the leading / whenever the URI carries an authority, so https://host/a/b.mp4 decodes to a/b.mp4; the fixup that puts it back has always been there but was wrapped in #ifdef __linux__, so Windows and macOS were left returning a working-directory-relative path where Linux returned a rooted one. This just drops the guard and lets the other platforms have the same hack - it is safe for every form the path matrix pins, since the UNC branch strips leading slashes before joining host and share and the drive-letter regex strips them before the C:, leaving file://server/... and file:///C:/... unchanged. Signed-off-by: Ben de Luca <bdeluca@gmail.com>
The Windows build guide had nothing on the test suite, which was fair enough while BUILD_TESTING=ON could not configure under MSVC. Covers configuring with it on, building a single test target, running through ctest and filtering by name, and notes that the DLL search path is already set up per test so nothing needs sourcing first. Signed-off-by: Ben de Luca <bdeluca@gmail.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
1 participant
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.
Enables GTest on Windows: gtest comes from the vcpkg CONFIG package under MSVC (pkg-config untouched elsewhere), plus per-test DLL search paths,
/bigobj, a short per-test timeout so hanging tests fail fast, and a few MSVC portability fixes in the tests themselves. The Windows build guide gets a section on running the tests.Then the bug the new tests pin down — initially raised on the Slack forum by XB:
uri_to_posix_path()built a UNC path for any URI with an authority, not justfile://, sohttps://host/a/b.mp4became\\host\a\b.mp4— and everyfs::call on that opens SMB to the host, ~22 s per call when a firewall drops the packets, serialized behind the single scanner actor. Now onlyfileURIs get the UNC treatment. Also fixes the double-slash collapse (it ran after the separators were already backslashes, so it never matched) and makes the leading-slash fixup for CAF's authority handling unconditional instead of Linux-only.The test suite is not clean on any platform yet; those failures are pre-existing and untouched here — the Linux failure set is identical before and after this change.