Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Conversation

ryoqun
Copy link
Contributor

@ryoqun ryoqun commented Apr 7, 2023

Reverts #30897

I want to use https://doc.rust-lang.org/nightly/std/sync/struct.Arc.html#method.into_inner, which is merged upstream after 1 day after our current nightly rustc...

cc: @yihau @ilya-bobyr

@ryoqun ryoqun requested review from brooksprumo and ilya-bobyr and removed request for ilya-bobyr April 7, 2023 07:00
@ilya-bobyr
Copy link
Contributor

Seems like the CI is showing the failure that caused the #30897 revert:
https://buildkite.com/solana-labs/solana/builds/93497#01875a84-c8a7-4fa3-adb1-ecf0793c956d/1701
I could not reproduce it locally, for some reason.

}

#[test]
#[ignore]
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it OK that geyser plugins could be broken on master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nope.. ;)

I'll try to repro this as well later.

@codecov
Copy link

codecov bot commented Apr 7, 2023

Codecov Report

Merging #31099 (5aad720) into master (4e3300e) will decrease coverage by 0.1%.
The diff coverage is n/a.

❗ Current head 5aad720 differs from pull request most recent head ff37ea7. Consider uploading reports for the commit ff37ea7 to get more accurate results

@@            Coverage Diff            @@
##           master   #31099     +/-   ##
=========================================
- Coverage    81.5%    81.4%   -0.1%     
=========================================
  Files         733      728      -5     
  Lines      206917   205019   -1898     
=========================================
- Hits       168759   167043   -1716     
+ Misses      38158    37976    -182     

@@ -1,2 +1,2 @@
[toolchain]
channel = "1.67.1"
channel = "1.68.0"
Copy link
Contributor

@brooksprumo brooksprumo Apr 7, 2023

Choose a reason for hiding this comment

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

We could also bump to 1.68.2 now. Or, if you only want to bump nightly, the bump to stable can be removed entirely.

(I understand that then this PR wouldn't be a "revert" anymore though, so can be ignored.)

@ryoqun
Copy link
Contributor Author

ryoqun commented Apr 19, 2023

phew, finally found some time to work on this...

good news: newer nightly fixed (no extra work for us, yay)

I could not reproduce it locally, for some reason.

odd. i could easily repro this on my machine:

$ ./scripts/coverage.sh test_geyser_reload
...
running 1 test
error: test failed, to rerun pass `-p solana-geyser-plugin-manager --lib`

Caused by:
  process didn't exit successfully: `/home/sol/work/solana-labs/nightly-geyser-coverage/target/cov/debug/deps/solana_geyser_plugin_manager-39729159b0399d3b test_geyser_reload --nocapture` (signal: 11, SIGSEGV: invalid memory reference)
Failed: 101
^^^ +++
--- grcov
+ grcov target/cov/tmp --llvm --ignore '*.cargo*' --ignore '*build.rs' --ignore 'bench-tps*' --ignore 'upload-perf*' --ignore 'bench-streamer*' --ignore 'local-cluster*' -t html -o target/cov/lcov-local
+ grcov target/cov/tmp --llvm --ignore '*.cargo*' --ignore '*build.rs' --ignore 'bench-tps*' --ignore 'upload-perf*' --ignore 'bench-streamer*' --ignore 'local-cluster*' -t lcov -o target/cov/lcov.info
+ cd target/cov
+ tar zcf report.tar.gz lcov-local
-rw-r--r-- 1 sol users 101795 Apr 19 13:17 target/cov/lcov-local/index.html

@yihau could you bump nightly to 2023-04-19 by creating yet another docker image, which now passes on the same machine which can repro the segv:

$ git diff
diff --git a/ci/rust-version.sh b/ci/rust-version.sh
index 501d2a2bd0..2b43b606de 100644
--- a/ci/rust-version.sh
+++ b/ci/rust-version.sh
@@ -29,7 +29,7 @@ fi
 if [[ -n $RUST_NIGHTLY_VERSION ]]; then
   nightly_version="$RUST_NIGHTLY_VERSION"
 else
-  nightly_version=2023-01-22
+  nightly_version=2023-04-19
 fi

@brooksprumo
Copy link
Contributor

When I first tried the Rust 1.68.0 upgrade, we ran into issues due to sccache. The sccache issue doesn't appear to have any new activity, so I'm not sure if we'll be able to bump the nightly version 😢 Hopefully though!

@ilya-bobyr
Copy link
Contributor

Dependabot now needs 1.68 or a newer toolchain.
A PR that is supposed to add a nice error is still pending.
For now, dependabot is failing with multiple error that are not very descriptive, if a repo is pinned to any toolchain before 1.68.
So, until we upgrade to 1.68 our dependency updated process is also stalled.

@ilya-bobyr
Copy link
Contributor

Is there a reason we want to upgrade both stable and nightly version of Rust in the same PR?
Would it be acceptable to merge the stable update and work on the failures in nightly?

If it is acceptable, consider this PR: #31276

@brooksprumo
Copy link
Contributor

Is there a reason we want to upgrade both stable and nightly version of Rust in the same PR? Would it be acceptable to merge the stable update and work on the failures in nightly?

If it is acceptable, consider this PR: #31276

Usually we do both since we can, but it's not required. I've been bumping stable some of the last few times without bumping nightly due to memory layout changes that needed to be resolved.

@ryoqun ryoqun mentioned this pull request Apr 20, 2023
@ilya-bobyr
Copy link
Contributor

Is there a reason we want to upgrade both stable and nightly version of Rust in the same PR? Would it be acceptable to merge the stable update and work on the failures in nightly?
If it is acceptable, consider this PR: #31276

Usually we do both since we can, but it's not required. I've been bumping stable some of the last few times without bumping nightly due to memory layout changes that needed to be resolved.

From the standpoint of bisection and reverting changes, it is better to split things that do not have to be connected.

Not sure if there is some time saving optimization in the image generation process.
As it is manual, I guess, it is easier to do it once, rather than twice.


I think it is possible to create GitHub actions that create Docker images and publish it, when files change.
Not sure if this automation is strictly necessary here.
But it would remove the image creation manual step, making it possible to do the full update process via GitHub for anyone with repo write permissions.

@yihau
Copy link
Contributor

yihau commented Apr 20, 2023

we can make it automatically. The reason why I haven't implemented it cuz I have some ideas for our ci environment. If the improvement happen, we don't need it anymore. I can adjust the priority if you feel impeded!

@ilya-bobyr
Copy link
Contributor

we can make it automatically. The reason why I haven't implemented it cuz I have some ideas for our ci environment. If the improvement happen, we don't need it anymore. I can adjust the priority if you feel impeded!

Oh, no, I do not have any immediate need for it :)
I was thinking out loud I guess %)

It seems that the CI scripts will actually install the right toolchain, even if it is not available. Though having it in the image probably saves a few minutes in the CI run.

@ryoqun
Copy link
Contributor Author

ryoqun commented Apr 27, 2023

closing; there should be a pr with latest nightly rustc soon as all the blocking issues are now resolved. cc: @yihau

@ryoqun ryoqun closed this Apr 27, 2023
@mvines mvines deleted the revert-30897-pr/revert-rust-1.68.0 branch May 25, 2023 14:51
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants