Make debug_triple depend on target json file content rather than file path by bjorn3 · Pull Request #98225 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation17 Commits2 Checks0 Files changed
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 }})
This ensures that changes to target json files will force a recompilation. And more importantly that moving the files doesn't force a recompilation.
This should fix Rust-for-Linux/linux#792 (cc @ojeda)
… path
This ensures that changes to target json files will force a recompilation. And more importantly that moving the files doesn't force a recompilation.
Relevant to the compiler team, which will review and decide on the PR/issue.
Area: Compile-target specifications
labels
r? @nagisa
(rust-highfive has picked a reviewer for you, use r? to override)
This comment has been minimized.
| @@ -2436,20 +2434,28 @@ impl TargetTriple { |
|---|
| /// Creates a target triple from the passed target path. |
| pub fn from_path(path: &Path) -> Result<Self, io::Error> { |
| let canonicalized_path = path.canonicalize()?; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be fine to remove this canonicalize operation now I think. The only difference would be that the triple would be the original file stem rather than the canonicalized one. This may be preferred in case of content addressed storage as the canonicalized name would be a non human readable hash.
This comment has been minimized.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on the general direction. r=me, up to you what to do with the nits inline.
Comment on lines +2441 to +2444
| ( |
|---|
| Self::TargetJson { path_for_rustdoc: _, triple: l_triple, contents: l_contents }, |
| Self::TargetJson { path_for_rustdoc: _, triple: r_triple, contents: r_contents }, |
| ) => l_triple == r_triple && l_contents == r_contents, |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor style nit, consider:
| ( |
|---|
| Self::TargetJson { path_for_rustdoc: _, triple: l_triple, contents: l_contents }, |
| Self::TargetJson { path_for_rustdoc: _, triple: r_triple, contents: r_contents }, |
| ) => l_triple == r_triple && l_contents == r_contents, |
| (l@Self::TargetJson { .. }, r@Self::TargetJson { .. }) => { |
| l.triple == r.triple && l.contents == r.contents |
| } |
or, if the intent was to have this an exhaustive match, maybe something like this (though there isn’t much value in this…):
| ( |
|---|
| Self::TargetJson { path_for_rustdoc: _, triple: l_triple, contents: l_contents }, |
| Self::TargetJson { path_for_rustdoc: _, triple: r_triple, contents: r_contents }, |
| ) => l_triple == r_triple && l_contents == r_contents, |
| (Self::TargetJson { path_for_rustdoc: _, triple, contents }, r@Self::TargetJson { .. }) => { |
| triple == r.triple && contents == r.contents |
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or, if the intent was to have this an exhaustive match
Indeed
maybe something like this
Rustfmt puts that suggestion on four lines too.
| impl<D: Decoder> Decodable for TargetTriple { |
| fn decode(d: &mut D) -> Self { |
| match d.read_usize() { |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little sketchy. This could fail if the implementation of the encoder has overriden emit_enum_variant to output something else than usize… Though of course Decoder does not provide a better way 🤷
This comment was marked as resolved.
consider squashing the history somewhat before bors r=nagisaing this ^^
📌 Commit b4b536d has been approved by nagisa
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
JohnTitor added a commit to JohnTitor/rust that referenced this pull request
…agisa
Make debug_triple depend on target json file content rather than file path
This ensures that changes to target json files will force a recompilation. And more importantly that moving the files doesn't force a recompilation.
This should fix Rust-for-Linux/linux#792 (cc @ojeda)
bors added a commit to rust-lang-ci/rust that referenced this pull request
Rollup of 4 pull requests
Successful merges:
- rust-lang#95534 (Add
core::mem::copyto complementcore::mem::drop.) - rust-lang#97912 (Stabilize
Path::try_exists()and improve doc) - rust-lang#98225 (Make debug_triple depend on target json file content rather than file path)
- rust-lang#98257 (Fix typos in
IntoFuturedocs)
Failed merges:
r? @ghost
@rustbot modify labels: rollup
Ericson2314 added a commit to alamgu/ledger-nanos-sdk that referenced this pull request
The first change is to parse once, and then use an enum for the device. This is hopefully a straightforward improvement.
The second change is to case on the OS name (leveraging LedgerHQ#38) instead of the target name. The target "name" isn't so structured, and it is unclear to what extent it should be anm exposed part of the target. (See rust-lang/rust#98225 for example, where the contents rather than json file path were used as a a key.)
With LedgerHQ#38 the device name is used for the OS field instead, and so we are robust to confusing behavior around names.
yhql pushed a commit to LedgerHQ/ledger-device-rust-sdk that referenced this pull request
The first change is to parse once, and then use an enum for the device. This is hopefully a straightforward improvement.
The second change is to case on the OS name (leveraging #38) instead of the target name. The target "name" isn't so structured, and it is unclear to what extent it should be anm exposed part of the target. (See rust-lang/rust#98225 for example, where the contents rather than json file path were used as a a key.)
With #38 the device name is used for the OS field instead, and so we are robust to confusing behavior around names.
56 tasks
rnestler added a commit to rnestler/archpkg-linux-rust that referenced this pull request
This gives us some warnings, but allows to compile out-of-tree modules since it fixes a bug with how the target triple is defined for a custom target.json file. See rust-lang/rust#98225
superstar0402 added a commit to superstar0402/wea that referenced this pull request
The first change is to parse once, and then use an enum for the device. This is hopefully a straightforward improvement.
The second change is to case on the OS name (leveraging #38) instead of the target name. The target "name" isn't so structured, and it is unclear to what extent it should be anm exposed part of the target. (See rust-lang/rust#98225 for example, where the contents rather than json file path were used as a a key.)
With #38 the device name is used for the OS field instead, and so we are robust to confusing behavior around names.
Labels
Area: Compile-target specifications
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Relevant to the compiler team, which will review and decide on the PR/issue.