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>
| 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
| { | ||
| 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
|
|
||
| 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.
|
Per discussion here I think we're basically only down to the I think we should probably have tracking issues for our PRs in general; I filed #1414 for that. Closing this one to get it off the board until we pick it back up. |
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