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

Two small changes to make by default, OOTB, work better with msys2. #831

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from

Conversation

xeiavicaflashrepository

msys2 is a rather popular semi-fork of cygwin to bring linux software to microsoft windows.

I made two tiny additions to make it work OTTB better with windows without having to patch it ourselves.

First one is if $HOSTTYPE is detected as msys2, do something differently from cygwin because of how symlinks work in msys2 compared to cygwin. Second one is to add a shebang to the start of src/cmd/INIT/mamprobe.sh because windows cannot load the file without sed-ing the shebang into it. This shouldn't make a difference because it's already a shell script to begin with.

I would like to see better type and HOSTTYPE detection with msys2. Let me know your thoughts and how I can change the patch to be better fitting for ksh93 and msys2.

@xeiavicaflashrepository
Copy link
Author

Also, I should add, there already exists someone with problems with msys2. This should help fix things.
#801

Copy link

@jeremyd2019 jeremyd2019 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there other places that know HOSTTYPE of cygwin.* that need to also check for msys.*?

Comment on lines +1793 to +1796
# Fix for msys2 because /bin is the same thing as /usr/bin.
if test /bin -ef /usr/bin; then
DEFPATH="/usr/bin:$DEFPATH"
fi

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned this may be the wrong thing, ie for Cygwin, which does the opposite as MSYS2 (it bind mounts /usr/bin to point to /bin). Could we check HOSTTYPE at this point in the script?

Suggested change
# Fix for msys2 because /bin is the same thing as /usr/bin.
if test /bin -ef /usr/bin; then
DEFPATH="/usr/bin:$DEFPATH"
fi
# Fix for msys2 because /bin is the same thing as /usr/bin.
case $HOSTTYPE in
msys.*)
if test /bin -ef /usr/bin; then
DEFPATH="/usr/bin:$DEFPATH"
fi
esac

@xeiavicaflashrepository
Copy link
Author

I want to add, that the files src/lib/libast/path/pathposix.c and src/lib/libast/path/pathnative.c use a function that has been deprecated and removed a long time ago. GNU ldd reports back a linking error. I fixed by commenting it out which I'll remove in my PR.

@jeremyd2019
Copy link

If you want to convert paths, you would use cygwin_conv_path

…fact it only pulled in sys/cygwin.h IF it detected as cygwin. Added an || statement to let it check if it's __MSYS__ as well. More will probable have to be changed.
@xeiavicaflashrepository
Copy link
Author

xeiavicaflashrepository commented Mar 14, 2025

@jeremyd2019
Get this, there was nothing wrong with the compiler script nor ksh93. It was windows itself that was the problem (What else is new). There is a for loop that starts with for i in ksh and what this would do is set the default shell to ksh. Windows didn't like that and it is why it resulted in a fork bomb of sorts. I'll close my other issue since I know it's not the script to blame.

I simply moved sh to the front, and it works now. As for the two files I modified, it would definitely break something if I removed them. They were merely deprecated but not removed. That's why it worked IF we set it to cygwin, because the if macro that pulls in sys/cygwin.h is ONLY if __CYGWIN__ is detected. I added in an || statement to include __MSYS__.

@jeremyd2019
Copy link

jeremyd2019 commented Mar 14, 2025 via email

@jeremyd2019
Copy link

There is no cygwin_conv_to in Cygwin codebase anymore. I still think you want to use cygwin_conv_path, but apparently this code isn't used because it should fail to link. BTW, don't know what you meant by GNU ldd. Did you mean ld? Or did you mean Cygwin's ldd?

@jeremyd2019
Copy link

No. That code would fail on modern Cygwin too. Should be changed to use cygwin_conv_path instead.

@jeremyd2019
Copy link

jeremyd2019 commented Mar 15, 2025

I was thinking, if there's interest here, I can put together a PR adding GHA testing on Cygwin. I don't understand why this code wasn't breaking the build on Cygwin/MSYS2 already. I am considering if I need to debug the "botch" check, because it definitely comes out false on MSYS2 at least.

@@ -1007,7 +1008,7 @@ ctime_now(const char* path)

if (sysstat(path, &fs) || (fs.st_mode & S_IWUSR) || syschmod(path, (fs.st_mode | S_IWUSR) & S_IPERM))
fs.st_mode = 0;
cygwin_conv_to_win32_path(path, tmp);
cygwin_conv_path(CCP_POSIX_TO_WIN_W | CCP_ABSOLUTE, path, tmp, 0);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cygwin_conv_path(CCP_POSIX_TO_WIN_W | CCP_ABSOLUTE, path, tmp, 0);
cygwin_conv_path(CCP_POSIX_TO_WIN_A | CCP_ABSOLUTE, path, tmp, sizeof(tmp));

Similar for the other calls to cygwin_conv_path.

No idea where conv_path_list_buf_size (or conv_last_list_buf_size) come from. But if you need to convert a path list, you can use cygwin_conv_path_list

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so it looks like these old deprecated functions were 32-bit only, and removed in cygwin/cygwin@3e917da

@jeremyd2019
Copy link

This code does not seem to be enabled on (current) Cygwin because the test in src/lib/libast/features/omitted tries to use functions prefixed with _ and Cygwin does not define them. If I define them all to the non-underscore variant, the test produces:

#define _win32_botch_copy       1
#define _win32_botch_rename     1
#define _win32_botch_execve     1
#define _win32_botch    1

I added include of <stdio.h> and:

#ifdef __CYGWIN__
#define _open open
#define _fstat fstat
#define _fstat fstat
#define _read read
#define _write write
#define _close close
#define _mkdir mkdir
#define _chdir chdir
#define _access access
#define _chmod chmod
#define _getpagesize getpagesize
#define _link link
#define _pathconf pathconf
#define _rename rename
#define _stat stat
#define _truncate truncate
#define _unlink unlink
#define _utime utime
#endif

But if this code is enabled, it is almost certainly going to be really broken.

@jeremyd2019
Copy link

I think maybe it's better that those "botch" patches are not enabled, Cygwin does those behaviors for a reason

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.

2 participants