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

Support non-UTF-8 in environment variable dependency tracking #133699

Open
madsmtm opened this issue Dec 1, 2024 · 3 comments
Open

Support non-UTF-8 in environment variable dependency tracking #133699

madsmtm opened this issue Dec 1, 2024 · 3 comments
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@madsmtm
Copy link
Contributor

madsmtm commented Dec 1, 2024

ParseSess::env_depinfo stores Symbol instead of OsStr.

While we're at it, might make sense to actually not store the environment variable value at all, and instead fetch it inside write_out_deps? (which assumes that the variable won't change during execution of the compiler, but it probably won't).

See also #130883 (comment).

@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Dec 1, 2024
@bjorn3
Copy link
Member

bjorn3 commented Dec 1, 2024

Cargo expects the dep-info file to be valid UTF-8 and parse_rustc_dep_info returns an error when it isn't. As the env var value is included in the dep-info file, this implies that the env var value itself has to be UTF-8 too.

@petrochenkov
Copy link
Contributor

In the original PR introducing the env depinfo we discussed support for non UTF-8, and the outcome was to postpone it until there's a sufficient motivation, it can always be done with some new directive, # env-dep-escaped or something (#71858 (comment)).

#130883 (comment) merely suggests moving the recovery point for non-UTF-8 env vars to fn write_out_deps.

@lolbinarycat lolbinarycat added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 1, 2024
@jieyouxu jieyouxu added C-discussion Category: Discussion or questions that doesn't represent real issues. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Dec 2, 2024
@madsmtm
Copy link
Contributor Author

madsmtm commented Dec 3, 2024

#130883 (comment) merely suggests moving the recovery point for non-UTF-8 env vars to fn write_out_deps.

Sorry, I misunderstood, then. Still, I think keeping it separate from that PR is valuable

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-discussion Category: Discussion or questions that doesn't represent real issues. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants