Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Investigate symlink patch breaking subst paths #41

Closed
lazka opened this issue May 5, 2021 · 26 comments · Fixed by #46
Closed

Investigate symlink patch breaking subst paths #41

lazka opened this issue May 5, 2021 · 26 comments · Fixed by #46

Comments

@lazka
Copy link
Member

lazka commented May 5, 2021

See #40 -> msys2/MSYS2-packages#2471 (comment)

Upstream questions:

@jeremyd2019
Copy link
Member

Compare incoming path with path returned by NtQueryInformationFile(FileNameInformation). If they differ, bail out as if the file doesn't exist. This forces path_conv::check to backtrack inner path components.

This works if the reason they differ is because there were symlinks involved somewhere in the path, but does not work if there's some other reason (WoW fs redirection and subst have been revealed so far). Not so sure that that was a safe assumption to make

@jeremyd2019
Copy link
Member

https://cygwin.com/pipermail/cygwin/2021-May/248457.html

So now the question becomes, what's different between cygwin and msys2-runtime that would cause this...

@lazka
Copy link
Member Author

lazka commented May 7, 2021

fstab setup makes the difference: https://cygwin.com/pipermail/cygwin/2021-May/248458.html

@jeremyd2019
Copy link
Member

https://cygwin.com/pipermail/cygwin/2021-May/248466.html
19d59ce

I wonder if treating subst paths as symlinks will cause issues for me using them as a way to get shorter paths for builds. Seems like things could silently 'dereference' them back to their too-long variants.

@lazka
Copy link
Member Author

lazka commented May 8, 2021

Just tried and cygpath de-references the symlink when converting to a Windows path.

@jeremyd2019
Copy link
Member

☹️

@lazka
Copy link
Member Author

lazka commented May 20, 2021

fixed #42 (comment)

@lazka lazka closed this as completed May 20, 2021
@jeremyd2019
Copy link
Member

It appears this was only fixed for the root of a SUBST drive... https://cygwin.com/pipermail/cygwin/2021-May/248612.html

@jeremyd2019 jeremyd2019 reopened this May 28, 2021
jeremyd2019 added a commit to jeremyd2019/msys2-runtime that referenced this issue May 29, 2021
jeremyd2019 added a commit to jeremyd2019/msys2-runtime that referenced this issue May 29, 2021
jeremyd2019 added a commit to jeremyd2019/msys2-runtime that referenced this issue May 29, 2021
@jeremyd2019
Copy link
Member

jeremyd2019 commented May 29, 2021

https://cygwin.com/pipermail/cygwin-patches/2021q2/011405.html

That turned out not to help, as makepkg calls pwd -P which intentionally dereferences symlinks.

@jeremyd2019
Copy link
Member

With the patch above, plus patching msys2_path_conv.cc posix_to_win32_path to use the new flag, AND changing pwd -P to pwd in /usr/bin/makepkg and /usr/share/makepkg/util/util.sh, mingw-w64-python-sphinxcontrib-serializinghtml was able to package successfully.

https://cygwin.com/pipermail/cygwin-patches/2021q2/011407.html

@jeremyd2019
Copy link
Member

https://cygwin.com/pipermail/cygwin-patches/2021q2/011408.html

I also had to patch out pwd -P into pwd in makepkg, but did not have to change msys2_path_conv.cc.

I wonder if Corinna is taking the long weekend off or something. Everybody's entitled to a break, but I want to know if I'm on the right track 🤞

@selectstriker2
Copy link

selectstriker2 commented Jun 4, 2021

I believe this is the issue I'm running into after updating to msys2-runtime-3.2.0-8.

Background
Building with arm-none-eabi-gcc and make hard codes the full path into the ELF file for debugging.
Eclipse is working from a project based in /h/project but now the path is being provided to make and gcc as //some.server/h/project. This breaks the ability to set breakpoints in source code using Eclipse because the paths don't match.

Rolling back to 3.2.0-6 seems to have resolved my issue for now.

@jeremyd2019
Copy link
Member

If you don't mind, maybe chime in on cygwin@cygwin.com with your case. Also, I could open a PR here with my latest patch so you could get a binary to try, to see if it helps your scenario. I don't know if it will or not, because cygwin processes will still see them as symlinks and may dereference them if they choose to.

@jeremyd2019
Copy link
Member

#46

@jeremyd2019
Copy link
Member

the combination of #46 and msys2/msys2-pacman#3 mostly works for me.

@jeremyd2019
Copy link
Member

the combination of #46 and msys2/msys2-pacman#3 mostly works for me.

I noticed in a test build that that wxPython-4.1.1 freaked out in this scenario, but was fine when the working copy was a 'real' path. I'm guessing because waf is using msys python, it could "see" the symlink and was confused.

@jeremyd2019
Copy link
Member

For the record, before I lose it, there was another report of this new behavior causing problems:
https://gitter.im/msys2/msys2?at=60d30baabef0c66d9d1ce65d

syarif-hsb @syarif-hsb Jun 23 03:23
Hello, how do I change the mounting of my msys root? I usually have it on H:/msys64. But last update, I think I did something that change the mount to //<network_name>/msys64. This disturb my workflow. Anybody know how to fix it?

@jeremyd2019
Copy link
Member

Thoughts about this? Should we try my workaround patch #46 (ish, that may be out of date by now, I think there was a formatting revision after that). Or just disable this block of code altogether and go back to the old behavior that worked for everyone but cmake tests?

@jeremyd2019
Copy link
Member

I'm getting to the point that I just want to wrap the block of code that tries to resolve inner native symlinks in if (0) in msys2 fork and be done with it. Maybe make it an option, but that seems like a cop-out.

jeremyd2019 added a commit to jeremyd2019/msys2-runtime that referenced this issue Jul 3, 2021
@jeremyd2019
Copy link
Member

Part of #46 was applied upstream: https://cygwin.com/pipermail/cygwin-patches/2021q3/011440.html

The change to make chdir pass the flag to not dereference "reparse points" was not applied, so the cwd for Windows programs will still be dereferencing subst/mapped drives. Also, there were cases where even that was not enough (having to patch makepkg to not call pwd -P, and wxPython build, and I think I've seen 'dereferenced' paths show up in the build process that haven't caused errors too)

@jeremyd2019
Copy link
Member

https://cygwin.com/pipermail/cygwin-patches/2021q3/011444.html

In response to making an option to disabling this:

If MSYS2 has to patch the code anyway, why not go the entire way and
move the full patch to MSYS2? Lets not add more CYGWIN options for rare
border cases, please.

So, what do you guys think about an MSYS2 patch to make it an option? Or just disable it like #54 and be done with it?

@selectstriker2
Copy link

Honestly, I'm not even sure what the original change was trying to resolve, as I've never run across a situation where I would need it. I'd say either disable it or put it under a flag that isn't used by default. But that's me as an MSYS2 user

@jeremyd2019
Copy link
Member

It was trying to solve some errors in cmake's test suite IIRC. #38 (comment)

@jeremyd2019
Copy link
Member

This seems to be a fairly rare situation, so I am mostly considering an option that enables this code by default, and the handful of people who use mapped network drives or subst and run into issues can be told to enable the option.

@selectstriker2
Copy link

Does that look like just setting an environment variable for the given flag?

@jeremyd2019
Copy link
Member

jeremyd2019 commented Jul 8, 2021

yeah. https://cygwin.com/cygwin-ug-net/using-cygwinenv.html except it's called MSYS here, and there are a few differing options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants