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

sess: warn w/out fluent bundle w/ user sysroot #95716

Merged
merged 1 commit into from
Apr 12, 2022
Merged

sess: warn w/out fluent bundle w/ user sysroot #95716

merged 1 commit into from
Apr 12, 2022

Conversation

davidtwco
Copy link
Member

Addresses #95512 (comment).

When a custom sysroot is requested, then don't error when translation resources are not found, only warn.

r? @bjorn3

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Apr 6, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2022
@bjorn3
Copy link
Member

bjorn3 commented Apr 6, 2022

rustc_interface has this fallback logic for codegem backends: https://github.com/rust-lang/rust/blob/master/compiler/rustc_interface/src/util.rs#L272

@davidtwco davidtwco force-pushed the translation-custom-sysroot-only-warn branch from ccbd796 to a7eb311 Compare April 6, 2022 06:50
@davidtwco
Copy link
Member Author

rustc_interface has this fallback logic for codegem backends: https://github.com/rust-lang/rust/blob/master/compiler/rustc_interface/src/util.rs#L272

Changed the approach this PR takes to use sysroot_candidates.

@bjorn3
Copy link
Member

bjorn3 commented Apr 6, 2022

LGTM, but I would like someone else to review too.

r? rust-lang/compiler

@rust-highfive rust-highfive assigned oli-obk and unassigned bjorn3 Apr 6, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Apr 6, 2022

Is this testable by just pointing a ui test to an empty directory or will that fail earlier due to other things?

@light4 light4 mentioned this pull request Apr 11, 2022
Closed
@davidtwco
sess: try sysroot candidates for fluent bundle
fc3cca2 
Instead of checking only the user provided sysroot or the default (when
no sysroot is provided), search user provided sysroot and then check
default sysroots for locale requested by the user.

Signed-off-by: David Wood <david.wood@huawei.com>
@davidtwco davidtwco force-pushed the translation-custom-sysroot-only-warn branch from a7eb311 to fc3cca2 Compare April 12, 2022 09:20
@davidtwco
Copy link
Member Author

Is this testable by just pointing a ui test to an empty directory or will that fail earlier due to other things?

I've added some more tests that I think cover most of the cases modified in this PR, more testing will be required when we actually have other translations and this mechanism is useful though.

I don't think pointing the sysroot at an empty directory is too helpful because it'll still check the default sysroot (which may contain a locale?) after that, so I've just tested that by checking for Klingon translations, assuming that we won't have that (knowing full well that someone might go away and do that translation just because this test depends on it not existing and I'm pretty okay with that).

I didn't use UI tests because we don't have a way to refer to get the path of a non-Rust auxiliary file like a custom Fluent resource, we've never needed that before.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 12, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Apr 12, 2022

📌 Commit fc3cca2 has been approved by oli-obk

@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 Apr 12, 2022
@fee1-dead fee1-dead mentioned this pull request Apr 12, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 12, 2022
@bors
Auto merge of rust-lang#95974 - fee1-dead:rollup-2fr55cs, r=fee1-dead
327caac 
Rollup of 5 pull requests

Successful merges:

 - rust-lang#95671 (feat: Allow usage of sudo [while not accessing root] in x.py)
 - rust-lang#95716 (sess: warn w/out fluent bundle w/ user sysroot)
 - rust-lang#95820 (simplify const params diagnostic on stable)
 - rust-lang#95900 (Fix documentation for wasm32-unknown-unknown)
 - rust-lang#95947 (`impl const Default for Box<[T]>` and `Box<str>`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 305763e into rust-lang:master Apr 12, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 12, 2022
@davidtwco davidtwco deleted the translation-custom-sysroot-only-warn branch April 13, 2022 01:24
@compiler-errors compiler-errors added the A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic label Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.62.0
Development

Successfully merging this pull request may close these issues.
7 participants
@davidtwco @bjorn3 @oli-obk @bors @compiler-errors @rust-highfive @rustbot