Make target-spec json file extensions case-insensitive

[boozook]

Currently user's target json file can have only json ext, but cannot have `JSON' or any other strange form.

This is a little fix with added support of case-insensitive file-ext for user specified target only.
That means that not supported in following cases:

• any directory traversal, such as in $RUST_TARGET_PATH
• same for rustc_target::spec::Target::search, so it doesn't work with JSON on case-sensitive FS, but works fine on case-insensitive FS.
• $RUST_TARGET_PATH dir traversal in the bootstrap is not touched too.

Performance cost:

Almost without changes.
Comparison using eq_ignore_ascii_case for only latest ext_len chars of the OsStr (as bytes).

Testing

How to test:

1. rustc --print target-spec-json -Zunstable-options > ./some-target.JSON
2. remove is-builtin field from the some-target.JSON or set to false
3. rustc --print target-spec-json -Zunstable-options --target=some-target.JSON should work
4. rustc --print target-spec-json -Zunstable-options --target=some-target should work on case-insensitive FS
5. rustc --print cfg --target=some-target should work too, on case-insensitive FS

Tests added.

Implementation note

I've separated the comparison into a method for potential future use instead of just str.ends_with(".json") and to maintain compatibility.
The method is public for the same reasons.

Fixes #127387 issue.

try-job: test-various
try-job: armhf-gnu
try-job: x86_64-msvc
try-job: aarch64-apple

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @wesleywiser (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[rustbot]

These commits modify compiler targets.
(See the Target Tier Policy.)

[tgross35]

Thanks for the fix, and welcome!

because I suppose no one but me will cry if it ever breaks, and this is minor change and doesn't changes existing behaviour, so existing tests with .json are passed. Should I add tests?

If you're up for it it's never a bad thing to have a regression test, even if it means you'll be the only one no longer crying :) You can just translate your test instructions into a rmake test (mirror a test that uses the newer rmake.rs, the Makefile tests are going away).

[rustbot]

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

[jieyouxu]

@bors try

[jieyouxu]

@bors try

[bors]

⌛ Trying commit e038876 with merge a1901f2...

[boozook]

Should I push just one commit instead of these few?

[bors]

⌛ Trying commit 4ab215b with merge dbc8e42...

[jieyouxu]

Should I push just one commit instead of these few?

When being reviewed, it's perfectly fine to have intermediate commits, but if you think it helps review or otherwise makes your commits more logical/cleaner, feel free to group logical changes into individual commits (whatever makes more sense) and rebase/force-push if you need to. If you do rebase/force-push, it can help to either remark on the addressed review comments or post a short description on what's different since last rebase/force push or last review if the difference is significant. A reviewer may ask you to reorganize the commit history before merge to tidy it up.

[bors]

⌛ Trying commit 4ab215b with merge d27bf40...

[rustbot]

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

• c6761ee

[boozook]

So, what's next steps? Should I close this PR or wait some?

[tgross35]

If you see support for case insensitivity going beyond a one-off fix here (which could get by with a compiler lead's (Wesley's) approval), it is probably good to just file a Major Change Proposal (MCP) to propose a specific policy change. This is an issue template at the compiler team repo. That will give you a chance to explain what use case we are currently not supporting well, which specific changes will help improve it, and collect input about it.

Please be patient; nobody is saying that this change is not desired or that you should close this PR. Its scope just needs to be better understood if it's not only target spec files that are causing problems. (Which the MCP should help clarify).

[boozook]

@jieyouxu,

I don't even think we have any CI runners that have a case insensitive filesystem, so I think the test wouldn't be exercised in the environment that the change is supposed to affect?

Let me explain it. This patch touches only that part where validation of user- or cargo- input via cli-arg or env, but not any fs exercises like dir-traversal. So before this patch I can't do rustc --target=FILE.JSON, but with the path I can. Nothing more. For example if I have in the pwd file FILE.JSON on the case-sensitive fs and try rustc --target=FILE I'll get an error because rustc will try to resole it as FILE.json. But that will be ok without error on the case-INsensitive fs.

Well, there is test for user-input only.

[jieyouxu]

So, what's next steps? Should I close this PR or wait some?

I would recommend waiting a bit to allow Wesley to provide you with better advice on which further steps to take.

[davidtwco]

r? @davidtwco

Apologies that this got lost in the cracks.

I think it's reasonable if niche that we would want to support accepting arbitrary filenames here (with whatever case or extension is provided, as long as we can find the file and parse it). It makes sense for our behaviour here to be consistent, so I would like to make sure that this is feasible anywhere else that we accept filenames from users, and I want to check if @rust-lang/cargo have any concerns with that other than the consistency between cargo/rustc?

If they don't, then we can file a MCP to agree on our policy going forward and then see this merged.

[epage]

I want to check if @rust-lang/cargo have any concerns with that other than the consistency between cargo/rustc?

My primary concern was us weighing whether consistency is important.

Would this change to rustc be prescriptive for cargo? Whether Cargo should be consistent within itself is another question that would need to be considered and if there is an expectation for Cargo to match then we'd need to block on that decision.

Out of curiosity, I just tried to mod HELLO; a HELLO.RS and that was not supported. So it seems like moving forward with this would be inconsistent within rustc. I recognize that is on you all to decide.

[weihanglo]

My primary concern was us weighing whether consistency is important.

Same here. Not only cargo and rustc but toolchain-wise including rustup, clippy and rustfmt. So I would say a policy where we accept where not

[ChrisDenton]

Thinking about this, I do wonder why rustc needs to care about the file extension at all.

Should we not just have a --target-spec option, then the file name (including the extension) can be anything the user wants it to be.

[epage]

The main reason I see for an extension check is to allow the format to be inferred if a new one is added in the future.

Us validating --manifest-path allowed cargo-script to be added and the early cargo team didn't predict that feature.

Another way to frame this is that the current behavior is consistent and we can always change it in the future but it a one-way door that can also constrain future decisions.

[epage]

Also, its easy to support case insensitive files when passed in but if a tool wants to support detecting the file, it requires scanning the whole directory.

[ChrisDenton]

Hm, I think this is a case where I would draw a distinction between rustc and Cargo. I think rustc almost never cares about any part of the file name, even the .rs is merely a strong convention. Whereas Cargo is "opinionated" (in the good sense), e.g. Cargo.toml has to be spelled exactly like that.

If we want to carve out space for future formats we could have --target:json=path/to/json or something like that.

[davidtwco]

My primary concern was us weighing whether consistency is important.

Would this change to rustc be prescriptive for cargo? Whether Cargo should be consistent within itself is another question that would need to be considered and if there is an expectation for Cargo to match then we'd need to block on that decision.

That's a good point, they wouldn't necessarily need to be consistent. I think I'd be okay if they weren't. If we're okay with that then it's just a matter of whether t-compiler have an opinion here.

[boozook]

Cargo PR as second part of this story, waits this PR.

---

This branch looks stale. Should I update or rebase it?

[jieyouxu]

This branch looks stale. Should I update or rebase it?

Probably wait until T-compiler discusses this change in today's triage meeting, the conflict doesn't block the discussions. If the members attending triage meeting don't have immediate concerns, we can proceed by filing an MCP (you can draft one if you prefer, alternatively I can draft one on your behalf).

[apiraino]

For more context, #127387 (and this pr) were previously discussed in triage meetings on Zulip here and here.

[apiraino]

Discussed during T-compiler triage on Zulip.

• To the concern of inconsistencies between rustc and other tooling, it seems we already have some of them (comment), though having rustc and cargo diverge on this is something we need to consider
• we are not sure if the question about the actual usecase was fully replied, more context would be helpful
• Targets like Cortex A were mentioned in comment. Can we have some more details about which exactly (this is our list of supported compile targets)

In short, in general we would be in favor of this change, and we would also be happy with an MCP documenting and detailing a bit more the intended scope of this change.

(please feel free to add more relevant questions from the meeting)

@rustbot label -i-compiler-nominated

[epage]

My primary concern was us weighing whether consistency is important.
Would this change to rustc be prescriptive for cargo? Whether Cargo should be consistent within itself is another question that would need to be considered and if there is an expectation for Cargo to match then we'd need to block on that decision.

That's a good point, they wouldn't necessarily need to be consistent. I think I'd be okay if they weren't. If we're okay with that then it's just a matter of whether t-compiler have an opinion here.

@davidtwco I wanted to check in as my point could have been missed and yours is ambiguous enough. I'm wondering if merging this PR is prescriptive to the Cargo team for merging rust-lang/cargo#14197.

• Should rustc and cargo agree on case-(in)sensitivity for target spec files?
• If yes, there is the question of self-consistency within Cargo
• If no, how much of a use case will be left for rustc to support this if Cargo chooses not to support this?

[davidtwco]

My primary concern was us weighing whether consistency is important.
Would this change to rustc be prescriptive for cargo? Whether Cargo should be consistent within itself is another question that would need to be considered and if there is an expectation for Cargo to match then we'd need to block on that decision.

That's a good point, they wouldn't necessarily need to be consistent. I think I'd be okay if they weren't. If we're okay with that then it's just a matter of whether t-compiler have an opinion here.

@davidtwco I wanted to check in as my point could have been missed and yours is ambiguous enough. I'm wondering if merging this PR is prescriptive to the Cargo team for merging rust-lang/cargo#14197.

• Should rustc and cargo agree on case-(in)sensitivity for target spec files?
• If yes, there is the question of self-consistency within Cargo
• If no, how much of a use case will be left for rustc to support this if Cargo chooses not to support this?

I think it's okay for rustc to accept more than Cargo does. I'm entirely happen for Cargo to be more opinionated than rustc in circumstances like this.

Discussed during T-compiler triage on Zulip.

• To the concern of inconsistencies between rustc and other tooling, it seems we already have some of them (comment), though having rustc and cargo diverge on this is something we need to consider
• we are not sure if the question about the actual usecase was fully replied, more context would be helpful
• Targets like Cortex A were mentioned in comment. Can we have some more details about which exactly (this is our list of supported compile targets)

I'm not sure this is exactly what we agreed - we're happy to accept a change that checks for target files without the mandatory .json extension, it seems unnecessary that we mandate a specific extension, we should accept any filename users give us. If we wanted to go further than that with respect to case insensitivity, then we'd need to know more about the use cases that this addresses.

[ehuss]

it seems unnecessary that we mandate a specific extension

I'm not sure I understand this. If we start supporting TOML spec files, would the compiler use a heuristic to determine which format it is? It seems unlikely that we would just drop JSON. I'm a little confused here.

[wesleywiser]

It's a good question but I think that's a slightly different issue. The rustc book currently says:

When rustc is given an option --target=TARGET (where TARGET is any string), it uses the following logic:

1. if TARGET is the name of a built-in target, use that
2. if TARGET is a path to a file, read that file as a json target
3. otherwise, search the colon-seperated list of directories found in the RUST_TARGET_PATH environment variable from left to right for a file named TARGET.json.

These steps are tried in order, so if there are multple potentially valid interpretations for a target, whichever is found first will take priority. If none of these methods find a target, an error is thrown.

The discussion in the compiler team triage meeting concluded that if you write --target foo.JSON, that should just be accepted. Arguably, it should already be accepted because of point 2 in the described process. What we don't want is to support modifying point 3 to accept TARGET.JSON or any variation thereof.

If we decide to add TOML support (and it seems like that should have been the correct format to use anyway), point 2 will have to be modified to look at the file extension or something. Obviously, that again raises the question of the extension case but our consensus was that if you told rustc to look for foo.JSON, that should be ok since the user explicitly told us what the filename is. In the same way, a check for "is this JSON or TOML" should not be case-sensitive if the user already told us the proper filename. However, if the user leaves it up to us to infer the correct filename (by not specifying the full file path, eg point 3) then we will not go probing for variations in filename (or extension) casing.

[davidtwco]

it seems unnecessary that we mandate a specific extension

I'm not sure I understand this. If we start supporting TOML spec files, would the compiler use a heuristic to determine which format it is? It seems unlikely that we would just drop JSON. I'm a little confused here.

We could try parse it as JSON and then fall back to TOML if that fails when no extension is provided.

[alex-semenyuk]

@boozook
Thanks for your contribution
From wg-triage. Could you please take a look comments above and resolve merge conflicts