Fixup Windows verbatim paths when used with the include! macro by ChrisDenton · Pull Request #125205 · rust-lang/rust (original) (raw)

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

ChrisDenton

On Windows, the following code can fail if the OUT_DIR environment variable is a verbatim path (i.e. begins with \\?\):

include!(concat!(env!("OUT_DIR"), "/src/repro.rs"));

This is because verbatim paths treat / literally, as if it were just another character in the file name.

The good news is that the standard library already has code to fix this. We can simply use components to normalize the path so it works as intended.

@rustbot

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

May 17, 2024

@rustbot

Some changes occurred in run-make tests.

cc @jieyouxu

@ChrisDenton ChrisDenton added the T-lang

Relevant to the language team, which will review and decide on the PR/issue.

label

May 17, 2024

@rust-log-analyzer

This comment has been minimized.

@ChrisDenton

While I'd very much welcome a review from jieyouxu, this will need lang approval. I think it could just be considered a bug fix but it might also be considered a change in language.

cc @joshtriplett maybe?

@Noratrieb

Given that I see most projects already using / as the separator, calling it a bug fix seems fair. Still, I agree that lang should see it.

@jieyouxu

Changes look reasonable from compiler side. Rolling a lang reviewer to decide if this behavior change is acceptable.

r? lang

@ChrisDenton ChrisDenton added S-waiting-on-team

Status: Awaiting decision from the relevant subteam (see the T- label).

and removed T-compiler

Relevant to the compiler team, which will review and decide on the PR/issue.

labels

May 22, 2024

@bors

@rust-log-analyzer

This comment has been minimized.

@ChrisDenton

When using concat! to join paths, the Unix path separator (/) is often used. This breaks on Windows if the base path is a verbatim path (i.e. starts with \\?\).

@joshtriplett

It looks like this is limited to verbatim paths, which seems completely safe. This wouldn't be safe to do for arbitrary Windows driver-interpreted paths, but it seems entirely safe for Windows verbatim paths.

@ChrisDenton

Thanks! So I'm going to go out on a limb and say jieyouxu's positive review still stands.

@bors r=jieyouxu

@bors

📌 Commit edc97a0 has been approved by jieyouxu

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.

S-waiting-on-team

Status: Awaiting decision from the relevant subteam (see the T- label).

labels

Sep 25, 2024

@nikomatsakis

1 similar comment

@tmandry

@scottmcm

I mean you can't use concat in path, right?

This isn't concat-specific though, because it'll change if you put a string literal in the include too.

I just think that we should do it for all compiler paths if we're doing it here.

@rfcbot

🔔 This is now entering its final comment period, as per the review above. 🔔

@traviscross

@rustbot labels -I-lang-nominated

We discussed this in the meeting today... and it's now in FCP.

@rfcbot

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@ChrisDenton

I mean you can't use concat in path, right?

This isn't concat-specific though, because it'll change if you put a string literal in the include too.

I just think that we should do it for all compiler paths if we're doing it here.

Oh, it turns out we do something kind of similar for path already:

// On windows, the base path might have the form
// `\\?\foo\bar` in which case it does not tolerate
// mixed `/` and `\` separators, so canonicalize
// `/` to `\`.
#[cfg(windows)]
let path_str = path_str.replace("/", "\\");

@ChrisDenton

This has passed lang FCP and was earlier approved by @jieyouxu so I'll merge this now. I'll look more at #[path] but it already looks like it does the equivalent by virtue of using Path::join (which handles verbatim paths).

Some(dir_path.join(path_str))

@bors r=jieyouxu

@bors

📌 Commit edc97a0 has been approved by jieyouxu

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-author

Status: This is awaiting some action (such as code changes or more information) from the author.

labels

Oct 22, 2024

bors added a commit to rust-lang-ci/rust that referenced this pull request

Oct 22, 2024

@bors

…iaskrgr

Rollup of 8 pull requests

Successful merges:

r? @ghost @rustbot modify labels: rollup

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request

Oct 22, 2024

@rust-timer

Rollup merge of rust-lang#125205 - ChrisDenton:verbatim-include, r=jieyouxu

Fixup Windows verbatim paths when used with the include! macro

On Windows, the following code can fail if the OUT_DIR environment variable is a verbatim path (i.e. begins with \\?\):

include!(concat!(env!("OUT_DIR"), "/src/repro.rs"));

This is because verbatim paths treat / literally, as if it were just another character in the file name.

The good news is that the standard library already has code to fix this. We can simply use components to normalize the path so it works as intended.

tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request

Jan 22, 2025

@tmeijn

This MR contains the following updates:

Package Update Change
rust minor 1.83.0 -> 1.84.0

MR created with the help of el-capitano/tools/renovate-bot.

Proposed changes to behavior should be submitted there as MRs.

Release Notes

rust-lang/rust (rust)

v1.84.0

Compare Source

==========================

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts

Cargo

Rustdoc

Compatibility Notes

Configuration

📅 Schedule: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 Automerge: Disabled by config. Please merge this manually once you are satisfied.

♻ Rebasing: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 Ignore: Close this MR and you won't be reminded about this update again.

This MR has been generated by Renovate Bot.

wip-sync pushed a commit to NetBSD/pkgsrc-wip that referenced this pull request

Feb 2, 2025

@he32

Pkgsrc changes:

Upstream changes:

Version 1.84.1 (2025-01-30)

Version 1.84.0 (2025-01-09)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts

Cargo

Rustdoc

Compatibility Notes

This was referenced

Feb 17, 2025

Labels

A-run-make

Area: port run-make Makefiles to rmake.rs

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

finished-final-comment-period

The final comment period is finished for this PR / Issue.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-lang

Relevant to the language team, which will review and decide on the PR/issue.