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

[vcpkg_configure_make] Automatically convert paths in all options that contain Windows format on Windows #19734

Closed
wants to merge 2 commits into from

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Aug 25, 2021

Since vcpkg_*_make is used in Windows to use the minGW cross-compilation environment, the path included in all options must be in unix format at this time.
Add automatic conversion code to prevent parameters in the wrong format from being passed in.

Here are some reasons why it was modified here:

  1. Using mingW bundled cygpath to convert the path is more appropriate and safer than using string(REGEX).
  2. At present, we only know that icu has this issue, and we don’t know and it is easy to ignore whether other ports and PRs also have such issues.
  3. Make sure that the path is in unix format should be something that the vcpkg tool function needs to do, not something that needs to be considered in portfile.cmake.

Related: #19726
Fixes #18086.

@JackBoosY JackBoosY added info:internal This PR or Issue was filed by the vcpkg team. category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:world-rebuild labels Aug 25, 2021
@JackBoosY JackBoosY mentioned this pull request Aug 25, 2021
@JackBoosY JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 25, 2021
@JackBoosY JackBoosY changed the title [vcpkg_configure_make] Automatically convert paths in all options tha… [vcpkg_configure_make] Automatically convert paths in all options that contain Windows format on Windows Aug 25, 2021
Comment on lines +241 to +245
execute_process(
COMMAND ${cygpath} "${matched_path}"
WORKING_DIRECTORY ${CURRENT_BUILDTREES_DIR}
OUTPUT_VARIABLE OUT_VARS
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can separate this part into a new vcpkg function in a new cmake file so that portfile.cmake can be called if needed.

@dg0yt
Copy link
Contributor

dg0yt commented Aug 25, 2021

Since vcpkg_*_make is used in Windows to use the minGW cross-compilation environment, the path included in all options must be in unix format at this time.

I don't think the situation is so clear. I see two major issues:

  • vcpkg uses msys2 which does automatic path conversions (though not perfectly) for command line paramaters and for environment variables when entering and leaving subsystems.
  • In some cases, paths will be passed to Windows tools, or written to files used by Windows tools. These tools do need Windows paths.

@JackBoosY
Copy link
Contributor Author

@dg0yt Yeah, if the path is only passed to the tool, yes. And we can't judge the function of the parameter inside vcpkg_configure_make.
So how about separate the convert path to a new vcpkg function and check Windows path in vcpkg_configure_make, then print warning message only?

@dg0yt
Copy link
Contributor

dg0yt commented Aug 26, 2021

So how about separate the convert path to a new vcpkg function and check Windows path in vcpkg_configure_make, then print warning message only?

I would appreciate a single function (or a set of functions) for msys path conversion instead of proliferating path conversion code over multiple consuming scripts. But when you want to use cygpath, you need a MSYS_ROOT. I still miss a good strategy on how to limit the number of roots. (We already have a second msys root for pkg-config.)

I don't like so much the idea of adding a warning message when it is unlikely to indicate an actual issue. AFAIK there is no actual issue besides icu ATM. Msys path conversion seems to do the right thing most of the time.

@dg0yt
Copy link
Contributor

dg0yt commented Aug 26, 2021

But when you want to use cygpath, you need a MSYS_ROOT.

And of course you need the matching cygpath for config/build.

@JackBoosY
Copy link
Contributor Author

So we prefer to use #19726 instead of this.

@JackBoosY JackBoosY closed this Aug 31, 2021
@JackBoosY JackBoosY deleted the dev/jack/18086 branch May 26, 2022 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:internal This PR or Issue was filed by the vcpkg team. requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[icu:x64-windows-static] build failure
3 participants