Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bump nightly to 2023-08-25 #33048

Closed
wants to merge 5 commits into from
Closed

Conversation

t-nelson
Copy link
Contributor

Problem

ci is targeting a four month old nightly toolchain. as a result, bumping stable to 1.72 leads to nightly incremental builds failing on unstable symbols in build artifacts that have since stabilized 🙃

there are still a couple controversial lints/workarounds to debate here, hence draft. we'll pop them out to separate PR(s) as resolved

Summary of Changes

bump nightly toolchain to 2023-08-25

REQUIRES: #33047

@t-nelson t-nelson requested a review from Lichtso August 29, 2023 05:14
) -> Result<K, Box<dyn error::Error>> {
let SignerSource {
kind,
derivation_path,
legacy,
} = parse_signer_source(path)?;
match kind {
SignerSourceKind::Prompt => {
let skip_validation = matches.is_present(SKIP_SEED_PHRASE_VALIDATION_ARG.name);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

so there's some bug with const refs in generic contexts that triggers ICE in the targeted toolchain. took a day to isolate and stumble into this "fix" via introducing replicode.

didn't see any related issues on github and haven't had time to try to minimize repro yet

Copy link
Contributor

Choose a reason for hiding this comment

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

Have you reported this already? Even without minimal reproduction it might be worth giving them a heads up on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not yet. if we can put this commit in, i'll have something to permalink. don't think the pr links persist after i delete the branch. otherwise hoping to have time to attempt a minimization tonight.

@@ -185,6 +185,7 @@ impl<'a> AccountInfo<'a> {
pub fn assign(&self, new_owner: &Pubkey) {
// Set the non-mut owner field
unsafe {
#[allow(invalid_reference_casting)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Lichtso says the compiler's being hyperbolic in calling this UB 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

From Rust perspective it is UB, but in LLVM the subsequent volatile write will become pretty well defined again.

Comment on lines +1976 to +1978
// allow lint false positive trait bound requirement (`CryptoRng` only
// implemented on `&'a mut T`
#[rustversion::attr(since(1.73), allow(clippy::needless_pass_by_ref_mut))]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

comment pretty much covers it. allegedly this lint was introduced in 1.72, but that version was complaining that it doesn't know about it, hence the toolchain version gating.

i'm guessing this one won't be contentious and we can just put it in with a corresponding tracking issue

@@ -76,6 +76,7 @@ _ scripts/cargo-for-all-lock-files.sh -- "+${rust_nightly}" clippy --workspace -
--deny=clippy::integer_arithmetic \
--deny=clippy::manual_let_else \
--deny=clippy::used_underscore_binding \
--allow=clippy::items-after-test-module \
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rust-lang/rust-clippy#11153

allowing this one globally Feels Bad Man ™️, but the offending code is emitted by a proc-macro and we can't put it in the right place locally 🤕

@yihau
Copy link
Member

yihau commented Aug 29, 2023

just uploaded the new nightly image 🐳 and retried the pipeline. it failed due to

Screenshot 2023-08-29 at 1 52 39 PM

@t-nelson
Copy link
Contributor Author

oh i also hit some dumb git permissions thing with the docker image. still need to sort that out

@t-nelson
Copy link
Contributor Author

just uploaded the new nightly image 🐳 and retried the pipeline. it failed due to

Screenshot 2023-08-29 at 1 52 39 PM

it's in #33047

@t-nelson
Copy link
Contributor Author

closing in favor of #32961. we need to bump nightly and stable simultaneously to avoid breaking incremental builds

@t-nelson t-nelson closed this Aug 31, 2023
@t-nelson t-nelson deleted the n20230825 branch September 13, 2023 03:37
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.

3 participants