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] fix case sensitive comparison in PATH system d… #34791

Merged
merged 2 commits into from
Nov 3, 2023

Conversation

flmmkch
Copy link
Contributor

@flmmkch flmmkch commented Oct 30, 2023

Fixes the icu build error with the following message on my system: Unable to find system dir in the PATH variable! Appending required msys paths!

Hopefully fixes #34450

@flmmkch
Copy link
Contributor Author

flmmkch commented Oct 30, 2023

@microsoft-github-policy-service agree company="Regnology"

@MonicaLiu0311 MonicaLiu0311 added the category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly label Oct 30, 2023
@MonicaLiu0311 MonicaLiu0311 added the info:reviewed Pull Request changes follow basic guidelines label Oct 31, 2023
@JavierMatosD JavierMatosD added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Oct 31, 2023
@BillyONeal
Copy link
Member

  1. It seems like this is unlikely to fix the indicated issue because it doesn't look like
    message(WARNING "Unable to find system dir in the PATH variable! Appending required msys paths!")
    got triggered there?
  2. This looks like the previous author intentionally did not do this; they clearly knew TOUPPER existed and yet chose to form the explicit list of different cases anyway. Did something change (as in, a previous PR that added the toupper stuff that was just incomplete) or similar?

@BillyONeal
Copy link
Member

RE: 2: e65af7b / #32750 (comment) This change seems consistent with what @Neumann-A indicated there so it seems OK to me.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

This change seems reasonable to me but I would really like an understanding of why this is expected to affect the linked issue given that the warning printed when the correct index is not found was not printed.

@Neumann-A
Copy link
Contributor

This looks like the previous author intentionally did not do this; they clearly knew TOUPPER existed and yet chose to form the explicit list of different cases anyway. Did something change (as in, a previous PR that added the toupper stuff that was just incomplete) or similar?

No I just missed that some kind of normalization to either tolower or toupper should be done to avoid dealing with a lot of mixed cased cases. TOUPPER was just used because upper cased paths have been observed before (e.g. SYSTEM)

@JavierMatosD JavierMatosD merged commit 9f03078 into microsoft:master Nov 3, 2023
15 checks passed
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:reviewed Pull Request changes follow basic guidelines 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] build failure
5 participants