-
-
Notifications
You must be signed in to change notification settings - Fork 988
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
Better os version handling #3448
Conversation
src/desktop/version.cpp
Outdated
#endif | ||
|
||
// | ||
// POSIX uname version. |
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.
Please don't remove this code. It's a fallback in case /etc/lsb_release
doesn't exist.
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.
Done - 1bb926a
src/desktop/version.cpp
Outdated
@@ -151,6 +151,22 @@ std::string os_version() | |||
if(!ver.empty()) { | |||
return ver; | |||
} | |||
} else { |
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 better to not use else
for this, in case /etc/lsb_release
is empty (see the check right above).
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.
I see, that's true
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.
Fixed - e5411de
If you don't like my idea with renaming namespaces to |
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.
I don't really care about namespace renaming either way.
src/desktop/version.cpp
Outdated
// | ||
|
||
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.
Trailing space, please remove.
src/desktop/version.cpp
Outdated
utsname u; | ||
|
||
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.
Trailing space.
src/desktop/version.cpp
Outdated
if(uname(&u) != 0) { | ||
ERR_DU << "os_version: uname error (" << strerror(errno) << ")\n"; | ||
} | ||
|
||
return formatter() << u.sysname << ' ' | ||
|
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.
Trailing space.
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.
Fixed - d6258b5
src/desktop/version.cpp
Outdated
|
||
return formatter() << u.sysname << ' ' | ||
return formatter() << u.sysname << ' ' |
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.
Please don't try to align the fields (they are not aligned; note that the following lines are indented with tabs).
Thank you for the pull request! 👍 |
This new implementation works more reliably, avoids spawning subshells, and is faster.
This new implementation works more reliably, avoids spawning subshells, and is faster.
This new implementation works more reliably, avoids spawning subshells, and is faster. (cherry-picked from commit 4282ee3)
Old os version detection was in case of macOS buggy so I reworked it. It is faster now and native.
TODO
Idea
I used
desktop::apple::os_version()
namespace. It may be nice to rework rest of platform specific code to use this namespace style (not only os_version, but also notifications, ...)