Use Build::getenv instead of env::var* in anywhere that makes sense
#1103
Conversation
Only call `self.getenv("RUSTC_LINKER")` if necessary
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
`RwLock` would enable readers to run in parallel. Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Use `Path` instead of `PathBuf` to avoid alloc Signed-off-by: Jiahao XU <[email protected]>
Cache everything in there in `apple_versions_cache` Signed-off-by: Jiahao XU <[email protected]>
Cache in `apple_sdk_root_cache` if possible Signed-off-by: Jiahao XU <[email protected]>
Only call `to_string_lossy()` if necessary Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Avoid one heap allocation that is de-allocated immediately
There was a problem hiding this comment.
Might make sense to ban std::env::var and std::env::var_os via. Clippy's disallowed_methods functionality? And then explicitly allow it inside the implementation of getenv
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
under `#[cfg(test)]` Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Thanks! Applied the suggestion, and turns out that the entire Took me a while to change them to use a generic function. |
The generic params would cause every function to be mono-ed twice, result in longer compilation time and larger binary. Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
Create a new local fn `has_msbuild_version` in `find_vs_version` to make the invocation easier. Also add `#[inline(always)]` to `has_msbuild_version` since it is only used in `find_vs_version` Signed-off-by: Jiahao XU <[email protected]>
Signed-off-by: Jiahao XU <[email protected]>
|
Updated the code to remove any generics in Having generics would make |
| @@ -3534,7 +3540,7 @@ impl Build { | |||
| // Loop through PATH entries searching for each toolchain. This ensures that we | |||
| // are more likely to discover the toolchain early on, because chances are good | |||
| // that the desired toolchain is in one of the higher-priority paths. | |||
| env::var_os("PATH") | |||
There was a problem hiding this comment.
It looks like this change causes crates to rebuild on Windows any time the PATH changes. Anecdotally, that seems to have a pretty high false positive rate, and since some PATH changes are associated with tools like rust-analyer that run constantly in the background, this can make it so that every build is a complete rebuild in some setups / with some dependencies. See for example BLAKE3-team/BLAKE3#324. I might file this as an issue shortly, but I wanted to mention it real quick before I forget.
There was a problem hiding this comment.
Perhaps we can special-case PATH here?
For example, Update the getenv to not emit anything for PATH
There was a problem hiding this comment.
Just crosslinking, should be fixed in #1215
So that we would get:
Also some optimisation