-
Notifications
You must be signed in to change notification settings - Fork 56
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
Rename os_version to os.version #163
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tom Forbes <[email protected]>
@@ -231,6 +231,8 @@ impl From<&str> for Os { | |||
"aix" => Os::AIX, | |||
"android" => Os::Android, | |||
"darwin" => Os::Darwin, | |||
// std::env::consts::OS returns "macos" instead of darwin | |||
"macos" => Os::Darwin, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I ask you to create another commit or PR about this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's been a long time, I suggest maintainers close this PR and properly commit this fix themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, and this PR is also obsolete, since master
already has the title changes.
Actually, according to the spec, all the |
@MOZGIII Do you intend to create another PR to take over this one? |
No |
I believe this should be
os.version
likeImageConfig
?For example this is what Github container repo returns:
Also the tests failed on MacOS. I fixed this by canonicalising the temporary directory as well:
Unfortunately
std::env::consts::OS
returnsmacos
instead of Darwin, so I added an extra arm to the match condition to handle this. This makesDefault
work properly on MacOS.Same as #134