-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
fix external_project failing due to the drive letter in the path on Windows #13916
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,7 +43,9 @@ | |
| "msvc", | ||
| "gcc", | ||
| "cygwin", | ||
| "!cygwin" | ||
| "!cygwin", | ||
| "windows", | ||
| "!windows" | ||
| ] | ||
| }, | ||
| "version": { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| ## The external_project module uses the cygpath command to convert paths | ||
|
|
||
| In previous versions, the external_project module on Windows used a Windows-style path (e.g., `C:/path/to/configure`) to execute the configure file, and a relative path from the drive root (e.g., `/path/to/prefix`) as the installation prefix. | ||
| However, since configure scripts are typically intended to be run in a POSIX-like environment (MSYS2, Cygwin, or GitBash), these paths were incompatible with some configure scripts. | ||
|
|
||
| The external_project module now uses the `cygpath` command to convert the configure command path and prefix to Unix-style paths (e.g., `/c/path/to/configure` for MSYS2 and `/cygdrive/c/path/to/configure` for Cygwin). | ||
| If the `cygpath` command is not found in the PATH, it will fall back to the previous behavior. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| from pathlib import Path | ||
| import os | ||
| import shlex | ||
| import shutil | ||
| import subprocess | ||
| import typing as T | ||
|
|
||
|
|
@@ -19,7 +20,7 @@ | |
| from ..interpreter.type_checking import ENV_KW, DEPENDS_KW | ||
| from ..interpreterbase.decorators import ContainerTypeInfo, KwargInfo, typed_kwargs, typed_pos_args | ||
| from ..mesonlib import (EnvironmentException, MesonException, Popen_safe, MachineChoice, | ||
| get_variable_regex, do_replacement, join_args, relpath) | ||
| get_variable_regex, do_replacement, join_args) | ||
| from ..options import OptionKey | ||
|
|
||
| if T.TYPE_CHECKING: | ||
|
|
@@ -89,19 +90,29 @@ def __init__(self, | |
| self.includedir = Path(_i) | ||
| self.name = self.src_dir.name | ||
|
|
||
| # On Windows if the prefix is "c:/foo" and DESTDIR is "c:/bar", `make` | ||
| # will install files into "c:/bar/c:/foo" which is an invalid path. | ||
| # Work around that issue by removing the drive from prefix. | ||
| if self.prefix.drive: | ||
| self.prefix = Path(relpath(self.prefix, self.prefix.drive)) | ||
| self.prefix = self._cygpath_convert(self.prefix) | ||
|
|
||
| # self.prefix is an absolute path, so we cannot append it to another path. | ||
| self.rel_prefix = Path(relpath(self.prefix, self.prefix.root)) | ||
| # On Windows (where cygpath is not applied), | ||
| # if the prefix is "c:/foo" and DESTDIR is "c:/bar", | ||
| # `make` will install files into "c:/bar/c:/foo" which is an invalid path. | ||
| # This also removes the drive letter from the prefix to workaround the issue. | ||
| self.rel_prefix = self.prefix.relative_to(self.prefix.anchor) | ||
|
|
||
| self._configure(state) | ||
|
|
||
| self.targets = self._create_targets(extra_depends) | ||
|
|
||
| def _cygpath_convert(self, winpath: Path) -> Path: | ||
| # On Cygwin, MSYS2 and GitBash, the configure command and the prefix | ||
| # should be converted to unix style path like "/c/foo" by cygpath command, | ||
| # because the colon in the drive letter breaks many configure scripts. | ||
| # Do nothing on other environment where cygpath is not available. | ||
| if winpath.drive and shutil.which('cygpath'): | ||
| _p, o, _e = Popen_safe(['cygpath', '-u', winpath.as_posix()]) | ||
| return Path(o.strip('\n')) | ||
| return winpath | ||
|
|
||
| def _configure(self, state: 'ModuleState') -> None: | ||
| if self.configure_command == 'waf': | ||
| FeatureNew('Waf external project', '0.60.0').use(self.subproject, state.current_node) | ||
|
|
@@ -116,6 +127,8 @@ def _configure(self, state: 'ModuleState') -> None: | |
| configure_path = Path(self.src_dir, self.configure_command) | ||
| configure_prog = state.find_program(configure_path.as_posix()) | ||
| configure_cmd = configure_prog.get_command() | ||
| if len(configure_cmd) >= 2 and configure_cmd[-1] == configure_path.as_posix(): | ||
| configure_cmd = configure_cmd[:-1] + [self._cygpath_convert(configure_path).as_posix()] | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is the main point of this PR. see #13699. |
||
| workdir = self.build_dir | ||
| self.make = state.find_program('make').get_command() | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,23 @@ | |
|
|
||
| srcdir=$(dirname "$0") | ||
|
|
||
| # some configure scripts seem to iterate over srcdir and other paths | ||
| # with for-loop using path_separator (most cases colon.) | ||
| PATH_SEPARATOR=: | ||
| (PATH='/bin;/bin'; FPATH=$PATH; sh -c :) >/dev/null 2>&1 && { | ||
| (PATH='/bin:/bin'; FPATH=$PATH; sh -c :) >/dev/null 2>&1 || | ||
| PATH_SEPARATOR=';' | ||
| } | ||
| IFS=$PATH_SEPARATOR | ||
| for i in $srcdir | ||
| do | ||
| if [ "$i" != "$srcdir" ] | ||
| then | ||
| echo "failed to extract $srcdir using path separator $PATH_SEPARATOR" | ||
| exit 1 | ||
| fi | ||
| done | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand what this is doing.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a test for the fix related to the comment above. |
||
|
|
||
| for i in "$@" | ||
| do | ||
| case $i in | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case cygpath is not found we don't remove the drive from
self.prefixanymore. That means we now runconfigure --prefix=c:/fooinstead ofconfigure --prefix=/foo. Is that intentional?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if you also need to convert
self.install_diras that's used for DESTDIR?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know which is appropriate in that case, but the latter has the problem of not working with another drive. If anyone has an environment with
shbut nocygpath, they may submit an issue or PR.Unlike the prefix used by
configure,DESTDIRis used bymake, right? So far I haven't had any issues with the drive letter forinstall_dir.