Expose the "deep-copy" symlink behavior via a new winsymlinks mode
#16
Conversation
This file contains 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
…/folders. The original patch to change the symbolic link behavior to create deep copies instead did so in a bit of a hacky way: it used the `WSYM_sysfile` code path, not introducing a separate mode. This makes it impossible for users to ask for Cygwin's behavior (and it also poses a maintenance burden: for example, we had to specifically disable Cygwin's code to prefer to create WSL-style symlinks whenever possible instead of using the original Cygwin hack to use special-formed system files). Proposed new commit message when rebasing the MSYS2 patches on top of the next Cygwin runtime version: Instead of creating Cygwin symlinks, use deep copy by default 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]>
Cygwin started to use WSL-style symbolic links by default. Since MSYS2 decided to patch the `WSYM_sysfile` mode to make deep copies instead, that logic had to be disabled specifically. But now that we introduced a separate mode (`WSYM_deepcopy`) for that, and use that by default, we can re-enable Cygwin's code so that users wishing to experience Cygwin's symbolic link behavior can do so by setting `MSYS=winsymlinks:sysfile`. Essentially, this drops the patch "Don't create WSL symlinks for MSYS2". Signed-off-by: Johannes Schindelin <[email protected]>
Alexpux
approved these changes
Oct 21, 2020
|
lgtm |
|
Looks good. (I didn't build though.) |
dscho
added a commit
to dscho/MSYS2-packages
that referenced
this pull request
Oct 22, 2020
This patch synchronizes msys2/MSYS2-packages/msys2-runtime with msys2/msys2-runtime after merging msys2/msys2-runtime#16. We specifically override Cygwin's symlink() behavior to make deep copies instead of creating a special-crafted system file that Cygwin (MSYS2, and only those two) will recognize as symbolic link. The patch to override this behavior is incomplete, though: it hacks up the WSYM_sysfile code path, which causes all kinds of problems: - Contrary to the documentation, MSYS=winsymlinks:native won't fall back to creating Cygwin-style symbolic links. Instead, it (quite surprisingly) creates deep copies instead. - There is no way to opt into the original Cygwin behavior because that code path is now dead code. - The fact that the WSYM_sysfile code path is modified to do something very different from what it is intended to do means that all changes that Cygwin makes to that code path have to be inspected, and occasionally even more hacks have to be put on top, such as 44bb133, where we specifically had to disable Cygwin's code to try creating WSL symlinks before falling back to creating system files instead). Let's fix this by introducing a proper new mode: MSYS=winsymlinks:deepcopy. This mode is made the default so that users should be unaffected by this PR. Users wishing to let the MSYS2 runtime create Cygwin-style symbolic links can ask for that by setting MSYS=winsymlinks:sysfile. By fixing this issue, we can now stop disabling Cygwin's WSL symlink behavior, too: it is in the sysfile code path and won't affect the default behavior of the MSYS2 runtime. While at it, the indentation of the original patch is fixed, too. Signed-off-by: Johannes Schindelin <[email protected]>
This was referenced Oct 22, 2020
Merged
elieux
pushed a commit
to msys2/MSYS2-packages
that referenced
this pull request
Oct 22, 2020
This patch synchronizes msys2/MSYS2-packages/msys2-runtime with msys2/msys2-runtime after merging msys2/msys2-runtime#16. We specifically override Cygwin's symlink() behavior to make deep copies instead of creating a special-crafted system file that Cygwin (MSYS2, and only those two) will recognize as symbolic link. The patch to override this behavior is incomplete, though: it hacks up the WSYM_sysfile code path, which causes all kinds of problems: - Contrary to the documentation, MSYS=winsymlinks:native won't fall back to creating Cygwin-style symbolic links. Instead, it (quite surprisingly) creates deep copies instead. - There is no way to opt into the original Cygwin behavior because that code path is now dead code. - The fact that the WSYM_sysfile code path is modified to do something very different from what it is intended to do means that all changes that Cygwin makes to that code path have to be inspected, and occasionally even more hacks have to be put on top, such as 44bb133, where we specifically had to disable Cygwin's code to try creating WSL symlinks before falling back to creating system files instead). Let's fix this by introducing a proper new mode: MSYS=winsymlinks:deepcopy. This mode is made the default so that users should be unaffected by this PR. Users wishing to let the MSYS2 runtime create Cygwin-style symbolic links can ask for that by setting MSYS=winsymlinks:sysfile. By fixing this issue, we can now stop disabling Cygwin's WSL symlink behavior, too: it is in the sysfile code path and won't affect the default behavior of the MSYS2 runtime. While at it, the indentation of the original patch is fixed, too. 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
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.
In https://gitter.im/msys2/msys2?at=5f8fe84abbffc02b582d6915, @Alexpux lamented that
tardoes not extract symbolic links but makes deep copies instead.The reason for this is that we specifically override Cygwin's
symlink()behavior to make deep copies instead of creating a special-crafted system file that Cygwin (MSYS2, and only those two) will recognize as symbolic link.The patch to override this behavior is incomplete, though: it hacks up the
WSYM_sysfilecode path, which causes all kinds of problems:MSYS=winsymlinks:nativewon't fall back to creating Cygwin-style symbolic links. Instead, it (quite surprisingly) creates deep copies instead.WSYM_sysfilecode path is modified to do something very different from what it is intended to do means that all changes that Cygwin makes to that code path have to be inspected, and occasionally even more hacks have to be put on top, such as 44bb133, where we specifically had to disable Cygwin's code to try creating WSL symlinks before falling back to creating system files instead).Let's fix this by introducing a proper new mode:
MSYS=winsymlinks:deepcopy. This mode is made the default so that users should be unaffected by this PR.Users wishing to let the MSYS2 runtime create Cygwin-style symbolic links can ask for that by setting
MSYS=winsymlinks:sysfile.By fixing this issue, we can now stop disabling Cygwin's WSL symlink behavior, too: it is in the
sysfilecode path and won't affect the default behavior of the MSYS2 runtime.While at it, the indentation of the original patch is fixed, too.