Conversation
795664c to
128ed99
Compare
There was a problem hiding this comment.
Pull request overview
Refactors Meson build configuration to start enabling Windows build bring-up and adds a GitHub Actions Windows CI job.
Changes:
- Introduces
host_systemgating in Meson to disable Linux-only features on Windows and adjust link dependencies. - Adds a Windows-only dummy
nvmeexecutable source and disables plugin/util source inclusion on Windows. - Adds a Windows GitHub Actions job using MSYS2/Mingw to configure and build with Meson.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
meson.build |
Adds host_system and gates features/deps; switches nvme sources to a Windows dummy; refactors link deps/args. |
util/meson.build |
Skips util sources on Windows. |
plugins/meson.build |
Disables plugin selection/build on Windows. |
libnvme/src/meson.build |
Attempts to skip libnvme sources/deps on Windows. |
libnvme/meson.build |
Uses want_tests/want_examples gating (disabled on Windows). |
libnvme/examples/telemetry-listen.c |
Adds missing include for select()/fd_set. |
nvme-dummy.c |
Adds minimal Windows bring-up executable source. |
.gitignore |
Ignores .vscode/. |
.github/workflows/build.yml |
Adds a Windows build job using MSYS2. |
Comments suppressed due to low confidence (1)
meson.build:154
- The json-c dependency check is inverted: when want_json_c is true (option enabled and non-Windows), json_c_dep is set to an empty dependency, which disables json-c even though it was requested. This also causes the else-branch to try to resolve json-c when want_json_c is false (including on Windows). Flip the conditional so that json_c_dep is resolved only when want_json_c is true, and otherwise set to an empty optional dependency.
if want_json_c
json_c_dep = dependency('', required: false)
else
json_c_dep = dependency(
'json-c',
required: get_option('json-c'),
version: '>=0.13',
fallback : ['json-c', 'json_c_dep'],
)
if json_c_dep.version().version_compare('>=0.14')
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| want_nvme = get_option('nvme').disabled() == false | ||
| want_libnvme = get_option('libnvme').disabled() == false | ||
| want_fabrics = get_option('fabrics').disabled() == false | ||
| want_fabrics = get_option('fabrics').disabled() == false and host_system != 'windows' | ||
| want_json_c = get_option('json-c').disabled() == false and host_system != 'windows' | ||
| want_tests = get_option('tests') and host_system != 'windows' |
There was a problem hiding this comment.
Windows builds currently still enable want_libnvme by default, but the libnvme/src/meson.build change leaves the libnvme sources list empty on Windows. Unless a Windows implementation/stub is added, consider disabling want_libnvme on Windows (similar to want_fabrics/tests/examples) to avoid Meson failing when creating the libnvme library target.
| sources = [] | ||
| if host_system == 'windows' | ||
| sources += [] | ||
| else | ||
| sources += [ | ||
| 'nvme/accessors.c', | ||
| 'nvme/base64.c', | ||
| 'nvme/cmds.c', | ||
| 'nvme/crc32.c', | ||
| 'nvme/filters.c', | ||
| 'nvme/ioctl.c', | ||
| 'nvme/lib.c', | ||
| 'nvme/linux.c', | ||
| 'nvme/log.c', | ||
| 'nvme/mi-mctp.c', | ||
| 'nvme/mi.c', | ||
| 'nvme/sysfs.c', | ||
| 'nvme/tree.c', | ||
| 'nvme/util.c', | ||
| ] | ||
| endif |
There was a problem hiding this comment.
On Windows, this leaves sources empty (want_fabrics is disabled on Windows, liburing/json sources are also gated away), but the file later unconditionally creates library('nvme', sources, ...). Meson typically errors on a library target with no sources; add at least a stub source for Windows or skip building libnvme on Windows.
| sources = [] | |
| if host_system == 'windows' | |
| sources += [] | |
| else | |
| sources += [ | |
| 'nvme/accessors.c', | |
| 'nvme/base64.c', | |
| 'nvme/cmds.c', | |
| 'nvme/crc32.c', | |
| 'nvme/filters.c', | |
| 'nvme/ioctl.c', | |
| 'nvme/lib.c', | |
| 'nvme/linux.c', | |
| 'nvme/log.c', | |
| 'nvme/mi-mctp.c', | |
| 'nvme/mi.c', | |
| 'nvme/sysfs.c', | |
| 'nvme/tree.c', | |
| 'nvme/util.c', | |
| ] | |
| endif | |
| sources = [] | |
| if host_system == 'windows' | |
| # On Windows, ensure we have at least one source file to avoid | |
| # Meson errors when creating the nvme library with an empty source list. | |
| sources += [ | |
| 'nvme/windows-stub.c', | |
| ] | |
| else | |
| sources += [ | |
| 'nvme/accessors.c', | |
| 'nvme/base64.c', | |
| 'nvme/cmds.c', | |
| 'nvme/crc32.c', | |
| 'nvme/filters.c', | |
| 'nvme/ioctl.c', | |
| 'nvme/lib.c', | |
| 'nvme/linux.c', | |
| 'nvme/log.c', | |
| 'nvme/mi-mctp.c', | |
| 'nvme/mi.c', | |
| 'nvme/sysfs.c', | |
| 'nvme/tree.c', | |
| 'nvme/util.c', | |
| ] | |
| endif |
| #include <stdio.h> | ||
|
|
||
| int main(int argc, char **argv) | ||
| { |
There was a problem hiding this comment.
argc and argv are unused here; if the build enables unused-parameter warnings (or treats warnings as errors in some environments), this can fail. Consider explicitly marking them unused (e.g., casting to void) or omitting the parameters if your style allows.
| { | |
| { | |
| (void)argc; | |
| (void)argv; |
7b6fb37 to
947d5fb
Compare
Restructure meson.build files to support windows (msys2). This is the first step in the windows port and does not generate a useful executalbe. Signed-off-by: Brandon Capener <bcapener@micron.com> [wagi: moved host_system condition up] Signed-off-by: Daniel Wagner <wagi@kernel.org>
Add a workflow to test the windows build. Signed-off-by: Daniel Wagner <wagi@kernel.org>
The example build fails because fd_set etc are not defined. Signed-off-by: Daniel Wagner <wagi@kernel.org>
Cleanup/refactoring of #3129 and CI build job added.