Skip to content

[vcpkg-configure-make] Fix include path prepending#24823

Merged
BillyONeal merged 2 commits intomicrosoft:masterfrom
dg0yt:host-path-check
May 20, 2022
Merged

[vcpkg-configure-make] Fix include path prepending#24823
BillyONeal merged 2 commits intomicrosoft:masterfrom
dg0yt:host-path-check

Conversation

@dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 20, 2022

@JackBoosY JackBoosY self-assigned this May 20, 2022
@JackBoosY JackBoosY added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label May 20, 2022
Copy link
Contributor

@JackBoosY JackBoosY left a comment

Choose a reason for hiding this comment

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

LGTM if the test passed.

@Thomas1664
Copy link
Contributor

I noticed some issues with the documentation at the top. I wanted to fix them by myself but they're too small for a PR. Can you include the diff?

diff --git a/scripts/cmake/vcpkg_configure_make.cmake b/scripts/cmake/vcpkg_configure_make.cmake
index 535941379..5c9693255 100644
--- a/scripts/cmake/vcpkg_configure_make.cmake
+++ b/scripts/cmake/vcpkg_configure_make.cmake
@@ -15,6 +15,7 @@ vcpkg_configure_make(
     [CONFIG_DEPENDENT_ENVIRONMENT <SOME_VAR>...]
     [CONFIGURE_ENVIRONMENT_VARIABLES <SOME_ENVVAR>...]
     [ADD_BIN_TO_PATH]
+    [DISABLE_VERBOSE_FLAGS]
     [NO_DEBUG]
     [SKIP_CONFIGURE]
     [PROJECT_SUBPATH <${PROJ_SUBPATH}>]
@@ -58,7 +59,7 @@ Script that needs to be called before configuration (do not use for batch files
 ### ADD_BIN_TO_PATH
 Adds the appropriate Release and Debug `bin\` directories to the path during configure such that executables can run against the in-tree DLLs.
 
-## DISABLE_VERBOSE_FLAGS
+### DISABLE_VERBOSE_FLAGS
 do not pass '--disable-silent-rules --verbose' to configure
 
 ### OPTIONS

@dg0yt
Copy link
Contributor Author

dg0yt commented May 20, 2022

I guess we won't see any difference in CI because all three environment variables are initially unset.
You should complement my local icu:x64-linux (C_INCLUDE_PATH with separator :) testing with similar icu:x64-windows testing (INCLUDE with separator ;, probably needs to be allowed in triplet file for windows due to environment cleaning).

@dg0yt
Copy link
Contributor Author

dg0yt commented May 20, 2022

I wanted to fix them by myself but they're too small for a PR. Can you include the diff?

Fine for me.
I would say there is a small window to collect some changes to this particular script now. I'd also review PRs against my fork, but there is no CI before it arrives here.

Co-authored-by: Thomas1664 <46387399+Thomas1664@users.noreply.github.com>
@BillyONeal
Copy link
Member

Fine for me. I would say there is a small window to collect some changes to this particular script now. I'd also review PRs against my fork, but there is no CI before it arrives here.

This fixes a serious problem and already passed CI, so I'm just merging it for now. Feel free to submit another PR with additional changes.

@BillyONeal BillyONeal merged commit f20c7bf into microsoft:master May 20, 2022
@BillyONeal
Copy link
Member

Thanks!

@dg0yt dg0yt deleted the host-path-check branch May 23, 2022 07:01
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[icu] build failure icu 70.1, error: Host path separator (:) in path; this is unsupported.

4 participants