Conversation
mikeller
commented
Sep 16, 2026
mikeller
commented
Member
- Add libmtp support to the MSVC build.
- Add support for building with WPD.
6 tasks
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The MSVC project may fail to compile or link when VCPKG_ROOT is set without libmtp installed.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds libmtp and WPD support to the Windows MSVC build and ignores VS Code metadata.
Changes:
- Adds vcpkg-based libmtp build settings across configurations.
- Adds WPD-related libraries.
- Ignores
.vscode/.
File summaries
| File | Summary |
|---|---|
contrib/msvc/libdivecomputer.vcxproj |
Adds libmtp/WPD configuration; the VCPKG_ROOT gate may enable libmtp when the port is unavailable. |
.gitignore |
Adds .vscode/ to ignored files. |
Review details
Suppressed comments (1)
contrib/msvc/libdivecomputer.vcxproj:76
- The project still exposes Win32/x86 configurations (for example,
contrib/README:34-37and the x86 CI job), but these unconditional paths always select the x64-windows triplet. Enabling libmtp for a Win32 build therefore links x64libmtp.libinto a 32-bit target and fails with an architecture mismatch. Selectx86-windowsfor Win32/x86 andx64-windowsfor x64, or disable libmtp for the 32-bit configurations.
<LibmtpIncludes Condition="'$(EnableLibmtp)'=='true'">$(VCPKG_ROOT)\installed\x64-windows\include;</LibmtpIncludes>
<LibmtpLibs Condition="'$(EnableLibmtp)'=='true'">libmtp.lib;ole32.lib;portabledeviceguids.lib;propsys.lib;</LibmtpLibs>
<LibmtpLibDirs Condition="'$(EnableLibmtp)'=='true'">$(VCPKG_ROOT)\installed\x64-windows\lib;</LibmtpLibDirs>
- Files reviewed: 1/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| <PropertyGroup Label="UserMacros" /> | ||
| <!-- Auto-enable libmtp support when VCPKG_ROOT is set, unless explicitly overridden. --> | ||
| <PropertyGroup> | ||
| <EnableLibmtp Condition="'$(EnableLibmtp)'=='' and '$(VCPKG_ROOT)'!=''">true</EnableLibmtp> |
Member
Author
There was a problem hiding this comment.
Both issues have been addressed in the rebase:
-
The auto-enable condition now uses
Exists('$(VCPKG_ROOT)\installed\x64-windows\include\libmtp.h')instead of just checkingVCPKG_ROOTis non-empty, so the port must actually be present for libmtp to be enabled. -
An explicit override sets
EnableLibmtptofalsefor Win32 configurations, so the x64-windows triplet paths are never referenced in 32-bit builds.
mikeller
force-pushed
the
add_windows_libmtp
branch
from
September 16, 2026 05:03
2ee6aec to
8719ef6
Compare
Member
Author
|
The DCO failure has been resolved: the "Add support for building with WPD." commit was amended to add the missing |
Copilot stopped reviewing on behalf of
mikeller due to an error
September 16, 2026 05:24
| <LibmtpPreprocessor Condition="'$(EnableLibmtp)'=='true'">HAVE_LIBMTP;</LibmtpPreprocessor> | ||
| <LibmtpIncludes Condition="'$(EnableLibmtp)'=='true'">$(VCPKG_ROOT)\installed\x64-windows\include;</LibmtpIncludes> | ||
| <LibmtpLibs Condition="'$(EnableLibmtp)'=='true'">libmtp.lib;ole32.lib;portabledeviceguids.lib;propsys.lib;</LibmtpLibs> | ||
| <LibmtpLibDirs Condition="'$(EnableLibmtp)'=='true'">$(VCPKG_ROOT)\installed\x64-windows\lib;</LibmtpLibDirs> |
| <PropertyGroup Label="UserMacros" /> | ||
| <!-- Enable libmtp support when the vcpkg port is present, unless explicitly overridden. --> | ||
| <PropertyGroup> | ||
| <EnableLibmtp Condition="'$(EnableLibmtp)'=='' and Exists('$(VCPKG_ROOT)\installed\x64-windows\include\libmtp.h')">true</EnableLibmtp> |
Comment on lines
+71
to
+74
| <EnableLibmtp Condition="'$(EnableLibmtp)'=='' and Exists('$(VCPKG_ROOT)\installed\x64-windows\include\libmtp.h')">true</EnableLibmtp> | ||
| <EnableLibmtp Condition="'$(EnableLibmtp)'==''">false</EnableLibmtp> | ||
| <!-- libmtp is x64-only until 32-bit vcpkg triplet support is confirmed. --> | ||
| <EnableLibmtp Condition="'$(EnableLibmtp)'=='true' and '$(Platform)'=='Win32'">false</EnableLibmtp> |
| <!-- libmtp is x64-only until 32-bit vcpkg triplet support is confirmed. --> | ||
| <EnableLibmtp Condition="'$(EnableLibmtp)'=='true' and '$(Platform)'=='Win32'">false</EnableLibmtp> | ||
| <LibmtpPreprocessor Condition="'$(EnableLibmtp)'=='true'">HAVE_LIBMTP;</LibmtpPreprocessor> | ||
| <LibmtpIncludes Condition="'$(EnableLibmtp)'=='true'">$(VCPKG_ROOT)\installed\x64-windows\include;</LibmtpIncludes> |
Comment on lines
+75
to
+78
| <LibmtpPreprocessor Condition="'$(EnableLibmtp)'=='true'">HAVE_LIBMTP;</LibmtpPreprocessor> | ||
| <LibmtpIncludes Condition="'$(EnableLibmtp)'=='true'">$(VCPKG_ROOT)\installed\x64-windows\include;</LibmtpIncludes> | ||
| <LibmtpLibs Condition="'$(EnableLibmtp)'=='true'">libmtp.lib;ole32.lib;portabledeviceguids.lib;propsys.lib;</LibmtpLibs> | ||
| <LibmtpLibDirs Condition="'$(EnableLibmtp)'=='true'">$(VCPKG_ROOT)\installed\x64-windows\lib;</LibmtpLibDirs> |
Comment on lines
+103
to
+104
| <AdditionalIncludeDirectories>..\..\include;$(LibmtpIncludes)%(AdditionalIncludeDirectories)</AdditionalIncludeDirectories> | ||
| <PreprocessorDefinitions>_CRT_SECURE_NO_WARNINGS;WIN32;_DEBUG;_WINDOWS;_USRDLL;LIBDIVECOMPUTER_EXPORTS;ENABLE_LOGGING;HAVE_VERSION_SUFFIX;HAVE_AF_IRDA_H;HAVE_WS2BTH_H;$(LibmtpPreprocessor)%(PreprocessorDefinitions)</PreprocessorDefinitions> |
mikeller
marked this pull request as draft
September 16, 2026 07:31
Add support for building with WPD.
Three improvements to the libmtp property-group block:
1. Guard the Exists() check with '$(VCPKG_ROOT)'!='' so that an
unset VCPKG_ROOT does not cause MSBuild to evaluate
Exists('\installed\x64-windows\include\libmtp.h') against the
drive root.
2. Fold the Win32 platform restriction directly into the enable
condition instead of overwriting EnableLibmtp in a separate
element, which was confusing due to MSBuild property-evaluation
order.
3. Make LibmtpLibDirs configuration-aware: Debug builds use the
vcpkg debug lib directory (installed\x64-windows\debug\lib),
Release builds use the release lib directory
(installed\x64-windows\lib). Previously Debug|x64 always
linked against the release import library.
Also add a short comment explaining why the Windows SDK system
libraries (ole32, portabledeviceguids, propsys) are listed
explicitly.
Signed-off-by: Michael Keller <github@ike.ch>
mikeller
force-pushed
the
add_windows_libmtp
branch
from
September 16, 2026 07:42
a954191 to
cf49027
Compare
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.