WIP/DNM: Use 2024 edition #1130
Conversation
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
Signed-off-by: John Eckersberg <jeckersb@redhat.com>
github-actions
bot
added
area/install
| @@ -229,7 +229,8 @@ match-architectures = ["x86_64", "aarch64"] | |||
| .to_string(); | |||
| let parsed_kargs = parse_kargs_toml(&file_content, sys_arch).unwrap(); | |||
| assert_eq!(parsed_kargs, ["console=tty0", "nosmt"]); | |||
| std::env::set_var("ARCH", "aarch64"); | |||
| // TODO: Audit that the environment access only happens in single-threaded code. | |||
There was a problem hiding this comment.
It definitely doesn't, rust runs unit tests in multiple threads by default. We need to stop setting this environment variable in the tests at all. At a quick look it's not clear we need this line of code, shouldn't we be just doing let sys_arch = "aarch64"?
There was a problem hiding this comment.
Yeah it doesn't even reference the environment variable anywhere. No idea what that was ever about. I'll go open a separate PR against main to fix just that.
There was a problem hiding this comment.
Yeah it doesn't even reference the environment variable anywhere. No idea what that was ever about. I'll go open a separate PR against main to fix just that.
lib/src/cli.rs
Outdated
| @@ -1012,7 +1013,7 @@ impl Opt { | |||
| I::Item: Into<OsString> + Clone, | |||
| { | |||
| let mut args = args.into_iter(); | |||
| let first = if let Some(first) = args.next() { | |||
| let first = match args.next() { Some(first) => { | |||
There was a problem hiding this comment.
Here and I think all the other changes this is cargo fix being conservative with the drop ordering change. IMO if let is easier to read than match and so we should not apply these changes.
There was a problem hiding this comment.
For reference since I had to go find it - https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-if-let-scope.html
| @@ -136,35 +136,35 @@ struct Generation { | |||
| dirmeta_found: BTreeSet<RcStr>, | |||
| } | |||
|
|
|||
| fn push_dirmeta(repo: &ostree::Repo, gen: &mut Generation, checksum: &str) -> Result<()> { | |||
| if gen.dirtree_found.contains(checksum) { | |||
| fn push_dirmeta(repo: &ostree::Repo, r#gen: &mut Generation, checksum: &str) -> Result<()> { | |||
There was a problem hiding this comment.
This is fine as an automated fix, but I think it'd be nicer to rename this to generation instead
There was a problem hiding this comment.
100%, I did exactly that manually before I realized that cargo fix would do its own thing.
We don't actually want to do this right now, but there wasn't much to it so I'll just post it for future-us when we actually do migrate