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

Added extract_regex_match function and updated NuGetProvider #1418

Closed
wants to merge 2 commits into from

Conversation

kazssym
Copy link
Contributor

@kazssym kazssym commented Jun 9, 2024

A new function extract_regex_match has been added to the vcpkg namespace in tools.cpp. This function performs a regex search on an output string and returns the first match or a formatted error message.

The NuGetProvider struct has been updated to replace the extract_prefixed_nonwhitespace function call with the new extract_regex_match function. This change enhances the flexibility and robustness of version extraction by using a regex match instead of a fixed prefix.

Fixes microsoft/vcpkg#38940

@kazssym
Copy link
Contributor Author

kazssym commented Jun 9, 2024

I tested locally with the Japanese localization. Please confirm it is OK.

@kazssym kazssym marked this pull request as ready for review June 9, 2024 09:01
@kazssym kazssym marked this pull request as draft June 9, 2024 09:53
@kazssym kazssym force-pushed the regex-version-extraction branch 2 times, most recently from b967d9e to 461e478 Compare June 9, 2024 10:06
A new function `extract_regex_match` has been added to the `vcpkg` namespace in `tools.cpp`. This function performs a regex search on an output string and returns the first match or a formatted error message.

The `NuGetProvider` struct has been updated to replace the `extract_prefixed_nonwhitespace` function call with the new `extract_regex_match` function. This change enhances the flexibility and robustness of version extraction by using a regex match instead of a fixed prefix.
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.

The whole reason extract_prefixed_nonwhitespace exists is that we are trying to break the dependency on std::regex because all major implementations of that are awful. Only the 'fake XML parsing' thing still uses it, and that entire component's days are numbered as there is no longer any reason to have a separate vcpkgTools.xml now that the bash bootstrap script no longer looks at it.

Can you write code that implements this instead and a comment with the example non-English output with which it is expected to work?

Alternately, should we be telling NuGet we want locale-independent output for this test somehow?

@kazssym
Copy link
Contributor Author

kazssym commented Jul 6, 2024

The whole reason extract_prefixed_nonwhitespace exists is that we are trying to break the dependency on std::regex because all major implementations of that are awful. Only the 'fake XML parsing' thing still uses it, and that entire component's days are numbered as there is no longer any reason to have a separate vcpkgTools.xml now that the bash bootstrap script no longer looks at it.

Can you write code that implements this instead and a comment with the example non-English output with which it is expected to work?

Alternately, should we be telling NuGet we want locale-independent output for this test somehow?

Thank you. The issue this pull request tries to resolve is nuget.exe's output is now locale-dependent and no longer has a fixed prefix. A sample is NuGet バージョン: 6.10.0.107 with the Japanese locale.

Possible alternatives would be either

  1. Execute nuget.exe always with the English locale.
  2. Use pattern matching without regex.
  3. Use a consistent regex engine.

Which one is better?

@kazssym
Copy link
Contributor Author

kazssym commented Jul 6, 2024

I found the -ForceEnglishOutput option can be used to get fixed locale-independent outputs, but it could not be used without any subcommand as in nuget.exe -ForceEnglilshOutput.

@kazssym
Copy link
Contributor Author

kazssym commented Jul 6, 2024

Setting the NUGET_CLI_LANGUAGE environment variable seems OK. Can we set this variable when executing nuget.exe?

@myd7349
Copy link
Contributor

myd7349 commented Jul 6, 2024

It works for me:

nuget.exe help -ForceEnglishOutput

@kazssym
Copy link
Contributor Author

kazssym commented Jul 6, 2024

It works for me:

nuget.exe help -ForceEnglishOutput

Hmm, you used the help command while I didn't.

@kazssym
Copy link
Contributor Author

kazssym commented Jul 6, 2024

Rewrote this PR in #1451.

@kazssym kazssym closed this Jul 8, 2024
@BillyONeal
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

vcpkg.exe cannot determine the version of the downloaded nuget.exe.
3 participants