-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
#722 Adding Host Name in Reporting #733
Conversation
src/reporter.cc
Outdated
@@ -79,7 +82,7 @@ void BenchmarkReporter::PrintBasicContext(std::ostream *out, | |||
// No initializer because it's already initialized to NULL. | |||
const char *BenchmarkReporter::Context::executable_name; | |||
|
|||
BenchmarkReporter::Context::Context() : cpu_info(CPUInfo::Get()) {} | |||
BenchmarkReporter::Context::Context() : cpu_info(CPUInfo::Get()), sys_info(SystemInformation::Get()) {} |
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.
Linewrap
src/reporter.cc
Outdated
@@ -38,6 +38,9 @@ void BenchmarkReporter::PrintBasicContext(std::ostream *out, | |||
|
|||
Out << LocalDateTimeString() << "\n"; | |||
|
|||
const SystemInformation &sys_info = context.sys_info; | |||
Out << "System Name "<< sys_info.name << "\n"; |
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.
Is this printed only for the json dumper, or for all of them?
Put differently, i don't think it should be in the console output, it is getting rather crowded already.
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.
Removed from console and added only to JSON
src/sysinfo.cc
Outdated
#endif | ||
return str; | ||
#else | ||
const unsigned COUNT = 64; |
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.
Just use HOST_NAME_MAX
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.
HOST_NAME_MAX was not present on OSx, added local initialization.
The Job on Travis failed in config, can you please have a look at it?
src/sysinfo.cc
Outdated
std::string GetSystemName() { | ||
#if defined(BENCHMARK_OS_WINDOWS) | ||
std::string str; | ||
const unsigned COUNT = 32767; |
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.
Again, why invent a magic number.
MAX_COMPUTERNAME_LENGTH+1
?
And that being said, isn't that a rather too big to have as an array on stack?
Maybe use resized std::string
?
test/reporter_output_test.cc
Outdated
@@ -16,6 +16,7 @@ static int AddContextCases() { | |||
AddCases(TC_ConsoleErr, | |||
{ | |||
{"%int[-/]%int[-/]%int %int:%int:%int$", MR_Default}, | |||
{"System Name", MR_Next}, |
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.
What is "system name"?
How is this different from "host name"?
If it is exactly the "host name", just use that term?
❌ Build benchmark 1584 failed (commit f70f5b39af by @jatinx) |
❌ Build benchmark 1585 failed (commit 7f7bb81561 by @jatinx) |
✅ Build benchmark 1586 completed (commit 7e957a81be by @jatinx) |
✅ Build benchmark 1587 completed (commit ffd0fa611f by @jatinx) |
✅ Build benchmark 1589 completed (commit 7fda14a749 by @jatinx) |
@LebedevRI did the asked changes, can you please have a look at it? |
@LebedevRI its been a week, is something wrong with my design, would like some comments if thats the case. |
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.
Sorry, lost track.
src/sysinfo.cc
Outdated
str = hostname; | ||
#else | ||
std::wstring wStr = hostname; | ||
str = std::string(wStr.begin(), wStr.end()); |
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.
Does this work? Is this already done elsewhere in the library?
wchar is bigger than char, what makes sure that this isn't lossy?
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.
Using Standard wstring_convert to fix this
src/sysinfo.cc
Outdated
#ifdef BENCHMARK_OS_MACOSX //Mac Doesnt have a copy for Host Name in it | ||
#define HOST_NAME_MAX 64 | ||
#endif | ||
const unsigned COUNT = HOST_NAME_MAX; |
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.
Can't you just use HOST_NAME_MAX
directly, why do you need COUNT
variable?
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.
Removing unnecessary COUNT variable references, using macro directly
src/sysinfo.cc
Outdated
int retVal = gethostname(hostname, COUNT); | ||
if (retVal != 0) return std::string("Unable to Get Host Name"); | ||
return std::string(hostname); | ||
#endif |
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.
And others?
benchmark/src/internal_macros.h
Lines 39 to 73 in 439d6b1
#if defined(__CYGWIN__) | |
#define BENCHMARK_OS_CYGWIN 1 | |
#elif defined(_WIN32) | |
#define BENCHMARK_OS_WINDOWS 1 | |
#if defined(__MINGW32__) | |
#define BENCHMARK_OS_MINGW 1 | |
#endif | |
#elif defined(__APPLE__) | |
#define BENCHMARK_OS_APPLE 1 | |
#include "TargetConditionals.h" | |
#if defined(TARGET_OS_MAC) | |
#define BENCHMARK_OS_MACOSX 1 | |
#if defined(TARGET_OS_IPHONE) | |
#define BENCHMARK_OS_IOS 1 | |
#endif | |
#endif | |
#elif defined(__FreeBSD__) | |
#define BENCHMARK_OS_FREEBSD 1 | |
#elif defined(__NetBSD__) | |
#define BENCHMARK_OS_NETBSD 1 | |
#elif defined(__OpenBSD__) | |
#define BENCHMARK_OS_OPENBSD 1 | |
#elif defined(__linux__) | |
#define BENCHMARK_OS_LINUX 1 | |
#elif defined(__native_client__) | |
#define BENCHMARK_OS_NACL 1 | |
#elif defined(__EMSCRIPTEN__) | |
#define BENCHMARK_OS_EMSCRIPTEN 1 | |
#elif defined(__rtems__) | |
#define BENCHMARK_OS_RTEMS 1 | |
#elif defined(__Fuchsia__) | |
#define BENCHMARK_OS_FUCHSIA 1 | |
#elif defined (__SVR4) && defined (__sun) | |
#define BENCHMARK_OS_SOLARIS 1 | |
#endif |
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.
The only segregation needed here is POSIX vs non POSIX Compliance.
POSIX has gethostname
src/sysinfo.cc
Outdated
if (!GetComputerName(hostname, &DWCOUNT)) | ||
return std::string("Unable to Get Host Name"); | ||
#ifndef UNICODE | ||
str = hostname; |
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.
Are you 100.0% sure this is safe?
https://docs.microsoft.com/en-us/windows/desktop/api/winbase/nf-winbase-getcomputernamew
On input, specifies the size of the buffer, in TCHARs. On output, the number of TCHARs copied to the destination buffer, not including the terminating null character.
I'd recommend to use 'returned' DWCOUNT
.
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.
Initialising TCHAR to null and only copying DWCOUNT into string.
src/sysinfo.cc
Outdated
str = hostname; | ||
#else | ||
std::wstring wStr = hostname; | ||
str = std::string(wStr.begin(), wStr.end()); |
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.
Also, same comment about DWCOUNT
.
src/sysinfo.cc
Outdated
str = std::string(wStr.begin(), wStr.end()); | ||
#endif | ||
return str; | ||
#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.
#else | |
#else // defined(BENCHMARK_OS_WINDOWS) | |
// Catch-all POSIX block. |
src/sysinfo.cc
Outdated
int retVal = gethostname(hostname, COUNT); | ||
if (retVal != 0) return std::string("Unable to Get Host Name"); | ||
return std::string(hostname); | ||
#endif |
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.
#endif | |
#endif // Catch-all POSIX block. |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
CLAs look good, thanks! |
✅ Build benchmark 1598 completed (commit 5c4b5786b0 by @) |
@@ -77,6 +77,8 @@ bool JSONReporter::ReportContext(const Context& context) { | |||
std::string walltime_value = LocalDateTimeString(); | |||
out << indent << FormatKV("date", walltime_value) << ",\n"; | |||
|
|||
out << indent << FormatKV("host_name", context.sys_info.name) << ",\n"; |
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.
- We are sure that this won't contain
\\
? (seeexecutable_name
) - Do we want to print
"Unable to Get Host Name"
?2 - I wonder if this should be after the
executable_name
.
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.
- No. We should check and only output if it's available.
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.
- Yes, I checked the output of the function, it just outputs the hostname for windows without the
\\
- Addressed
- I am not sure about this one, it can be done, but since the output being in json only, is it desirable?
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.
Aren't we still always printing it?
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.
Looks about right other than this.
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.
are you talking about the 2nd point?
I have placed an empty string incase it fails to fetch the host name
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.
In general seems to look ok, but i'd prefer to leave this for @dominichamon.
✅ Build benchmark 1600 completed (commit 58dde37f5d by @jatinx) |
include/benchmark/benchmark.h
Outdated
@@ -1293,6 +1293,14 @@ struct CPUInfo { | |||
BENCHMARK_DISALLOW_COPY_AND_ASSIGN(CPUInfo); | |||
}; | |||
|
|||
struct SystemInformation { |
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.
nit: for consistency with CPUInfo, maybe SystemInfo.
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.
Can be done, let me know if you want me proceed with this
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.
Yes please.
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
src/sysinfo.cc
Outdated
TCHAR hostname[COUNT] = {'\0'}; | ||
DWORD DWCOUNT = COUNT; | ||
if (!GetComputerName(hostname, &DWCOUNT)) | ||
return std::string("Unable to Get Host Name"); |
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.
return empty string and don't print in that case?
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.
we can use something like this
https://github.com/google/benchmark/blob/master/src/benchmark.cc#L430-L431
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.
merging requested changes
Sometimes, on travis-ci, the last build on MacOS fails in configure. |
✅ Build benchmark 1603 completed (commit f61aac7286 by @jatinx) |
✅ Build benchmark 1604 completed (commit e16277d78a by @jatinx) |
Thanks! I'll keep an eye on the travis build. |
PR with CLA signed