Skip to content

Conversation

pugachAG
Copy link
Contributor

@pugachAG pugachAG commented Jan 3, 2023

https://blog.rust-lang.org/2022/12/15/Rust-1.66.0.html

Updated the same files as in the previous version bump: #7993

This enables #7650 (see this comment).

Also includes fixes for the following warning:

warning: for loop over an `Option`. This is more readably written as an `if let` statement
     ...
     = note: `#[warn(for_loops_over_fallibles)]` on by default

@pugachAG pugachAG requested a review from a team as a code owner January 3, 2023 13:19
@pugachAG pugachAG requested review from akhi3030 and jakmeier January 3, 2023 13:19
@pugachAG pugachAG marked this pull request as draft January 3, 2023 13:24
@pugachAG pugachAG marked this pull request as ready for review January 3, 2023 13:49
Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

But please wait before we merge it. Daily estimation is currently broken and I want to fix it before we merge this. Then we have a before-vs-after comparison for what the Rust upgrade did to performance. (Usually it's unnoticeable but we want to verify that)

@jakmeier
Copy link
Contributor

jakmeier commented Jan 3, 2023

Fix available in #8285

But we should wait one day after merging the fix so the fix and this PR end up in different daily runs.

Copy link
Contributor

@jakmeier jakmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix has been merged and I started a manual CI build. This PR here could also be merged now.

@jakmeier jakmeier linked an issue Jan 6, 2023 that may be closed by this pull request
@near-bulldozer near-bulldozer bot merged commit 1d3e763 into near:master Jan 6, 2023
near-bulldozer bot pushed a commit that referenced this pull request Jan 9, 2023
)

With #8284 we upgraded cargo version to `1.66`, so rust-lang/cargo#11114 is available. This PR is the same as #7130, but we can safely merge it now.

See #7650 for more context.

In order to test it I've used the code from [`librocksdb-sys/build.rs`](https://github.com/rust-rocksdb/rust-rocksdb/blob/master/librocksdb-sys/build.rs#L111) as part of `neard/build.rs`:
```
 fn main() {
-    if let Err(err) = try_main() {
-        eprintln!("{}", err);
-        std::process::exit(1);
+    let target_feature = std::env::var("CARGO_CFG_TARGET_FEATURE").unwrap();
+    let target_features: Vec<_> = target_feature.split(',').collect();
+    if target_features.contains(&"neon") {
+        panic!("FAIL: {target_feature}");
+    } else {
+        panic!("OK: {target_feature}");
     }
 }
```

It was was tested on M1 macbook, so I've changed the feature to `neon` (which is enabled by default), my `.cargo/config.toml` is as follows:
```
-[target.'cfg(target_arch = "x86_64")']
-rustflags = ["-Ctarget-feature=+sse4.1,+sse4.2", "-Cforce-unwind-tables=y"]
+[target.'cfg(target_arch = "aarch64")']
+rustflags = ["-Ctarget-feature=-neon", "-Cforce-unwind-tables=y"]
```

`neon` was successfully removed when using `1.66` toolchain:
```
OK: crc,...,vh
```

while it was still present with `1.65`:
```
FAIL: aes,..,neon,...,vh
```

This proves that rustflags are passed as env variable to `build.rs` in `1.66`, which was not the case in `1.65`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update rust toolchain to 1.66 when it is available
2 participants