Make debug_triple depend on target json file content rather than file path #98225
Conversation
This file contains 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
… 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.
bjorn3
added
T-compiler
|
r? @nagisa (rust-highfive has picked a reviewer for you, use r? to override) |
|
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
bjorn3
commented
Jun 18, 2022
| @@ -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()?; | |||
There was a problem hiding this comment.
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.
bjorn3
force-pushed
the
stable_target_json_hash
branch
from
a76a577
to
8c296ae
Compare
This comment has been minimized.
This comment has been minimized.
nagisa
reviewed
Jun 19, 2022
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, |
There was a problem hiding this comment.
Minor style nit, consider:
Suggested change
| ( | |
| 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…):
Suggested change
| ( | |
| 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 | |
| } |
There was a problem hiding this comment.
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<D> for TargetTriple { | ||
| fn decode(d: &mut D) -> Self { | ||
| match d.read_usize() { |
There was a problem hiding this comment.
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 🤷
|
Fixed all review comments apart from #98225 (comment) |
This comment was marked as resolved.
This comment was marked as resolved.
|
consider squashing the history somewhat before |
bjorn3
force-pushed
the
stable_target_json_hash
branch
from
622015a
to
b4b536d
Compare
|
@bors r=nagisa |
|
📌 Commit b4b536d has been approved by |
bors
added
S-waiting-on-bors
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
Jun 19, 2022
…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
Jun 20, 2022
Rollup of 4 pull requests Successful merges: - rust-lang#95534 (Add `core::mem::copy` to complement `core::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 `IntoFuture` docs) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Ericson2314
added a commit
to alamgu/ledger-nanos-sdk
that referenced
this pull request
Aug 25, 2022
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
Aug 29, 2022
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.
47 tasks
rnestler
added a commit
to rnestler/archpkg-linux-rust
that referenced
this pull request
May 23, 2023
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
Aug 19, 2024
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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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)