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.

Sign up for GitHub

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

Jump to bottom

Don't require the output from libtest to be valid UTF-8 #112277

Merged
merged 1 commit into from
Jun 4, 2023
Merged

Don't require the output from libtest to be valid UTF-8 #112277

merged 1 commit into from
Jun 4, 2023

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Jun 4, 2023

On Windows this is sometimes not the case, for reasons I can't track down (maybe related to localization? the bug report had a french locale).

This works around the problem, although I'm not sure how to confirm we're not generating invalid build metrics in this case.

Fixes #112273.

@rustbot
Copy link
Collaborator

rustbot commented Jun 4, 2023

r? @ozkanonur

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Jun 4, 2023
@jyn514
Don't require the output from libtest to be valid UTF-8
f9a81e4 
On Windows this is sometimes not the case, for reasons I can't track down.

This works around the problem, although I'm not sure how to confirm we're not generating invalid
build metrics in this case.
onur-ozkan
onur-ozkan approved these changes Jun 4, 2023
View reviewed changes
Copy link
Member

@onur-ozkan onur-ozkan left a comment

Choose a reason for hiding this comment

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

Solid fix! The change looks almost the same, except that it solves the problem. Looks good to me:)

@onur-ozkan
Copy link
Member

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 4, 2023

📌 Commit f9a81e4 has been approved by ozkanonur

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 4, 2023
@onur-ozkan
Copy link
Member

This works around the problem, although I'm not sure how to confirm we're not generating invalid build metrics in this case.

Feel free to r-/r+ if you want to leave a comment/fixme on the change

@jyn514
Copy link
Member Author

jyn514 commented Jun 4, 2023

mmm i'm not sure what i'd change - if @pietroalbini has thoughts i can change the behavior here, but otherwise i think it's ok as-is. non-utf8 output should be extremely rare anyway.

@the8472
Copy link
Member

the8472 commented Jun 4, 2023

This works around the problem

Does it though? Printing non-utf8 to a windows console will just error a bit later. https://doc.rust-lang.org/std/io/fn.stdout.html

When operating in a console, the Windows implementation of this stream does not support non-UTF-8 byte sequences. Attempting to write bytes that are not valid UTF-8 will return an error.

@jyn514
Copy link
Member Author

jyn514 commented Jun 4, 2023

@the8472 #112273 (comment) says this fixed the problem 🤷

@matthiaskrgr matthiaskrgr mentioned this pull request Jun 4, 2023
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 4, 2023
@bors
Auto merge of rust-lang#112282 - matthiaskrgr:rollup-1g9w02p, r=matth…
101fa90 
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#112247 (rust-lld: add rpath entry to the correct `lib` folder)
 - rust-lang#112274 (Migrate GUI colors test to original CSS color format)
 - rust-lang#112277 (Don't require the output from libtest to be valid UTF-8)

Failed merges:

 - rust-lang#112251 (rustdoc: convert `if let Some()` that always matches to variable)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0716ac9 into rust-lang:master Jun 4, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@onur-ozkan onur-ozkan onur-ozkan approved these changes

Assignees

@onur-ozkan onur-ozkan

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap)
Projects
None yet
Milestone
1.72.0
Development

Successfully merging this pull request may close these issues.

Error when testing with x.py
5 participants
@jyn514 @rustbot @onur-ozkan @bors @the8472