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

fix external_project failing due to the drive letter in the path on Windows #13916

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

Conversation

na-trium-144
Copy link

@na-trium-144 na-trium-144 commented Nov 17, 2024

closes #13699

On Windows, the windows-style configure command path (like C:/path/to/configure) seems to break many configure scripts because they treat the colon in the drive letter as path separator.
Converting the configure command to unix-style path (like /c/path/to/configure) using cygpath command fixes the issue.
cygpath should be available on cygwin, msys2 and gitbash.

Edit: this PR also includes fix for enabling test cases/common/230 external project on msys2. (related to #13895)
To enable this, I had to add windows and !windows as platform value in test configuration.

Edit: Also includes fix for a bug where the leading / in the prefix is missing on MinGW, which appears to be introduced in #13886.

I'm new to contributing to meson. Please let me know if there are something I need to change.

@na-trium-144 na-trium-144 force-pushed the configure_path_windows branch 2 times, most recently from fdc35d9 to 20a9be7 Compare November 17, 2024 17:39
@eli-schwartz
Copy link
Member

cygpath should be available on cygwin, msys2 and gitbash.

How do we know that all scripts for configuring an external project via --prefix etc. are necessarily running from cygwin? In comparison, we have existing functions for determining if meson is being run from a cygwin or msys2 environment.

I suppose that it may be fairly rare for homebrewed build systems to exist and actual autoconf scripts won't run without some runtime environment similar to cygwin / msys2. How does a bare mingw install handle this?

@na-trium-144
Copy link
Author

na-trium-144 commented Nov 17, 2024

It does not necessarily need to be running on cygwin, as long as there is sh and cygpath. msys2 and gitbash also provides cygpath (in spite of its name).

Initially I didn't think about other environment that provide sh command without cygpath command on Windows.
However now I searched a little and found at least two: BusyBox and UnxUtils.
Although I have not tested whether these can run autoconf scripts, it seems both of these have sh and do not have cygpath, and both of these uses windows style path.

So if we need to support such environments, maybe the path conversion should be done only when cygpath exists?
Then this PR would at least fix the behavior on cygwin, msys2, and gitbash, while keeping the behavior in other environment as-is, I think.

Edit: I've updated the code for this.
(But then it may fail when there are multiple sh environment installed and pick wrong cygpath. Should I care about such edge cases?)

@na-trium-144
Copy link
Author

I've pushed some additional fixes (see edited description), and now CI passed.
Could someone review this please?
maybe @eli-schwartz ?

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

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

Some of these commits are missing explanations for why they are needed.

Using "added" for some commits and "fix" for others is inconsistent style ;) consistency would be "add / fix" or "added / fixed". Git style is to use imperative mood: https://cbea.ms/git-commit/#imperative

mesonbuild/modules/external_project.py Outdated Show resolved Hide resolved
mesonbuild/modules/external_project.py Outdated Show resolved Hide resolved
@na-trium-144

This comment was marked as resolved.

On Cygwin, MSYS2 and GitBash, the configure command should be
converted to unix style path by cygpath command,
because the colon in the drive letter breaks many configure scripts.
It is needed to run test case common/230 on windows.
This computation of prefix and rel_prefix was re-written in mesonbuild#13886
but it introduced another bug where the leading slash was missing.
In addition drive root should have trailing slash,
or it would use different path as base of relpath in some cases.
@na-trium-144 na-trium-144 marked this pull request as ready for review November 23, 2024 16:35
@na-trium-144
Copy link
Author

@eli-schwartz Thanks for review! I fixed them all, so please check it again.

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