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

[testing ci] add tests for unicode tarballs, fix utf-8 on macOS CI #20060

Closed
wants to merge 3 commits into from

Conversation

strega-nil
Copy link
Contributor

see the errors in #19963

@BillyONeal
Copy link
Member

I think this belongs as an e2e test rather than here?

@strega-nil-ms
Copy link
Contributor

@BillyONeal it's testing vcpkg_extract_source_archive, so having it as a port test is fine, no?

@BillyONeal
Copy link
Member

@BillyONeal it's testing vcpkg_extract_source_archive, so having it as a port test is fine, no?

Kinda related to #19034 where I made the same comment. As far as I know this is testing scripts/cmake meaning it logically belongs in the tool repo.

@strega-nil-ms
Copy link
Contributor

@BillyONeal At that point, we should probably pull out the test_ports directory, but until then the scripts still live in vcpkg, right?

@NancyLi1013 NancyLi1013 added category:scripts-audit Part of the scripts audit effort. category:infrastructure Pertaining to the CI/Testing infrastrucutre info:internal This PR or Issue was filed by the vcpkg team. labels Sep 9, 2021
@dg0yt
Copy link
Contributor

dg0yt commented Sep 9, 2021

At that point, we should probably pull out the test_ports directory, but until then the scripts still live in vcpkg, right?

Note the table I posted in #19034 (comment). IMO key question is what triggers the test. If scripts are moved out of vcpkg, #20060 can be moved out, too. But some tests in test_ports are really meant to be triggered by changes to ports (i.e. to vcpkg repo), in order to ensure usability of the ports or selected non-default feature combinations.

@strega-nil-ms strega-nil-ms changed the title [testing] add tests for unicode tarballs [testing ci] add tests for unicode tarballs, fix utf-8 on macOS CI Sep 10, 2021
@@ -10,6 +10,8 @@ jobs:
clean: resources
timeoutInMinutes: 1440 # 1 day
variables:
- name: LC_ALL
Copy link
Member

Choose a reason for hiding this comment

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

It seems like maybe vcpkg itself should be setting this rather than hiding it in CI? It would be in keeping with other environment variable handling we do....

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know how to set this variable in general; I don't even know if it's possible. Perhaps we could do some locale parsing; however, for CI, it makes sense that we set it to en_US.UTF-8.

@BillyONeal
Copy link
Member

I think the correct fix here is to have vcpkg set the locale by testing a few common ones like:

  • C.UTF-8
  • POSIX.UTF-8
  • en_US.UTF-8

and choosing the first matching one.

Notably, this change plus #19963 will break existing macos customers, most of whom aren't passing LC_ALL like this.

@JackBoosY
Copy link
Contributor

So, should we wait for the next vcpkg release?

@BillyONeal
Copy link
Member

So, should we wait for the next vcpkg release?

It looks like the current tool should have the fix:

image

@strega-nil-ms
Copy link
Contributor

Closing in favor of the next tool

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:infrastructure Pertaining to the CI/Testing infrastrucutre category:scripts-audit Part of the scripts audit effort. info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants