Rebase to cygwin-3.6.6 #123
Merged
Conversation
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
Signed-off-by: Jon Turney <jon.turney@dronecode.org.uk>
For dlopen()'ed DLL, __cxa_finalize() should always be called at dll detach time. The reason is as follows. In the case that dlopen()'ed DLL A is dlclose()'ed in the destructor of DLL B, and the destructor of DLL B is called in exit_state, DLL A will be unloaded by dlclose(). If __cxa_finalize() for DLL A is not called here, the destructor of DLL A will be called in exit() even though DLL A is already unloaded. This causes crash at exit(). In this case, __cxa_finalize() should be called before unloading DLL A even in exit_state. Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258877.html Fixes: c019a66 ("* dll_init.cc (dll_list::detach) ... Don't call __cxa_finalize in exiting case.") Reported-by: Thomas Huth <th.huth@posteo.eu> Reviewed-by: Mark Geisert <mark@maxrnd.com>, Jon Turney <jon.turney@dronecode.org.uk>, Corinna Vinschen <corinna-cygwin@cygwin.com> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit 0d2f981940a496a41b0b650a4ba286d9137302f7)
If dlopen() for DLL A is called in constructor DLL B, the constructor for DLL A is called twice, once called via cygwin_attach_dll() called from LoadLibrary(), and again from dll_list::init(). That is, the DLL with DLL_LOAD does not need dll::init() in dll_list::init(). This issue was found when debugging the issue: https://cygwin.com/pipermail/cygwin/2025-October/258877.html This patch remove dll::init() call in dll_list::init() for DLL_LOAD. Fixes: 2eb392b ("dll_init.cc: Revamp. Use new classes.") Reviewed-by: Mark Geisert <mark@maxrnd.com>, Jon Turney <jon.turney@dronecode.org.uk>, Corinna Vinschen <corinna-cygwin@cygwin.com> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit d77cb8e350346a060bb6ce722b93851f5b5f8bd3)
The atexit()'ed function may deadlock if it waits for another thread which calls atexit() in the current __call_atexit.c code. This patch unlock __atexit_recursive_mutex while calling atexit()'ed functions to avoid the deadlock mentioned above. glibc and Darwin do the same, so it sounds reasonable. Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258930.html Reported-by: Tomohiro Kashiwada <tomohiro-kashiwada@ezweb.ne.jp> Reviewed-by: Corinna Vinschen <corinna@vinschen.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit a2a8bc771f2f5c1c17f05d55a3d04ac09b022127)
Previously, variable i_all_lf was allocated and released in several
functions: lf_setlock(), lf_clearlock(), and lf_getlock(), and was
used only temporarily as noted in flock.cc. This pattern easily leads
to bugs like those that occurred in flock.cc, such as:
lf_setlock() lf_clearlock()
| .
i_all_lf = tp.w_get() .
| .
+---------------------->+
|
i_all_lf = tp.wget()
|
do something
|
(release i_all_lf implicitly)
|
+<----------------------+
| .
accessing i_all_lf (may destroy tmp_pathbuf area)
| .
The similar problem also happens in multi-thread case.
Fix and prevent the bugs by converting i_all_lf to a pointer pointing
to a static array in the inode_t class. Given inodes are allocated on
the cygheap, reduce MAX_LOCKF_CNT by one so an inode_t fits into a 64K
chunk.
Avoid accessing the all locks list outside of locking and move
get_all_locks_list() call in lf_clearlock() to fhandler_base::lock()
to avoid calling the function twice.
Addresses: https://cygwin.com/pipermail/cygwin/2025-October/258914.html
Fixes: ae181b0 ("Cygwin: lockf: Make lockf() return ENOLCK when too many
locks")
Reported-by: Nahor <nahor.j+cygwin@gmail.com>
Co-authored-by: Corinna Vinschen <corinna@vinschen.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
Signed-off-by: Corinna Vinschen <corinna@vinschen.de>
(cherry picked from commit 8c5dafb99cf018adddf6291dcd7dccc303cfd415)
Accessing i_all should always be performed via the i_all_lf pointer, never directly. Add leading underscores to support this notion. Improve inode_t comments a bit so it's hopefully clearer what all the members are doing. Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit 008f8bfba19c65312237ba6493e34877a4b7a6bd)
The commit a2a8bc771f2f has a problem that __atexit which includes
args for atexit()'ed function may be touched by another thread
since the mutex is unlocked while calling the function.
With this path, the args, etc. are copyed to local variable to
prevent this problem.
Fixes: a2a8bc771f2f ("newlib: Unlock the mutex while calling atexit()'ed functions")
Suggested-by: Sebastian Huber <sebastian.huber@embedded-brains.de>
Reviewed-by: Corinna Vinschen <corinna@vinschen.de>, Sebastian Huber <sebastian.huber@embedded-brains.de>
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
(cherry picked from commit 7c7d9cf585d0d7c905f600eff14bf4f4e07fb178)
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
- allow calling without argument - allow not only - but also -l to regenerate stock environment - allow numerical group IDs - In usage output, advertise -l instead of - and do not advertise the ability to run an arbitrary command instead of just a new shell Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit 8e3e5a784f302d59f0edcb305a3650e3e404391a)
Especially document -l as primary option and - just for Linux compatibility. Note that a command on the commandline is a Cygwin extension and incompatible with POSIX and Linux. Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit a212fd40a8deb9c75d4c433aef4cb007341fe1f7)
Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit 43ddce194c496f81519b4c5cdba0dd39eea0edce)
This patch fixes the bug in ESC sequence parser for title set used when pseudo console is enabled in pty_master_fwd_thread(). Previously, if multiple ESC sequences exist in a fowarding chunk, the later one might not be processed appropriately. Fixes: 10d083c ("Cygwin: pty: Inherit typeahead data between two input pipes.") Reviewed-by: Corinna Vinschen <corinna@vinschen.de>, Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit 347884ef62b94295a7d7389716d2b1de115e7a28)
In the ESC sequence parser in pty_master_fwd_thread, the termination ST (ESC \) was not supported for title set ESC sequence, that is, only BEL was detected. This patch allows ST as a termination. Fixes: 10d083c ("Cygwin: pty: Inherit typeahead data between two input pipes.") Reviewed-by: Corinna Vinschen <corinna@vinschen.de>, Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit f840dd0a0b11498cdc206c8ad48df1975d511209)
If the ESC comes when the state is not zero due to a broken ESC sequence, the second ESC sequence cannot be handled properly. With this patch, ESC that come in the middle of another sequence make the state machine start parsing new ESC secuence. Fixes: 10d083c ("Cygwin: pty: Inherit typeahead data between two input pipes.") Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit a2e8a63c90e3427a4e6f4e5a52754626fdf87599)
If the ESC sequence ends at the middle of the sequence, the 'state' remains non-zero. This prevent the next state machine from working as expected. On contrary, 'start_at' is set in each parser, so is not necessary to be initialized. Fixes: 10d083c ("Cygwin: pty: Inherit typeahead data between two input pipes.") Suggested-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit 53bbf4756c7f7c4e300406cd7f33a876ca9b4731)
In Windows 11, the command "rlwrap cmd" outputs garbaged screen. This is because rlwrap treats text between NLs as a line, while pseudo console sometimes omits NL before "CSIm;nH". This issue does not happen in Windows 10. This patch fixes the issue by adding CR NL before "CSIm:nH" into the output from the pseudo console if the OS is Windows 11. Reviewed-by: Corinna Vinschen <corinna@vinschen.de>, Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit 835358af6465dd1be1263525d6b970621a28f676)
If is_console_app() returns false, it means the app is GUI. In this case, standard handles would not be setup for non-cygwin app. Therefore, it is safer to return true for unknown case. Setting-up standard handles for GUI apps is pointless indeed, but not unsafe. Fixes: bb42852 ("Cygwin: pty: Implement new pseudo console support.") Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit d9bbeb69ba8b5c0eeb294ff665e16cba7039a5df)
When that function was introduced in bb42852 (Cygwin: pty: Implement new pseudo console support., 2020-08-19) (back then, it was added to `spawn.cc`, later it was moved to `fhandler/termios.cc` in 32d6a6c (Cygwin: pty, console: Encapsulate spawn.cc code related to pty/console., 2022-11-19)), it was implemented with strong assumptions that neither creating the file handle nor reading 1024 bytes from said handle could fail. This assumption, however, is incorrect. Concretely, I encountered the case where `is_console_app()` needed to open an app execution alias, failed to do so, and still tried to read from the invalid handle. Let's add some error handling to that function. Fixes: bb42852 (Cygwin: pty: Implement new pseudo console support., 2020-08-19) Co-authored-by: Takashi Yano <takashi.yano@nifty.ne.jp> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> (cherry picked from commit c9b98d2e553b5d3c08b597ec1c84b7ccf0a249fe)
… first This function contains special handling of these file extensions, treating them as console applications always, even if the first 1024 bytes do not contain a PE header with the console bits set. However, Batch and Command files are never expected to have such a header, therefore opening them and reading their first bytes is a waste of time. Let's honor the best practice to deal with easy conditions that allow early returns first. Fixes: bb42852 (Cygwin: pty: Implement new pseudo console suppot., 2020-08-19) Reviewed-by: Takashi Yano <takashi.yano@nifty.ne.jp> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> (cherry picked from commit 18b3c0fd0a26b3fc1da720e17103512b90eb71fc)
An app execution alias cannot be opened for read (CreateFile() with GENERIC_READ fails with ERROR_CANT_ACCESS_FILE) because it does not resolve the reparse point for app execution alias. Therefore, we need to know if the path is an app execution alias when opening it. This patch adds new api path_conv::is_app_execution_alias() for that purpose. Reviewed-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit 0cfa0465d13e77221a0d8d72d2c0dd4588bb387f)
This patch changes the argument for passsing a path to an app to fhandler_termios::spawn_worker() from const WCHAR *runpath to path_conv &pc. The purpose of this patch is to prepare for a subsequent patch, that is intended to fix a bug in executing Microsoft Store apps. Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit c87b33fc5ee0e38860a03dc59dbd3c4e0b74e51b)
Microsoft Store apps are run via app execution aliases, i.e. special reparse points. Currently, spawn.cc does not resolve a reparse point when retrieving the path of app after the commit f74dc93, that disabled to follow windows reparse point by adding PC_SYM_NOFOLLOW_REP flag. However, unlike proper reparse point, app execution aliases are not resolved when trying to open the file via CreateFile(). As a result, if the path, that is_console_app() received, is the reparse point for an app execution alias, the func retuned false due to open-failure because CreateFile() cannot open an app execution alias, while it can open normal reparse point. If is_console_app() returns false, standard handles for console app (such as WSL) would not be setup. This causes that the console input cannot be transfered to the non-cygwin app. This patch fixes the issue by locally converting the path once again using option PC_SYM_FOLLOW (without PC_SYM_NOFOLLOW_REP), which is used inside is_console_app() to resolve the reparse point, if the path is an app execution alias. Fixes: f74dc93 ("fix native symlink spawn passing wrong arg0") Reviewed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit 28b82486d39f6594f6fd3f8ba2ffdbe30ea932b2)
Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp> (cherry picked from commit ede095ca4ff4c9d2640dd85187d51b86b8cb4c65)
The upstream master branch has been renamed to main. Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit 6515799b0c4d557f631a7633566914752dce62de)
Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit e0a7045d1f6df2f500ec3eb13e70ee1d25edbd5c)
needed to handle recent changes in Unicode.org data file layout (cherry picked from commit 973798bf0c97bbb6a100ac058971972287474634)
as done by either of the following script invocations: cd newlib/libc; make -f Makefile.unidata cd build/*/newlib; make unidata (cherry picked from commit 349435db1b3797a4d143d252433caba101f6996b)
Commit dc7b673 ("Cygwin: uinfo: prefer token primary group") broke the code overriding the primary group in two different ways: - It changed the way myself->gid was set before checking its value. Prior to dc7b673, myself->gid was always set to the primary group from the passwd entry (pw_gid). With the patch, it was set to the primary group from the Windows user token (token_gid) in the first place. The following condition checking if pw_gid is different from token_gid did so, by checking token_gid against myself->gid, rather than against pw_gid. After dc7b673 this was always false and the code block overriding the primary group in Cygwin and the Windows user token with pw_gid was never called anymore. The solution is obvious: Do not check token_gid against myself->gid, but against the desires primary GID value in pw_gid instead. - The code block overriding the primary group simply assumed that myself->gid was already set to pw_gid, but, as outlined above, this was not true anymore after dc7b673. This is a subtle error, because it leads to having the wrong primary GID in `id' output, while the primary group SID in the user token was correctly set. But as soon as you run this under strace or GDB, the problem disappears, because the second process tree under GDB or strace takes over from the already changed user token. The solution is to override myself->gid with pw_gid once more, after successfully changing the primary GID to pw_gid. Fixes: dc7b673 ("Cygwin: uinfo: prefer token primary group") Addresses: https://cygwin.com/pipermail/cygwin/2025-December/259068.html Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit 877dd793a94c54e651e9f755656add0e529749cf)
Do not only allow to override the (localized) group "None" as primary group, but also the user account. The user account is used as primary group in the user token, if the user account is a Microsoft Account or an AzureAD account. Fixes: dc7b673 ("Cygwin: uinfo: prefer token primary group") Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit 351adcd96f026a4ac4e771764d5dbc512190dbf2)
…hines When overriding the (localized) primary group "None" of a local SAM account via SAM comment entry (e.g. '<cygwin group="some_group"/>') on a Active Directory domain member machine, we have to take into account, that the local account domain (actually the machine name) is always prepended to local account names, i. e. MACHINE+account because the names without prepended domain are reserved for the primary AD domain accounts. Therefore commit cc332c9 added code to prepend the local account domain to the group name from the SAM comment, if the machine is a domain member. But here's the problem: If the group in the SAM comment entry is a real local group, prepending the local account domain is all nice and dandy. But if the account used in the SAM comment is a builtin like "Authenticated Users" (S-1-5-11) or an alias like "Users" (S-1-5-32-545), this falls flat. This patch keeps the check for "MACHINE+account" first. This avoids fetching the AD group rather than the local SAM group, if a local group has the same name as an AD group. But now, if the group prepended with the local account domain doesn't result in a valid group entry, try again with the naked group name, to allow aliases or builtin accounts to pass as primary group. Fixes: cc332c9 ("* uinfo.cc [...] (pwdgrp::fetch_account_from_windows): Drop outdated comment. Fix code fetching primary group gid of group setting in SAM description field.") Signed-off-by: Corinna Vinschen <corinna@vinschen.de> (cherry picked from commit 0870013a6487a25c1dc24f255daa0e6afd396bfb)
This seems to have been necessary in 2015 to address the expectations of some tests in Git's test suite, but seems no longer to be necessary?!? Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This topic branch fixes the problem where a UTF-16 command-line was converted to UTF-8 in an incorrect way (because Cygwin treated it as if it was a file name and applied some magic that is intended to allow for otherwise invalid file names on Windows). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
msys2-runtime: restore fast path for current user primary group
Workaround certain anti-malware programs Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
See https://docs.github.com/en/code-security/dependabot/working-with-dependabot/keeping-your-actions-up-to-date-with-dependabot#enabling-dependabot-version-updates-for-actions for details. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
AutoHotKey is not only a convenient way to add keyboard shortcuts for functionality (or applications) that does not come with shortcuts, but it is in general a powerful language to remote control GUI elements. We will use this language to implement a couple of automated tests that should hopefully prevent regressions as we have experienced in the past (for example, a regression that was fixed and immediately re-broken, which went unnoticed for months). So let's start by adding a library of useful functions, to be extended as needed. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
One particularly important part of Git for Windows' MSYS2 runtime is that it is used to run Git's tests, and regressions happened there: For example, the first iteration of MSYS2 runtime v3.5.5 caused plenty of hangs. This was realized unfortunately only after deploying the msys2-runtime Pacman package, and some painful vacation-time scrambling was required to revert to v3.5.4.This was realized unfortunately only after deploying the msys2-runtime Pacman package, and some painful vacation-time scrambling was required to revert to v3.5.4. To verify that this does not happen anymore, let's reuse what `setup-git-for-windows-sdk` uses in Git's very own CI: - determine the latest successful `ci-artifacts` workflow run in git-for-windows/git-sdk-64 - download its Git files and build artifacts - download its minimal-sdk - overwrite the MSYS2 runtime in the minimal-sdk - run the test suite and the assorted validations just like the `ci-artifacts` workflow (from which these jobs are copied) This obviously adds a hefty time penalty (around 7 minutes!) to every MSYS2 runtime PR in the git-for-windows org. Happily, these days we don't need many of those, and the balance between things like the v3.5.5 scramble and waiting a little longer for the CI to finish is clearly in favor of the latter. Co-authored-by: Jeremy Drake <github@jdrake.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This is a forked repository... Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The issue reported in microsoft/git#730 was fixed, but due to missing tests for the issue a regression slipped in within mere weeks. Let's add an integration test that will (hopefully) prevent this issue from regressing again. This integration test is implement as an AutoHotKey script. It might look unnatural to use a script language designed to implement global keyboard shortcuts, but it is a quite powerful approach. While there are miles between the ease of developing AutoHotKey scripts and developing, say, Playwright tests, there is a decent integration into VS Code (including single-step debugging), and AutoHotKey's own development and community are quite vibrant and friendly. I had looked at alternatives to AutoHotKey, such as WinAppDriver, SikuliX, nut.js and AutoIt, in particular searching for a solution that would have a powerful recording feature similar to Playwright, but did not find any that is 1) mature, 2) well-maintained, 3) open source and 4) would be easy to integrate into a GitHub workflow. In the end, AutoHotKey appeared my clearest preference. So how is the test implemented? It lives in `ui-test/` and requires AutoHotKey v2 as well as Windows Terminal (the Legacy Prompt would not reproduce the problem). It then follows the reproducer I gave to the Cygwin team: 1. initialize a Git repository 2. install a `pre-commit` hook 3. this hook shall spawn a non-Cygwin/MSYS2 process in the background 4. that background process shall print to the console after Git exits 5. open a Command Prompt in Windows Terminal 6. run `git commit` 7. wait until the background process is done printing 8. press the Cursor Up key 9. observe that the Command Prompt does not react (in the test, it _does_ expect a reaction: the previous command in the command history should be shown, i.e. `git commit`) In my reproducer, I then also suggested to press the Enter key and to observe that now the "More ?" prompt is shown, but no input is accepted, until Ctrl+Z is pressed. Naturally, the test should not expect _that_ ;-) There were a couple of complications I needed to face when developing this test: - I did not find any easy macro recorder for AutoHotKey that I liked. It would not have helped much, anyway, because intentions are hard to record. - Before I realized that there is excellent AutoHotKey support in VS Code via the AutoHotKey++ and AutoHotKey Debug extensions, I struggled quite a bit to get the syntax right. - Windows Terminal does not use classical Win32 controls that AutoHotKey knows well. In particular, there is no easy way to capture the text that is shown in the Terminal. I tried the (pretty excellent!) [OCR for AutoHotKey](https://github.com/Descolada/OCR), but it uses UWP OCR which does not recognize constructs like "C:\Users\runneradmin>" because it is not English (or any other human language). I ended up with a pretty inelegant method of selecting the text via mouse movements and then copying that into the clipboard. This stops scrolling and I worked around that by emulating the mouse wheel afterwards. - Since Windows Terminal does not use classical Win32 controls, it is relatively hard to get to the exact bounding box of the text, as there is no convenient way to determine the size of the title bar or the amount of padding around the text. I ended up hard-coding those values, I'm not proud of that, but at least it works. - Despite my expectations, `ExitApp` would not actually exit AutoHotKey before the spawned process exits and/or the associated window is closed. For good measure, run this test both on windows-2022 (corresponding to Windows 10) and on windows-2025 (corresponding to Windows 11). Co-authored-by: Eu-Pin Tien <eu-pin.tien@diamond.ac.uk> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Converted ui-tests workflow into a test matrix that uses both the windows-2022 and windows-2025 runners.
The test logs are quite interesting to have, and not only those: In case of a fatal failure, the test directory is valuable information, too. Let's always upload them as build artifacts. For convenience, let's just reuse the `ui-tests/` directory as the place to put all of those files; Technically, we do not need the files in there that are tracked by Git, but practically speaking, it is neat to have them packaged in the same `.zip` file as the test logs and stuff. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Sometimes the logs are empty and it is highly unclear what has happened.
In such a scenario, a picture is indeed worth more than a thousand
words.
Note that this commit is more complicated than anyone would like, for
two reasons:
- While PowerShell is the right tool for the job, a PowerShell step in
GitHub Actions will pop up a Terminal window, _hiding_ what we want to
screenshot. To work around that, I tried to run things in a Bash step.
_Also_ opens a Terminal window! Node.js to the rescue.
- _Of course_ it is complicated to take a screenshot. The challenge is
to figure out the dimensions of the screen, which should be as easy as
looking at `[System.Windows.Forms.Screen]::PrimaryScreen`'s `Bounds`
attribute.
Easy peasy, right? No, it's not. Most machines nowadays have a
_ridiculous_ resolution which is why most setups have a _zoom factor_.
Getting to that factor should be trivial, by calling
`GetDeviceCaps(hDC, LOGPIXELSX)`, but that's not working in modern
Windows! There is a per-monitor display scaling ("DPI"). But even
_that_ is hard to get at, calling `GetDpiForMonitor()` will still
return 96 DPI (i.e. 100% zoom) because PowerShell is not marked as
_Per-Monitor DPI Aware_. Since we do not want to write a manifest into
the same directory as `powershell.exe` resides, we have to jump
through yet another hoop to get that.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The Ctrl+C way to interrupt run-away processes is highly important. It was recently broken in multiple ways in the Cygwin runtime (and hence also in the MSYS2 runtime). Let's add some integration tests that will catch regressions. It is admittedly less than ideal to add _integration_ tests; While imitating exactly what the end user does looks appealing at first, excellent tests impress by how quickly they allow regressions not only to be identified but also to be fixed. Even worse: all integration tests, by virtue of working in a broader environment than, say, unit tests, incur the price of sometimes catching unactionable bugs, i.e. bugs in software that is both outside of our control as well as not the target of our testing at all. Nevertheless, seeing as Cygwin did not add any unit tests for those Ctrl+C fixes (which is understandable, given how complex testing for Ctrl+C without UI testing would be), it is better to have integration tests than no tests at all. So here goes: This commit introduces a test that verifies that the MSYS2 `sleep.exe` can be interrupted when run from PowerShell in a Windows Terminal. This was broken in v3.6.0 and fixed in 7674c51 (Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread, 2025-07-01). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
This was the actual use case that was broken and necessitated the fix in 7674c51 (Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread, 2025-07-01). It does require an SSH server, which Git for Windows no longer ships. Therefore, this test uses the `sshd.exe` of OpenSSH for Windows (https://github.com/powershell/Win32-OpenSSH) in conjunction with Git for Windows' `ssh.exe` (because using OpenSSH for Windows' variant of `ssh.exe` would not exercise the MSYS2 runtime and therefore not demonstrate a regression, should it surface in the future). To avoid failing the test because OpenSSH for Windows is not available, the test case is guarded by the environment variable `OPENSSH_FOR_WINDOWS_DIRECTORY` which needs to point to a directory that contains a working `sshd.exe`. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In the previous commit, I added a new UI test that generates a somewhat large repository for testing the clone via SSH. Since that repository is created in the test directory, that would inflate the `ui-tests` build artifact rather dramatically. So let's create the repository outside of that directory. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
The fixes of 7674c51 (Cygwin: console: Set ENABLE_PROCESSED_INPUT when disable_master_thread, 2025-07-01) were unfortunately not complete; There were still a couple of edge cases where Ctrl+C was unable to interrupt processes. Let's add a demonstration of that issue. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In 0ae6a6f (Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader, 2025-06-27), a quite problematic bug was fixed where somewhat large-ish repositories could not be cloned via SSH anymore. This fix was not accompanied by a corresponding test case in Cygwin's test suite, i.e. there is no automated way to ensure that there won't be any regressions on that bug (and therefore it would fall onto end users to deal with those). This constitutes what Michael C. Feathers famously characterized as "legacy code" in his book "Working Effectively with Legacy Code": To me, legacy code is simply code without tests. I've gotten some grief for this definition. What do tests have to do with whether code is bad? To me, the answer is straightforward, and it is a point that I elaborate throughout the book: Code without tests is bad code. It doesn’t matter how well written it is; it doesn’t matter how pretty or object-oriented or well-encapsulated it is. With tests, we can change the behavior of our code quickly and verifiably. Without them, we really don’t know if our code is getting better or worse. Just to drive this point home, let me pull out Exhibit A: The bug fix in question, which is the latest (and hopefully last) commit in a _long_ chain of bug fixes that fix bugs introduced by preceding bug fixes: - 9e4d308 (Cygwin: pipe: Adopt FILE_SYNCHRONOUS_IO_NONALERT flag for read pipe., 2021-11-10) fixed a bug where Cygwin hung by mistake while piping output from one .NET program as input to another .NET program (potentially introduced by 3651990 (Cygwin: pipe: Avoid false EOF while reading output of C# programs., 2021-11-07), which was itself a bug fix). It introduced a bug that was fixed by... - fc691d0 (Cygwin: pipe: Make sure to set read pipe non-blocking for cygwin apps., 2024-03-11). Which introduced a bug that was purportedly fixed by... - 7ed9adb (Cygwin: pipe: Switch pipe mode to blocking mode by default, 2024-09-05). Which introduced a bug that was fixed by... - cbfaeba (Cygwin: pipe: Fix incorrect write length in raw_write(), 2024-11-06). Which introduced a bug that was fixed by... the SSH hang fix in 0ae6a6f (Cygwin: pipe: Fix SSH hang with non-cygwin pipe reader, 2025-06-27). There is not only the common thread here that each of these bug fixes introduced a new bug, but also the common thread that none of the commits introduced new test cases into the test suite that could potentially have helped prevent future breakages in this code. So let's at least add an integration test here. Side note: I am quite unhappy with introducing integration tests. I know there are a lot of fans out there, but I cannot help wondering whether they favor the convenience of writing tests quickly over the vast cost of making debugging any regression a highly cumbersome and unenjoyable affair (try single-stepping through a test case that requires several processes to be orchestrated in unison). Also, integration tests have the large price of introducing moving parts outside the code base that is actually to be tested, opening the door for breakages caused by software (or infrastructure, think: network glitches!) that are completely outside the power or responsibility of the poor engineer tasked with fixing the breakages. Nevertheless, I have been unable despite days of trying to wrap my head around the issue to figure out a way to reproduce the `fhandler_pipe_fifo::raw_write()` hang without involving a MINGW `git.exe` and an MSYS2/Cygwin `ssh.exe`. So: It's the best I could do with any reasonable amount of effort. It's better to have integration tests that would demonstrate regressions than not having any tests for that at all. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
ci: run Git's entire test suite
There have been way too many regressions in Cygwin as of late, in particular in the console handling. The worst part? Many of those bugs were introduced _in bug fix patches_! Here are a bunch of tests that are designed to help Git for Windows increase confidence in upgrades to newer Cygwin versions. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
On hosted GitHub Actions runners, there is always this Log window visible on the Desktop, and due to some magic logic, this window is sometimes in the foreground on the `windows-2025` runners. Let's minimize it so that it is out of the way and does not interfere with the AutoHotKey-based UI tests. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 4 to 5. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v4...v5) --- updated-dependencies: - dependency-name: actions/upload-artifact dependency-version: '5' dependency-type: direct:production update-type: version-update:semver-major ... Originally-authored-by: dependabot[bot] <support@github.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…ows-2025-ui-tests-more-robust ui-tests: minimize Log window
Bumps [actions/checkout](https://github.com/actions/checkout) from 5 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - [Commits](actions/checkout@v5...v6) Bumps [actions/upload-artifact](https://github.com/actions/upload-artifact) from 5 to 6. - [Release notes](https://github.com/actions/upload-artifact/releases) - [Commits](actions/upload-artifact@v5...v6) Originally-authored-by: dependabot[bot] <support@github.com> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
…pload/download-artifact-actions Upgrade `upload-artifact`/`download-artifact` Actions
…t/github_actions/actions/checkout-6 build(deps): bump actions/checkout from 5 to 6
Member
Author
|
/open pr The workflow run was started |
Member
Author
Hrmpf. In my endeavor to fix the ugly timeout issues, I might have worsimproved the logic. Sure, it no longer times out, but it also seems to not find the correct workflow run, either: This workflow run ran last week and opened the cURL PR, not an |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
13 participants
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.
This PR rebases Git for Windows' MSYS2 runtime patches to cygwin-3.6.6.
Range-diff vs previous rebase
enable_pconvalue forMSYSMSYSenvironment variable@@ Metadata ## Commit message ## fixup! CI: add a GHA for doing a basic build test - Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 5. + Bumps [actions/checkout](https://github.com/actions/checkout) from 4 to 6. - [Release notes](https://github.com/actions/checkout/releases) - [Changelog](https://github.com/actions/checkout/blob/main/CHANGELOG.md) - - [Commits](https://github.com/actions/checkout/compare/v4...v5) + - [Commits](https://github.com/actions/checkout/compare/v4...v6) --- updated-dependencies: - dependency-name: actions/checkout - dependency-version: '5' + dependency-version: '6' dependency-type: direct:production update-type: version-update:semver-major ... @@ .github/workflows/build.yaml: jobs: steps: - name: Checkout code - uses: actions/checkout@v4 -+ uses: actions/checkout@v5 ++ uses: actions/checkout@v6 - name: setup-msys2 uses: msys2/setup-msys2@v2@@ winsup/cygwin/mount.cc: mount_info::create_root_entry (const PWCHAR root) + sys_wcstombs_path (native_root, PATH_MAX, root); assert (*native_root != '\0'); if (add_item (native_root, "/", - MOUNT_SYSTEM | MOUNT_IMMUTABLE | MOUNT_AUTOMATIC) + MOUNT_SYSTEM | MOUNT_IMMUTABLE | MOUNT_AUTOMATIC | MOUNT_NOACL) @@ winsup/cygwin/mount.cc: mount_info::conv_to_posix_path (PWCHAR src_path, char *posix_path, } tmp_pathbuf tp;@@ .github/workflows/ui-tests.yml (new) + + runs-on: ${{ matrix.os }} + steps: -+ - uses: actions/download-artifact@v5 ++ - uses: actions/download-artifact@v7 + with: + name: ${{ inputs.msys2-runtime-artifact-name }} + path: ${{ runner.temp }}/artifacts @@ .github/workflows/ui-tests.yml (new) + $p = Get-ChildItem -Recurse "${env:RUNNER_TEMP}\artifacts" | where {$_.Name -eq "msys-2.0.dll"} | Select -ExpandProperty VersionInfo | Select -First 1 -ExpandProperty FileName + cp $p "c:/Program Files/Git/usr/bin/msys-2.0.dll" + -+ - uses: actions/cache/restore@v4 ++ - uses: actions/cache/restore@v5 + id: restore-wt + with: + key: wt-${{ env.WT_VERSION }} @@ .github/workflows/ui-tests.yml (new) + run: | + curl -fLo "$RUNNER_TEMP/wt.zip" \ + https://github.com/microsoft/terminal/releases/download/v$WT_VERSION/Microsoft.WindowsTerminal_${WT_VERSION}_x64.zip -+ - uses: actions/cache/save@v4 ++ - uses: actions/cache/save@v5 + if: steps.restore-wt.outputs.cache-hit != 'true' + with: + key: wt-${{ env.WT_VERSION }} @@ .github/workflows/ui-tests.yml (new) + run: | + "$WINDIR/system32/tar.exe" -xf "$RUNNER_TEMP/wt.zip" && + cygpath -aw terminal-$WT_VERSION >>$GITHUB_PATH -+ - uses: actions/cache/restore@v4 ++ - uses: actions/cache/restore@v5 + id: restore-ahk + with: + key: ahk-${{ env.AUTOHOTKEY_VERSION }} @@ .github/workflows/ui-tests.yml (new) + run: | + curl -L -o "$RUNNER_TEMP/ahk.zip" \ + https://github.com/AutoHotkey/AutoHotkey/releases/download/v$AUTOHOTKEY_VERSION/AutoHotkey_$AUTOHOTKEY_VERSION.zip -+ - uses: actions/cache/save@v4 ++ - uses: actions/cache/save@v5 + if: steps.restore-ahk.outputs.cache-hit != 'true' + with: + key: ahk-${{ env.AUTOHOTKEY_VERSION }} @@ .github/workflows/ui-tests.yml (new) + mkdir -p "$RUNNER_TEMP/ahk" && + "$WINDIR/system32/tar.exe" -C "$RUNNER_TEMP/ahk" -xf "$RUNNER_TEMP/ahk.zip" && + cygpath -aw "$RUNNER_TEMP/ahk" >>$GITHUB_PATH -+ - uses: actions/setup-node@v5 # the hook uses node for the background process ++ - uses: actions/setup-node@v6 # the hook uses node for the background process + -+ - uses: actions/checkout@v5 ++ - uses: actions/checkout@v6 + with: + sparse-checkout: | + ui-tests@@ .github/workflows/ui-tests.yml: jobs: run: type bg-hook.log + - name: Upload test results + if: always() -+ uses: actions/upload-artifact@v4 ++ uses: actions/upload-artifact@v6 + with: + name: ui-tests-${{ matrix.os }} + path: ui-tests@@ .github/workflows/ui-tests.yml: jobs: + screenshot $bounds "ui-tests/screenshot.png" - name: Upload test results if: always() - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v6sleepin Windows Terminal can be interrupted@@ .github/workflows/ui-tests.yml: on: @@ .github/workflows/ui-tests.yml: jobs: "$WINDIR/system32/tar.exe" -C "$RUNNER_TEMP/ahk" -xf "$RUNNER_TEMP/ahk.zip" && cygpath -aw "$RUNNER_TEMP/ahk" >>$GITHUB_PATH - - uses: actions/setup-node@v5 # the hook uses node for the background process -+ - uses: actions/cache/restore@v4 + - uses: actions/setup-node@v6 # the hook uses node for the background process ++ - uses: actions/cache/restore@v5 + id: restore-win32-openssh + with: + key: win32-openssh-${{ env.WIN32_OPENSSH_VERSION }} @@ .github/workflows/ui-tests.yml: jobs: + run: | + curl -fLo "$RUNNER_TEMP/win32-openssh.zip" \ + https://github.com/PowerShell/Win32-OpenSSH/releases/download/v$WIN32_OPENSSH_VERSION/OpenSSH-Win64.zip -+ - uses: actions/cache/save@v4 ++ - uses: actions/cache/save@v5 + if: steps.restore-win32-openssh.outputs.cache-hit != 'true' + with: + key: win32-openssh-${{ env.WIN32_OPENSSH_VERSION }} @@ .github/workflows/ui-tests.yml: jobs: + "$WINDIR/system32/tar.exe" -C "$RUNNER_TEMP" -xvf "$RUNNER_TEMP/win32-openssh.zip" && + echo "OPENSSH_FOR_WINDOWS_DIRECTORY=$(cygpath -aw "$RUNNER_TEMP/OpenSSH-Win64")" >>$GITHUB_ENV - - uses: actions/checkout@v5 + - uses: actions/checkout@v6 with: ## ui-tests/ctrl-c.ahk ##pinginterrupt test.bat/.cmdfile extensions firstThis closes git-for-windows/git#6031