Update rust version to 1.77, chrono to 0.4.35 #26273
Conversation
rjobanp
force-pushed
the
rust-77
branch
13 times, most recently
from
1739b49 to
7cd47f0
Compare
|
I just fixed what appears to be the last test failure (just kicked off another full re-run), if anyone wants to be a hero and do a final review/approval |
| # parse the package name from a package_id that looks like one of: | ||
| # git+https://github.com/MaterializeInc/rust-server-sdk#launchdarkly-server-sdk@1.0.0 | ||
| # path+file:///Users/roshan/materialize/src/catalog#mz-catalog@0.0.0 | ||
| # registry+https://github.com/rust-lang/crates.io-index#num-rational@0.4.0 | ||
| # file:///path/to/my-package#0.1.0 | ||
| package_id = message["package_id"] | ||
| if "@" in package_id: | ||
| package = package_id.split("@")[0].split("#")[-1] | ||
| else: | ||
| package = message["package_id"].split("#")[0].split("/")[-1] |
There was a problem hiding this comment.
Why was this change required? Maybe the upgrade to Rust 1.77 changed output of cargo build?
There was a problem hiding this comment.
Yes the spec of package-id was updated in the json output: rust-lang/cargo#13311
took a while to figure this out.
| @@ -300,7 +300,7 @@ pub(crate) struct ActiveComputeState<'a, A: Allocate> { | |||
| } | |||
|
|
|||
| /// A token that keeps a sink alive. | |||
| pub struct SinkToken(Box<dyn Any>); | |||
| pub struct SinkToken(#[allow(dead_code)] Box<dyn Any>); | |||
There was a problem hiding this comment.
| pub struct SinkToken(#[allow(dead_code)] Box<dyn Any>); | |
| pub struct SinkToken { | |
| _guard: Box<dyn Any>, | |
| } |
This and the other sites triggering "dead code" warning are because the inner fields aren't technically read, we just rely on their Drop impls getting called. If you make this a named field it should remove the need for #[allow(dead_code)]
There was a problem hiding this comment.
Yeah that'll be cleaner! Discussed offline with @ParkMyCar -- will address these in a follow up PR to get this update in asap since it'll take some time to update all instantiations of the relevant structs
| @@ -222,7 +222,7 @@ impl AvroDecode for RowDecoder { | |||
|
|
|||
| // Get around orphan rule | |||
| #[derive(Debug)] | |||
| pub(super) struct RowWrapper(pub Row); | |||
| pub(super) struct RowWrapper(#[allow(dead_code)] pub Row); | |||
There was a problem hiding this comment.
Row doesn't seem to be a guard here, but I'm not sure if we ever actually read from the created Row? Could you leave a comment here explaining that this might not actually be dead?
| /// 5874897-12-31, chrono is limited to December 31, 262142, which we mirror | ||
| /// here so we can use chrono's formatting methods and have guaranteed safe | ||
| /// conversions. | ||
| pub const HIGH_DAYS: i32 = 95_015_644; | ||
| pub const HIGH_DAYS: i32 = 95_015_279; |
There was a problem hiding this comment.
This is a little scary because a date could exist in MZ today that is after the new limit, do you know what would happen in this case? probably a panic?
There was a problem hiding this comment.
Yes unfortunately a panic will likely occur if a date of this value already exists. I mentioned to @benesch yesterday, I'm hopeful that since the max limit was defined by chrono and not by postgres, it's unlikely that any of our customers used it as some sort of notional timestamp maximum
Motivation
A recent update to https://github.com/MaterializeInc/rust-postgres/ uses new chrono features and upgrades the version of rust to 1.77. That update of rust-postgres is needed in #26186 so I decided to go ahead and update the version for MZ.
The majority of the changes are due to the deprecations of many timestamp methods on
chrono::NaiveDateTime-- see https://github.com/chronotope/chrono/releases/tag/v0.4.35 for details.Tips for reviewer
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.