To fix Issue#2164, create infrastructure to compile Maiko for Windows System for Linux v1 as a separate OS from Linux #536
Conversation
…e buildRelease workflow and assicated Dockerfile to build the WSL1 variants along wih standard linux variants.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @fghalasz, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini or gemini-code-assist here, providing a summary of this pull request to help everyone get up to speed quickly.
This pull request aims to address Issue #2164, which describes problems running Maiko on Windows Subsystem for Linux v1 (WSL1) due to its lack of a virtual interval timer. The core idea is to create a separate build target specifically for WSL1, similar to how Cygwin is handled, which uses a simulated timer.
The changes involve updating the build system to detect WSL1, adding new platform definitions, and introducing new makefiles for compiling Maiko on this specific environment. This infrastructure is intended to allow for separate WSL1 builds in future releases.
Highlights
- WSL1 Detection: The
bin/osversionscript is updated to specifically detect if it's running within a WSL1 environment by checking for the presence and absence of certain environment variables (WSL_DISTRO_NAMEandWSL_INTEROP). It will now return "wsl1" instead of "linux" in this case. - Platform Definitions: The
inc/maiko/platform.hheader file is modified to introduce a new platform definition,MAIKO_OS_WSL1, which is defined when both__linux__and__wsl1__are set. This new definition also enablesMAIKO_EMULATE_TIMER_INTERRUPTSandMAIKO_EMULATE_ASYNC_INTERRUPTS, indicating the need for simulated timers on this platform. The existing__linux__definition is updated to exclude the__wsl1__case. - New Makefile Configurations: Several new makefiles (
makefile-init-wsl1.*andmakefile-wsl1.*) are added to support building Maiko for WSL1 on both aarch64 and x86_64 architectures using themakerightbuild system. These makefiles include the-D__wsl1__compiler flag.
Changelog
Click here to see the changelog
- bin/makefile-cygwin.x86_64-sdl
- Added the
-D__wsl1__flag to theDFLAGSvariable. (Note: This change appears to be in a Cygwin makefile, which might be unintended.)
- Added the
- bin/makefile-init-wsl1.aarch64
- New file added.
- Provides makefile options for building the
ldeinitexecutable for WSL1 on aarch64. - Sets
DFLAGSto include-D__wsl1__,-DNOVERSION, and-DINIT.
- bin/makefile-init-wsl1.x86_64
- New file added.
- Provides makefile options for building the
ldeinitexecutable for WSL1 on x86_64. - Sets
DFLAGSto include-D__wsl1__,-DNOVERSION, and-DINIT.
- bin/makefile-wsl1.aarch64-sdl
- New file added.
- Provides makefile options for building the
ldeandldesdlexecutables for WSL1 on aarch64 with SDL. - Note: The comments in this file currently describe options for 'Linux, ARM64 and SDL' and the
-D__wsl1__flag is not present inDFLAGSin the provided patch content, suggesting it might be a copy of a standard Linux makefile.
- bin/makefile-wsl1.aarch64-x
- New file added.
- Provides makefile options for building the
ldeandldexexecutables for WSL1 on aarch64 with X-Window. - Sets
DFLAGSto include-D__wsl1__.
- bin/makefile-wsl1.x86_64-sdl
- New file added.
- This file appears to contain two concatenated makefile definitions.
- The first part seems intended for WSL1 x86_64 X-Window (includes
-D__wsl1__). - The second part seems intended for Linux x86_64 SDL (includes
-D__wsl1__in its DFLAGS, which is unusual for a Linux makefile).
- bin/makefile-wsl1.x86_64-x
- New file added.
- Provides makefile options for building the
ldeandldexexecutables for WSL1 on x86_64 with X-Window. - Sets
DFLAGSto include-D__wsl1__.
- bin/osversion
- Modified the case statement for
*-*-linux*to include a check for WSL1 environment variables (WSL_DISTRO_NAMEandWSL_INTEROP). - If detected as WSL1, outputs "wsl1"; otherwise, outputs "linux".
- Modified the case statement for
- inc/maiko/platform.h
- Modified the
#ifdef __linux__block todefined(__linux__) && !defined(__wsl1__). - Added a new
#if defined(__linux__) && defined(__wsl1__)block. - The new block defines
MAIKO_OS_LINUX,MAIKO_OS_WSL1,MAIKO_OS_NAMEas "Windows System for Linux v1",MAIKO_OS_UNIX_LIKE,MAIKO_EMULATE_TIMER_INTERRUPTS, andMAIKO_EMULATE_ASYNC_INTERRUPTS.
- Modified the
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
WSL, a Linux shell,
On Windows it does dwell.
Timers misbehave,
New builds we must save,
To make Maiko work well.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces the necessary infrastructure to compile Maiko for Windows Subsystem for Linux v1 (WSL1) as a distinct platform. This is a valuable addition to support users on WSL1, especially given its limitations with virtual interval timers.
The changes involve modifications to osversion for detecting WSL1, additions to platform.h for WSL1-specific C preprocessor flags, and a new set of makefiles for WSL1 builds.
Overall, the approach is sound. However, there are a few critical and high-severity issues in the new makefiles that need to be addressed to ensure correct and consistent builds for WSL1 and to avoid impacting existing Cygwin builds. Please see the detailed comments below.
Summary of Findings
- Incorrect flag in Cygwin Makefile: The
bin/makefile-cygwin.x86_64-sdlfile has-D__wsl1__added toDFLAGS, which is likely incorrect for a Cygwin build and could lead to unexpected behavior. - Missing WSL1 flag in aarch64 SDL Makefile: The new
bin/makefile-wsl1.aarch64-sdlis missing the crucial-D__wsl1__flag inDFLAGS, and its header comment refers to 'Linux' instead of 'WSL1'. - Corrupted x86_64 SDL Makefile for WSL1: The
bin/makefile-wsl1.x86_64-sdlfile contains a mix of an X11 configuration and an SDL configuration, making it unusable as is. It needs to be a single, correct makefile for WSL1 x86_64 SDL. - WSL1 Detection and Platform Definition: The changes to
bin/osversionfor detecting WSL1 and toinc/maiko/platform.hfor defining WSL1-specific preprocessor macros appear correct and well-implemented.
Merge Readiness
The pull request makes good progress towards supporting WSL1. However, there are critical and high-severity issues in the makefiles that need to be addressed before this PR can be considered ready for merging. Specifically, bin/makefile-wsl1.x86_64-sdl needs a complete correction, bin/makefile-wsl1.aarch64-sdl needs the -D__wsl1__ flag added, and the extraneous -D__wsl1__ flag should be removed from bin/makefile-cygwin.x86_64-sdl.
I am unable to approve pull requests directly. Please ensure these issues are resolved and consider further review before merging.
… lde executables.
…ilding wsl1 versions of maiko
WSL1 does not support a virtual interval timer (similar to Cygwin). As a result, the standard linux build for Maiko results is a non-functional Maiko on WSL1 - as documented in Issue #2164.
This PR creates separate builds for WSL1, which are identical to the Linux builds except for the use of the simulated interval timer currently used in Cygwin builds to replace the missing virtual interval timer.
Changes are as follows:
Changes to support the new WSL1 platform in Medley releases will be included in an upcoming PR is the Medley repo.
Also PR#2180 in Medley repo, removes the forced use of vnc on WSL1 - handy for testing this PR.