Skip to content

Conversation

@mekwall
Copy link
Contributor

@mekwall mekwall commented Jul 6, 2025

This pull request introduces support for native Windows by adding Windows-specific implementations and conditional compilation to various parts of the codebase. The changes ensure compatibility with Windows systems while maintaining functionality on other platforms.

Build System Updates:

  • Added a Windows runner (windows-latest) to the GitHub Actions workflow in .github/workflows/cmake.yml. Updated CMake configuration, build, and test steps to handle Windows-specific paths and commands.

Codebase Updates for Windows Compatibility:

  • CMake Adjustments: Introduced conditional flags for MSVC in cmake/CXXSniffer.cmake to set appropriate compiler flags for Windows (/W4) versus other platforms (-Wall -Wextra).
  • Datetime Handling: Updated Datetime::timegm in src/Datetime.cpp to use _putenv_s and _tzset for setting the time zone on Windows. [1] [2]

Filesystem (FS.cpp) Enhancements:

  • Platform-Specific Includes and Macros: Added Windows-specific headers and macros for filesystem operations, such as _mkdir, _unlink, and FlushFileBuffers. [1] [2]
  • Path Operations: Implemented Windows-specific logic for Path::realpath, Path::expand, and Path::glob to handle Windows paths and environment variables like USERPROFILE. [1] [2] [3] [4]
  • File and Directory Operations: Added Windows-compatible implementations for file creation, removal, locking, unlocking, truncating, and directory operations like create and remove_directory. [1] [2] [3] [4] [5] [6] [7]

Header File Updates:

  • Updated src/FS.h to include Windows-specific headers and define mode_t for compatibility.

These changes collectively enable the codebase to function seamlessly on Windows platforms while preserving its behavior on Unix-like systems.

Copilot AI review requested due to automatic review settings July 6, 2025 09:28
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds native Windows support throughout the codebase by introducing conditional compilation and Windows API calls, plus build workflow updates.

  • Introduces Windows-specific process execution, file system, and path handling in shared.cpp, FS.cpp, Table.cpp, and Datetime.cpp
  • Adjusts CMake flags for MSVC and updates GitHub Actions to include a Windows runner
  • Adds Windows headers, macros, and typedefs in FS.h and CXXSniffer.cmake

Reviewed Changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/shared.cpp Added Windows implementation of execute() and compare()
src/format.cpp Conditional include of <strings.h> on non-Windows
src/Table.cpp Added Windows-specific isatty check and <io.h>
src/PEG.cpp Switched from and to && for consistency
src/FS.h Included <windows.h> and defined mode_t on Windows
src/FS.cpp Implemented Windows file/dir operations and path logic
src/Datetime.cpp Windows variant of timegm using _putenv_s
cmake/CXXSniffer.cmake Set /W4 flags when using MSVC
.github/workflows/cmake.yml Added windows-latest runner and platform checks
Comments suppressed due to low confidence (2)

src/FS.cpp:191

  • The new Windows branch in Path::realpath isn't currently covered by tests. Adding unit tests for this code path will ensure it behaves correctly on Windows and prevent regressions.
  DWORD required_size = GetFullPathNameA(_data.c_str(), 0, nullptr, nullptr);

.github/workflows/cmake.yml:34

  • [nitpick] The workflow steps duplicate similar shell logic for different platforms. Consider extracting the platform check into a reusable script or consolidating the commands to reduce duplication and improve clarity.
        shell: bash

@djmitche djmitche requested a review from lauft July 6, 2025 12:48
@djmitche
Copy link
Collaborator

djmitche commented Jul 6, 2025

@lauft is this something you could take a look at?

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@lauft lauft self-assigned this Jul 9, 2025
Copy link
Member

@lauft lauft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's give it a try.
Although, making Taskwarrior/Timewarrior support Windows will be an extra effort...

@lauft lauft merged commit 327e7a3 into GothenburgBitFactory:master Jul 13, 2025
10 checks passed
@djmitche
Copy link
Collaborator

That's in progress in GothenburgBitFactory/taskwarrior#3824!

@mekwall
Copy link
Contributor Author

mekwall commented Jul 14, 2025

@lauft @djmitche I've done it here: mekwall/taskwarrior#1

I'll see if I get some time over this week to get a PR in to the taskwarrior repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants