Skip to content

Conversation

@dscho
Copy link
Collaborator

@dscho dscho commented Jan 26, 2025

Range-diff to v3.5.5
  • 1: bdad035 = 1: 61c7294 Add MSYS2 triplet

  • 2: 900232b = 2: e6895db Fix msys library name in import libraries

  • 3: 585fadf = 3: e501600 Rename dll from cygwin to msys

  • 4: d076660 = 4: 70cdcba Add functionality for converting UNIX paths in arguments and environment variables to Windows form for native Win32 applications.

  • 5: 35a477b = 5: d6cd58e Add functionality for changing OS name via MSYSTEM environment variables.

  • 6: 41ed62e = 6: e7d3c8f - Move root to /usr. - Change sorting mount points. - By default mount without ACLs. - Can read /etc/fstab with short mount point format.

  • 7: 8096583 = 7: 8c5cc28 Instead of creating Cygwin symlinks, use deep copy by default

  • 8: 0ba54e8 = 8: e907429 Automatically rewrite TERM=msys to TERM=cygwin

  • 9: e28b742 = 9: 8102cde Do not convert environment for strace

  • 10: 4a24304 = 10: c68f8fd strace.cc: Don't set MSYS=noglob

  • 11: b3a4a5b = 11: e6cc70d Add debugging for strace make_command_line

  • 12: f8c0980 = 12: e25ab86 strace --quiet: be really quiet

  • 13: c4d9d83 = 13: 8e18233 path_conv: special-case root directory to have trailing slash

  • 14: 1c5a6e8 = 14: 0c102ea When converting to a Unix path, avoid double trailing slashes

  • 15: 8b5e9b5 = 15: 8ed6549 msys2_path_conv: pass PC_NOFULL to path_conv

  • 16: b09fb7f = 16: 853cc54 path-conversion: Introduce ability to switch off conversion.

  • 17: b239180 = 17: 5d54a24 dcrt0.cc: Untangle allow_glob from winshell

  • 18: 1e94f3e = 18: d85da3a dcrt0.cc (globify): Don't quote literal strings differently when dos_spec

  • 19: e6c5e3e = 19: a3e7629 Add debugging for build_argv

  • 20: d44fc10 = 20: ca226ec environ.cc: New facility/environment variable MSYS2_ENV_CONV_EXCL

  • 21: 98c1641 = 21: 89f8cac Fix native symbolic link spawn passing wrong arg0

  • 22: d12ab6d = 22: a46c753 Introduce the enable_pcon value for MSYS

  • 23: 865ad5d = 23: 5f0df24 popen: call /usr/bin/sh instead of /bin/sh

  • 24: 5f36fdb = 24: c1a9410 Disable the 'cygwin' GitHub workflow

  • 25: 862887f ! 25: 9354423 CI: add a GHA for doing a basic build test

    @@ .github/workflows/build.yaml (new)
     +        shell: msys2 {0}
     +        run: |
     +          (cd winsup && ./autogen.sh)
    -+          ./configure --disable-dependency-tracking
    ++          ./configure --disable-dependency-tracking --with-msys2-runtime-commit="$GITHUB_SHA"
     +          make -j8
     +
     +      - name: Install
  • 26: 764bd7f ! 26: 3edbe01 CI: fix the build with gcc 13

    @@ .github/workflows/build.yaml: jobs:
     +          # resulting in errors due to -Werror. Disable them for now.
     +          export CXXFLAGS="-Wno-error=stringop-truncation -Wno-error=array-bounds -Wno-error=overloaded-virtual -Wno-narrowing -Wno-use-after-free"
                (cd winsup && ./autogen.sh)
    -           ./configure --disable-dependency-tracking
    +           ./configure --disable-dependency-tracking --with-msys2-runtime-commit="$GITHUB_SHA"
                make -j8
  • 27: bea6a07 = 27: d49861d Set up a GitHub Action to keep in sync with Cygwin

  • 28: 080e2b0 = 28: 6d0c637 Expose full command-lines to other Win32 processes by default

  • 29: 9f45f58 = 29: 0e5a7de Add a helper to obtain a function's address in kernel32.dll

  • 30: f6f7e2c = 30: 98632e1 Emulate GenerateConsoleCtrlEvent() upon Ctrl+C

  • 31: 86cd50d = 31: c890b0c kill: kill Win32 processes more gently

  • 32: 4b228ef = 32: 5ab3720 Cygwin: make option for native inner link handling.

  • 33: 6e0c3b5 = 33: 1fcaaa3 docs: skip building texinfo and PDF files

  • 34: ecf50e6 = 34: bbd5897 install-libs: depend on the "toollibs"

  • 35: 8cb4ff2 = 35: 7e40bdf POSIX-ify the SHELL variable

  • 36: 1f6020c = 36: 4c80b59 Handle ORIGINAL_PATH just like PATH

  • 37: 79e3880 = 37: 0c124b9 uname: allow setting the system name to CYGWIN

  • 38: ef59c45 = 38: 54d1ac9 Pass environment variables with empty values

  • 39: c347d6c = 39: 6fe00e2 Optionally disallow empty environment values again

  • 40: 210e5f3 = 40: 82d1e3b build_env(): respect the MSYS environment variable

  • 41: bef1e6b = 41: 855f33f Revert "Cygwin: Enable dynamicbase on the Cygwin DLL by default"

  • 42: 8103a09 ! 42: c8c911a CI: set -Wno-error=maybe-uninitialized

    @@ .github/workflows/build.yaml: jobs:
     -          export CXXFLAGS="-Wno-error=stringop-truncation -Wno-error=array-bounds -Wno-error=overloaded-virtual -Wno-narrowing -Wno-use-after-free"
     +          export CXXFLAGS="-Wno-error=stringop-truncation -Wno-error=array-bounds -Wno-error=overloaded-virtual -Wno-narrowing -Wno-use-after-free -Wno-error=maybe-uninitialized"
                (cd winsup && ./autogen.sh)
    -           ./configure --disable-dependency-tracking
    +           ./configure --disable-dependency-tracking --with-msys2-runtime-commit="$GITHUB_SHA"
                make -j8
  • 43: 6e10b1b ! 43: f840e12 Avoid sharing cygheaps across Cygwin versions

    @@ winsup/configure.ac: AC_CHECK_TOOL(RANLIB, ranlib, ranlib)
     +    then
     +        MSYS2_RUNTIME_COMMIT_HEX="0x$(expr "$MSYS2_RUNTIME_COMMIT" : '\(.\{,8\}\)')ull"
     +    else
    -+        AC_MSG_WARN([Could not determine msys2-runtime commit"])
    ++        AC_MSG_WARN([Could not determine msys2-runtime commit])
     +        MSYS2_RUNTIME_COMMIT=
     +        MSYS2_RUNTIME_COMMIT_HEX=0
     +    fi
  • 44: f61f7fb ! 44: 905cab1 uname: report msys2-runtime commit hash, too

    @@ winsup/configure.ac: AC_ARG_WITH([msys2-runtime-commit],
     +        MSYS2_RUNTIME_COMMIT_SHORT="$(expr "$MSYS2_RUNTIME_COMMIT" : '\(.\{,8\}\)')"
     +        MSYS2_RUNTIME_COMMIT_HEX="0x${MSYS2_RUNTIME_COMMIT_SHORT}ul"
          else
    -         AC_MSG_WARN([Could not determine msys2-runtime commit"])
    +         AC_MSG_WARN([Could not determine msys2-runtime commit])
              MSYS2_RUNTIME_COMMIT=
     +        MSYS2_RUNTIME_COMMIT_SHORT=
              MSYS2_RUNTIME_COMMIT_HEX=0
  • 45: debafbf = 45: 856e949 Cygwin: find_fast_cwd: don't run assembler checking code on ARM64

  • 46: 2c55ca5 = 46: c94b46b cygthread: suspend thread before terminating.

  • 47: e5dc132 = 47: 62f6088 Cygwin: revert use of CancelSyncronousIo on wait_thread.

  • 48: 5453f9f = 48: 7f3ae80 Cygwin: cache IsWow64Process2 host arch in wincap.

  • 49: 985e265 = 49: 7da06fd Cygwin: uname: add host machine tag to sysname.

  • 50: 33799c2 < -: ---------- Cygwin: signal: Do not handle signal when __SIGFLUSHFAST is sent

  • 51: 964efe3 < -: ---------- fixup! CI: add a GHA for doing a basic build test

  • 52: 2570087 < -: ---------- fixup! Avoid sharing cygheaps across Cygwin versions

  • 53: bfb6ba0 < -: ---------- fixup! CI: add a GHA for doing a basic build test

  • 54: 674137e < -: ---------- Revert "Cygwin: signal: Do not handle signal when __SIGFLUSHFAST is sent"

  • 55: 16b8373 < -: ---------- Cygwin: cygwait: Make cygwait() reentrant

  • 56: ae26e17 < -: ---------- Cygwin: signal: Do not handle signal when __SIGFLUSHFAST is sent

  • 57: 25a3862 < -: ---------- Cygwin: signal: Avoid frequent TLS lock/unlock for SIGCONT processing

  • -: ---------- > 50: 2c0e426 fixup! Instead of creating Cygwin symlinks, use deep copy by default

Alexpux and others added 30 commits January 26, 2025 20:20
Cygwin's speclib doesn't handle dashes or dots. However, we are about to
rename the output file name from `cygwin1.dll` to `msys-2.0.dll`.

Let's preemptively fix up all the import libraries that would link
against `msys_2_0.dll` to correctly link against `msys-2.0.dll` instead.
…ent variables to Windows form for native Win32 applications.
…t without ACLs. - Can read /etc/fstab with short mount point format.
The new `winsymlinks` mode `deepcopy` (which is made the default) lets
calls to `symlink()` create (deep) copies of the source file/directory.

This is necessary because unlike Cygwin, MSYS2 does not try to be its
own little ecosystem that lives its life separate from regular Win32
programs: the latter have _no idea_ about Cygwin-emulated symbolic links
(i.e. system files whose contents start with `!<symlink>\xff\xfe` and
the remainder consists of the NUL-terminated, UTF-16LE-encoded symlink
target).

To support Cygwin-style symlinks, the new mode `sysfile` is introduced.

Co-authored-by: Johannes Schindelin <[email protected]>
With MSys1, it was necessary to set the TERM variable to "msys". To
allow for a smooth transition from MSys1 to MSys2, let's simply handle
TERM=msys as if the user had not specified TERM at all and wanted us to
use our preferred TERM value.
Strace is a Windows program so MSYS2 will convert all arguments and environment vars and that makes debugging msys2 software with strace very tricky.
Commit message for this code was:

* strace.cc (create_child): Set CYGWIN=noglob when starting new process so that

  Cygwin will leave already-parsed the command line alonw."

I can see no reason for it and it badly breaks the ability to use
strace.exe to investigate calling a Cygwin program from a Windows
program, for example:
strace mingw32-make.exe
.. where mingw32-make.exe finds sh.exe and uses it as the shell.
The reason it badly breaks this use-case is because dcrt0.cc depends
on globbing to happen to parse commandlines from Windows programs;
irrespective of whether they contain any glob patterns or not.

See quoted () comment:
"This must have been run from a Windows shell, so preserve
 quotes for globify to play with later."
The biggest problem with strace spitting out `create_child: ...` despite
being asked to be real quiet is that its output can very well interfere
with scripts' operations.

For example, when running any of Git for Windows' shell scripts with
`GIT_STRACE_COMMANDS=/path/to/logfile` (which is sadly an often needed
debugging technique while trying to address the many MSYS2 issues Git for
Windows faces), any time the output of any command is redirected into a
variable, it will include that `create_child: ...` line, wreaking havoc
with Git's expectations.

So let's just really be quiet when we're asked to be quiet.

Signed-off-by: Johannes Schindelin <[email protected]>
When converting `/c/` to `C:\`, the trailing slash is actually really
necessary, as `C:` is not an absolute path.

We must be very careful to do this only for root directories, though. If
we kept the trailing slash also for, say, `/y/directory/`, we would run
into the following issue: On FAT file systems, the normalized path is
used to fake inode numbers. As a result, `Y:\directory\` and
`Y:\directory` have different inode numbers!!!

This would result in very non-obvious symptoms. Back when we were too
careless about keeping the trailing slash, it was reported to the Git
for Windows project that the `find` and `rm` commands can error out on
FAT file systems with very confusing "No such file or directory" errors,
for no good reason.

During the original investigation, Vasil Minkov pointed out in
git-for-windows/git#1497 (comment),
that this bug had been fixed in Cygwin as early as 1997... and the bug
was unfortunately reintroduced into early MSYS2 versions.

Signed-off-by: Johannes Schindelin <[email protected]>
When calling `cygpath -u C:/msys64/` in an MSYS2 setup that was
installed into `C:/msys64/`, the result should be `/`, not `//`.

Let's ensure that we do not append another trailing slash if the
converted path already ends in a slash.

This fixes #112

Signed-off-by: Johannes Schindelin <[email protected]>
In theory this doesn't make a difference because posix_to_win32_path()
is only called with rooted/absolute paths, but as pointed out in
#103 PC_NOFULL will preserve
the trailing slash of unix paths (for some reason).

See "cygpath -m /bin/" (preserved) vs "cygpath -am /bin/" (dropped)

One use case where we need to trailing slashes to be preserved is the GCC build
system:
https://github.com/gcc-mirror/gcc/blob/6d82e0fea5f988e829912a/gcc/Makefile.in#L2314

The Makefile appends a slash to the prefixes and the C code doing relocation will
treat the path as a directory if there is a trailing slash. See
msys2/MINGW-packages#14173 for details.

With this change all our MSYS2 path_conv tests pass again.
When calling windows native apps from MSYS2, the runtime tries to
convert commandline arguments by a specific set of rules. This idea was
inherited from the MSys/MinGW project (which is now seemingly stale, yet
must be credited with championing this useful feature, see MinGW wiki
https://web.archive.org/web/20201112005258/http://www.mingw.org/wiki/Posix_path_conversion).

If the user does not want that behavior on a big scale, e.g. inside a
Bash script, with the changes introduced in this commit, the user can
now set the the environment variable `MSYS_NO_PATHCONV` when calling
native windows commands.

This is a feature that has been introduced in Git for Windows via
git-for-windows/msys2-runtime#11 and it predates
support for the `MSYS2_ENV_CONV_EXCL` and `MSYS2_ARG_CONV_EXCL`
environment variables in the MSYS2 runtime; Many users find the
simplicity of `MSYS_NO_PATHCONV` appealing.

So let's teach MSYS2 proper this simple trick that still allows using
the sophisticated `MSYS2_*_CONV_EXCL` facilities but also offers a
convenient catch-all "just don't convert anything" knob.

Signed-off-by: 마누엘 <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
Otherwise if globbing is allowed and we get called from a
Windows program, build_argv thinks we've been called from
a Cygwin program.
…spec

Reverts 25ba8f3. I can't figure out what
the intention was. I'm sure I'll find out soon enough when everything breaks.

This change means that input of:
  '"C:/test.exe SOME_VAR=\"literal quotes\""'

becomes:
  'C:/test.exe SOME_VAR="literal quotes"'

instead of:
  'C:/test.exe SOME_VAR=\literal quotes\'

.. which is at least consistent with the result for:
  '"no_drive_or_colon SOME_VAR=\"literal quotes\""'

The old result of course resulted in the quoted string being split into
two arguments at the space which is clearly not intended.

I *guess* backslashes in dos paths may have been the issue here?
If so I don't care since we should not use them, ever, esp. not at
the expense of sensible forward-slash-containing input.
Works very much like MSYS2_ARG_CONV_EXCL. In fact it uses the same
function, arg_heuristic_with_exclusions (). Also refactors parsing
the env. variables to use new function, string_split_delimited ().

The env. that is searched through is the merged (POSIX + Windows)
one. It remains to be seen if this should be made an option or not.

This feature was prompted because the R language (Windows exe) calls
bash to run configure.win, which then calls back into R to read its
config variables (LOCAL_SOFT) and when this happens, msys2-runtime
converts R_ARCH from "/x64" to an absolute Windows path and appends
it to another absolute path, R_HOME, forming an invalid path.
It is simply the negation of `disable_pcon`, i.e. `MSYS=enable_pcon` is
equivalent to `MSYS=nodisable_pcon` (the former is slightly more
intuitive than the latter) and likewise `MSYS=noenable_pcon` is
equivalent to `MSYS=disable_pcon` (here, the latter is definitely more
intuitive than the former).

This is needed because we just demoted the pseudo console feature to be
opt-in instead of opt-out, and it would be awkward to recommend to users
to use "nodisable_pcon"... "nodisable" is not even a verb.

Signed-off-by: Johannes Schindelin <[email protected]>
We mount /usr/bin to /bin, but in a chroot this is broken and we
have no /bin, so try to use the real path.

chroot is used by pacman to run install scripts when called with --root
and this broke programs in install scripts calling popen()
(install-info from texinfo for example)

There are more paths hardcoded to /bin in cygwin which might also be broken
in this scenario, so this maybe should be extended to all of them.
It does not work at all. For example, `rpm -E %fedora` says that there
should be version 33 of rpmsphere at
https://github.com/rpmsphere/noarch/tree/master/r, but there is only
version 32.

Another thing that is broken: Cygwin now assumes that a recent
mingw-w64-headers version is available, but Fedora apparently only
offers v7.0.0, which is definitely too old to accommodate for the
expectation of cygwin/cygwin@c1f7c4d1b6d7.

Signed-off-by: Johannes Schindelin <[email protected]>
Build with --disable-dependency-tracking because we only build once
and this saves 3-4 minutes in CI.
This will help us by automating an otherwise tedious task.

Signed-off-by: Johannes Schindelin <[email protected]>
In the Cygwin project, it was decided that the command-line of Cygwin
processes, as shown in the output of `wmic process list`, would suffer
from being truncated to 32k (and is transmitted to the child process via
a different mechanism, anyway), and therefore only the absolute path of
the executable is shown by default.

Users who would like to see the full command-line (even if it is
truncated) are expected to set `CYGWIN=wincmdln` (or, in MSYS2's case,
`MSYS=wincmdln`).

Seeing as MSYS2 tries to integrate much better with the surrounding
Win32 ecosystem than Cygwin, it makes sense to turn this on by default.

Users who wish to suppress it can still set `MSYS=nowincmdln`.

Signed-off-by: Johannes Schindelin <[email protected]>
In particular, we are interested in the address of the CtrlRoutine
and the ExitProcess functions. Since kernel32.dll is loaded first thing,
the addresses will be the same for all processes (matching the
CPU architecture, of course).

This will help us with emulating SIGINT properly (by not sending signals
to *all* processes attached to the same Console, as
GenerateConsoleCtrlEvent() would do).

Co-authored-by: Naveen M K <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
This patch is heavily inspired by the Git for Windows' strategy in
handling Ctrl+C.

When a process is terminated via TerminateProcess(), it has no chance to
do anything in the way of cleaning up. This is particularly noticeable
when a lengthy Git for Windows process tries to update Git's index file
and leaves behind an index.lock file. Git's idea is to remove the stale
index.lock file in that case, using the signal and atexit handlers
available in Linux. But those signal handlers never run.

Note: this is not an issue for MSYS2 processes because MSYS2 emulates
Unix' signal system accurately, both for the process sending the kill
signal and the process receiving it. Win32 processes do not have such a
signal handler, though, instead MSYS2 shuts them down via
`TerminateProcess()`.

For a while, Git for Windows tried to use a gentler method, described in
the Dr Dobb's article "A Safer Alternative to TerminateProcess()" by
Andrew Tucker (July 1, 1999),
http://www.drdobbs.com/a-safer-alternative-to-terminateprocess/184416547

Essentially, we injected a new thread into the running process that does
nothing else than running the ExitProcess() function.

However, this was still not in line with the way CMD handles Ctrl+C: it
gives processes a chance to do something upon Ctrl+C by calling
SetConsoleCtrlHandler(), and ExitProcess() simply never calls that
handler.

So for a while we tried to handle SIGINT/SIGTERM by attaching to the
console of the command to interrupt, and generating the very same event
as CMD does via GenerateConsoleCtrlEvent().

This method *still* was not correct, though, as it would interrupt
*every* process attached to that Console, not just the process (and its
children) that we wanted to signal. A symptom was that hitting Ctrl+C
while `git log` was shown in the pager would interrupt *the pager*.

The method we settled on is to emulate what GenerateConsoleCtrlEvent()
does, but on a process by process basis: inject a remote thread and call
the (private) function kernel32!CtrlRoutine.

To obtain said function's address, we use the dbghelp API to generate a
stack trace from a handler configured via SetConsoleCtrlHandler() and
triggered via GenerateConsoleCtrlEvent(). To avoid killing each and all
processes attached to the same Console as the MSYS2 runtime, we modify
the cygwin-console-helper to optionally print the address of
kernel32!CtrlRoutine to stdout, and then spawn it with a new Console.

Note that this also opens the door to handling 32-bit process from a
64-bit MSYS2 runtime and vice versa, by letting the MSYS2 runtime look
for the cygwin-console-helper.exe of the "other architecture" in a
specific place (we choose /usr/libexec/, as it seems to be the
convention for helper .exe files that are not intended for public
consumption).

The 32-bit helper implicitly links to libgcc_s_dw2.dll and
libwinpthread-1.dll, so to avoid cluttering /usr/libexec/, we look for
the helped of the "other" architecture in the corresponding mingw32/ or
mingw64/ subdirectory.

Among other bugs, this strategy to handle Ctrl+C fixes the MSYS2 side of
the bug where interrupting `git clone https://...` would send the
spawned-off `git remote-https` process into the background instead of
interrupting it, i.e. the clone would continue and its progress would be
reported mercilessly to the console window without the user being able
to do anything about it (short of firing up the task manager and killing
the appropriate task manually).

Note that this special-handling is only necessary when *MSYS2* handles
the Ctrl+C event, e.g. when interrupting a process started from within
MinTTY or any other non-cmd-based terminal emulator. If the process was
started from within `cmd.exe`'s terminal window, child processes are
already killed appropriately upon Ctrl+C, by `cmd.exe` itself.

Also, we can't trust the processes to end it's subprocesses upon receiving
Ctrl+C. For example, `pip.exe` from `python-pip` doesn't kill the python
it lauches (it tries to but fails), and I noticed that in cmd it kills python
also correctly, which mean we should kill all the process using
`exit_process_tree`.

Co-authored-by: Naveen M K <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
If the Cygwin dll's architecture is different from the host system's
architecture, append an additional tag that indicates the host system
architecture (the Cygwin dll's architecture is already indicated in
machine).

Signed-off-by: Jeremy Drake <[email protected]>
(cherry picked from commit 7923059)
@dscho dscho requested a review from jeremyd2019 January 26, 2025 19:26
@dscho dscho self-assigned this Jan 26, 2025
@dscho
Copy link
Collaborator Author

dscho commented Jan 26, 2025

  • 46: 2c55ca5 = 46: c94b46b cygthread: suspend thread before terminating.

  • 47: e5dc132 = 47: 62f6088 Cygwin: revert use of CancelSyncronousIo on wait_thread.

@jeremyd2019 do we want to drop these?

@jeremyd2019
Copy link
Member

  • 46: 2c55ca5 = 46: c94b46b cygthread: suspend thread before terminating.
  • 47: e5dc132 = 47: 62f6088 Cygwin: revert use of CancelSyncronousIo on wait_thread.

@jeremyd2019 do we want to drop these?

No, these are the fixes for the arm64 hang, they did not get backported to cygwin-3_5-branch. Maybe squash them into one if you really want to, but they went into cygwin/master like that...

@dscho
Copy link
Collaborator Author

dscho commented Jan 26, 2025

  • 46: 2c55ca5 = 46: c94b46b cygthread: suspend thread before terminating.
  • 47: e5dc132 = 47: 62f6088 Cygwin: revert use of CancelSyncronousIo on wait_thread.

@jeremyd2019 do we want to drop these?

No, these are the fixes for the arm64 hang, they did not get backported to cygwin-3_5-branch.

Ah, sorry, I thought that the actual fixes went into cygwin-3_5-branch and that these were some extra left-overs from earlier attempts. Thank you for clarifying!

Maybe squash them into one if you really want to, but they went into cygwin/master like that...

Nah, better keep them as-are, so that the rebase to v3.6 will be able to figure out automatically that they are upstream already.

@dscho dscho mentioned this pull request Jan 26, 2025
@dscho dscho marked this pull request as ready for review January 26, 2025 20:22
@dscho dscho marked this pull request as draft January 26, 2025 20:23
@dscho
Copy link
Collaborator Author

dscho commented Jan 26, 2025

Uh oh. Git's test suite is not happy:

image

I suspect that my fixup!s are incorrect because those failures seem to occur around symlinks, which the fixup! code path touches (search for "not ok" in the "Search logs" box on the failed job's page).

@dscho
Copy link
Collaborator Author

dscho commented Jan 26, 2025

I suspect that my fixup!s are incorrect

Well, one mistake that is quite obvious is that !isdevice() should be converted to ondisk() (note the missing ! in the latter expression, which is what I missed 🤦 when I tried to do the rebase "on the side"). Let's see whether inverting the logic as needed fixes the test failures.

@lazka
Copy link
Member

lazka commented Jan 26, 2025

@dscho did you see the inline comment above? #254 (review)

@dscho
Copy link
Collaborator Author

dscho commented Jan 26, 2025

@dscho did you see the inline comment above? #254 (review)

Nope, I totally missed this. I am currently trying (since Friday) to convert a complex Azure Pipeline to a GitHub workflow (I really wish that GitHub invested anything in GitHub Actions, including features to make debugging a lot less cumbersome), all while being sick, and I know that it's an idiotic idea of mine that I also do the MSYS2 runtime on the side. Sorry about that. Feel free to ignore me for now.

@lazka
Copy link
Member

lazka commented Jan 26, 2025

np, thanks for your work.

@dscho dscho marked this pull request as ready for review January 26, 2025 22:15
@dscho dscho force-pushed the tentative/msys2-3.5.6 branch 2 times, most recently from a2cfafe to 16f69cd Compare January 27, 2025 20:10
In Cygwin v3.5.6, the `isdevice()` method was dropped, and the commit
message of 002aad0 (Cygwin: path_conv: simplify, rearrange, rename
combined device checks, 2025-01-21), which is the commit that dropped
it, explains:

    Drop the isdevice() check in favor of a new isondisk() check.

On the face of it, the message is clear: we should use `isondisk()`
instead of `isdevice()`.

As it turns out, MSYS2's code that pretends to create a symbolic link
upon `ln -s` but instead makes a deep copy used to call `isdevice()`:

	if (!src_path.isdevice () && !src_path.is_fs_special ())
		// do a deep copy and return

	// otherwise fall back to creating a sysfile link

A deep dive into the issue reveals that in this instance, the condition
is actually equivalent to:

	if (!src_path.isspecial() ||
	    (src_path.isfifo() && !src_path.isondisk()))

Further study (by trying it out) reveals that replacing this condition
with `src_path.isondisk()` fails to produce the expected result. Here is
the expected result, which we receive whether we use MSYS2 runtime
v3.5.5 or the version with `src_path.isspecial()`:

	$ mknod foo c 1 3
	$ ln -s foo foolnk
	$ ls -l foo*
	crw-rw-rw- 1 x None 1, 3 Jan 27 10:43 foo
	lrwxrwxrwx 1 x None    3 Jan 27 10:43 foolnk -> foo

and with `isondisk()`, this happens instead:

	$ mknod foo c 1 3
	$ ln -s foo foolnk
	$ ls -l foo*
	crw-rw-rw- 1 x None  1, 3 Jan 27 10:41 foo
	-r--r--r-- 1 x None   126 Jan 27 10:41 foolnk

Further experimentation reveals that FIFOs are handled without bothering
with the `isfifo() && !isondisk()` condition, merely testing
`!isspecial()` is enough (and recapitulates the behavior of v3.5.5):

	$ mkfifo fifo
	$ ln -s fifo fifolnk
	$ ls -l fifo fifolnk
	prw-rw-rw- 1 None 0 Jan 27 20:44 fifo
	lrwxrwxrwx 1 None 4 Jan 27 20:44 fifolnk -> fifo

Therefore, let's just go with the sweet-and-simple `!isspecial()`.

This commit will live un-autosquashed in the `msys2-3.5.6` branch to
allow for all of the above to be documented in the commit history.

Helped-by: Jeremy Drake <[email protected]>
Signed-off-by: Johannes Schindelin <[email protected]>
@jeremyd2019 jeremyd2019 force-pushed the tentative/msys2-3.5.6 branch from 16f69cd to 2c0e426 Compare January 27, 2025 22:00
@dscho dscho merged commit 2c0e426 into msys2-3.5.6 Jan 28, 2025
2 checks passed
@dscho dscho deleted the tentative/msys2-3.5.6 branch January 28, 2025 06:51
dscho added a commit to dscho/MSYS2-packages that referenced this pull request Jan 28, 2025
This corresponds to msys2/msys2-runtime#254.

Signed-off-by: Johannes Schindelin <[email protected]>
@dscho
Copy link
Collaborator Author

dscho commented Jan 28, 2025

msys2/MSYS2-packages#5167

lazka pushed a commit to msys2/MSYS2-packages that referenced this pull request Jan 28, 2025
This corresponds to msys2/msys2-runtime#254.

Signed-off-by: Johannes Schindelin <[email protected]>
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.

8 participants