-
Notifications
You must be signed in to change notification settings - Fork 316
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
bsls_timeutil: Fix OS_DARWIN compilation. #6
Conversation
I believe Andrew is on a business trip this week and has limited time/access to respond to this thread; given how simple this fix is, we should probably just proceed with merging it (or an equivalent fix). |
Fixes sys/time.h ifdef logic to include all OS_UNIX other than OS_LINUX and OS_AIX.
@kpfleming I left out the static_cast and made the patch simply fix the header include. |
sys/time.h is still needed on Linux because gettimeofday is declared there. I think it appears to work without it because it gets obtained via a transitive include. |
@azakhar OS_LINUX uses |
Sorry, I was wrong about gettimeofday. #ifdef logic for includes has a non-straightforward correlation to #ifdef logic of the implementation. But still, sys/time.h looks like an essential header which is used in at least one other source file (bsls_atomicoperations.t.cpp). Rather complicating the logic of includes here why can't we assume sys/time.h always exists and is safe to include? |
Because it does not always exist. We already had an issue where Ubuntu 12.04 does not provide <sys/time.h> by default, and building/testing BSL does not actually require it. The fix for that issue was to narrow the #includes to only bring in <sys/time.h> when it was known to be required. |
Does Ubuntu 12.04 provide build-essential by default? I assume there are some prerequisites to building/testing BSL, and sys/time.h (the package that provides it) can be included into those prerequisites. |
build-essential is available on Ubuntu 12.04 but not installed by default, but of course it is required to be able to build BSL (and pretty much any other C/C++ software). However build-essential (and the packages that it installs) does not provide sys/time.h. That header is only provided (for some unknown reason) by the multiarch libc6-dev package, which is not installed by default and is not necessary for any BSL builds. This change occurred in Debian unstable and is already present in Ubuntu, and will be part of Debian 7 when it ships. So, the simple answer is: many (possibly most) Linux systems do not require sys/time.h to build BSL, and some popular Linux systems do not provide it unless unusual packages are installed. Thus, the code was changed to not include sys/time.h on Linux. |
Based on this discussion, the clearest approach should be to include sys/time.h explicitly only for those platforms that require it. Some time back, Kevin proposed a solution that had each separate #ifdef sections for each dependency that sys/time.h satisfied (gethrtime or gettimeofday). Mike Giroux suggested using #elif to clearly show that these cases are mutually exclusive. I have posted the result on a review branch: review/issue-5-bsls-timeutil |
What about bsls_atomicoperations.t.cpp depending on sys/time.h? Also build-essential depends on libc6-dev and libc6-dev does provide sys/time.h in both i386 and amd64 packages: http://packages.ubuntu.com/precise/amd64/libc6-dev/filelist. It moved to x86_64-linux-gnu subdirectory but that should be in the -I path of gcc by default. |
Landed on commit e652e35 |
On my Ubuntu 12.04 systems, libc6-dev is installed, but /usr/include/sys/time.h is not present. The compiler claims it cannot find the file when I try compile something which tries to include it. Whether that lower-level directory should be found by the compiler or not is not something I'm aware of. |
Well... now I can't reproduce the original problem, so I'm not quite sure what to think. Compiling a simple foo.cpp that includes sys/time.h works just fine. I suspect that the reason BSL was not compiling before was because it overrides the compiler's built-in include paths and constructs its own, but that's just a mildly educated guess. |
I have merged the review branch into master. Closing this pull request. |
…r/drqs-100629050-bdld-topological-sortutil commit 927a7c9 Merge: 330a19b 3316ef1 Author: Henry Verschell <hverschell@bloomberg.net> Date: Tue Jul 25 15:05:17 2017 -0400 Merge pull request #1148 from afeher/drqs-100629050-bdld-topological-sortutil Drqs 100629050 bdlb topological sortutil commit 3316ef1 Author: Attila Feher <afeher@bloomberg.net> Date: Tue Jul 25 10:20:07 2017 -0400 Added duplicated edge test case. commit b11b142 Author: Attila Feher <afeher@bloomberg.net> Date: Mon Jul 24 16:45:24 2017 -0400 Non-copiable helper. commit 7cd03af Author: Attila Feher <afeher@bloomberg.net> Date: Mon Jul 24 15:52:39 2017 -0400 Adding allocators back. commit 0324cb4 Author: Attila Feher <afeher@bloomberg.net> Date: Wed Jul 19 15:56:28 2017 -0400 Review comments applied. commit eccbb93 Author: Attila Feher <afeher@bloomberg.net> Date: Fri Jul 14 14:18:24 2017 -0400 Add concepts' documentation Also renamed "mapping" to "edge". commit 0153faa Author: Attila Feher <afeher@bloomberg.net> Date: Fri Jul 14 13:07:58 2017 -0400 Review #6 commit 4193c8d Author: Attila Feher <afeher@bloomberg.net> Date: Wed Jul 12 14:03:34 2017 -0400 Review #5 commit e660415 Author: Attila Feher <afeher@bloomberg.net> Date: Mon Jul 10 10:38:06 2017 -0400 bde_verify commit 617f8f5 Author: Attila Feher <afeher@bloomberg.net> Date: Mon Jul 10 08:30:31 2017 -0400 Review #4 commit 056f533 Author: Attila Feher <afeher@bloomberg.net> Date: Thu Jul 6 17:46:54 2017 -0400 Matrix build bugfixes commit 4aae4a1 Author: Attila Feher <afeher@bloomberg.net> Date: Thu Jul 6 16:21:30 2017 -0400 Review #3 commit 8986d02 Author: Attila Feher <afeher@bloomberg.net> Date: Thu Jun 29 16:15:16 2017 -0400 Review #2 commit 278f6fb Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 14:23:05 2017 -0400 Review #1 commit fc0e763 Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 12:25:08 2017 -0400 Remove warning from test driver. commit 1cf374c Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 12:08:48 2017 -0400 Add back a string based sort. commit a5954c9 Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 11:52:24 2017 -0400 Use 'int' to order the same on all platforms. commit 4cbc133 Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 11:51:38 2017 -0400 Fix warnings. commit 98fbe71 Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 11:32:43 2017 -0400 Remove C++11 feature use. commit a978191 Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 10:15:56 2017 -0400 More tests. commit adba27a Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 09:52:36 2017 -0400 Change output argument type to pointer. commit 857d3be Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 09:17:50 2017 -0400 Implement tests. commit 2b0bd80 Author: Attila Feher <afeher@bloomberg.net> Date: Fri May 19 09:17:27 2017 -0400 Add HASH and EQUAL type parameters. commit c58c336 Author: Attila Feher <afeher@bloomberg.net> Date: Tue May 16 15:50:05 2017 -0400 DRQS 100629050 bdlb_topologicalsortutil.
* functions to create 'bdlt' data types from fuzz data * update contract to describe UB * fuzz testing additions and modifications * revisions to initial fuzzing * add BSLS_PRE_DONE macro to date functions to illustrate the use; modify implementation to follow the original design * removed unnecessary function and incorrect documentation * small change to DESCRIPTION * cleaning up, working toward prototype * minor changes * wip; proof-of-concept done; much work toward prototyping TBD * updating to use 'bslim_fuzzutil' * restoring files etc. * name changes * name changes and additions * more cleanup and name changes * added test driver for 'bsls_fuzztest' * added logic borrowed from 'bsls_asserttest' to determine component; modified test driver to invoke another bsls component out of contract * modified the precondition-check-failed-in-another-component test case * adding more versions of other component failure tests * adding main test driver code, though not certain it belongs in this component * revisions to the test driver and documentation * fixed behavior of 'handleException'; wrote and revised contracts, and updated documentation * update 'bdlt_date.t' to use 'bslim_fuzz*' components; remove inheritance from 'std::exception' * broke out exception into separate component; other cleanup * small changes * introduce fuzz testing * added 'RAW' macro and corresponding function. * added 'case 2' to test driver; remove 'return 0' from 'RAW' macro * added HandlerGuard; removed component, replaced with 'AssertTestException' * restructuring code to conform to BDE standards * working around the 'constexpr' issue * move #includes and modify doc * update documentation * modifications to enable the use of the 'BEGIN/END' macros in 'constexpr' functions * added logic to handle 'constexpr' context; added code to test * adding macros for IS_CONSTANT_EVALUATED * adding 'bsls_consteval' * test driver development * removing extraneous entry in .mem and fixing BDEVerify issues * replacing 'constexpr' with the appropriate macros * another attempt at compiling in MSVC-17 * another attempt at function vs macro * usage example * fix typo in doc * addressing BDEVerify issues * addressing BDEVerify issues * adding test driver for 'bsls_preconditions' * responding to review comments * modifying USAGE EXAMPLE and doc * cleaning up doc and test driver * addressing BDEVerify issues * adding testing for 'RAW' version of macro * USAGE EXAMPLE and other test driver changes * add flag to enable logging instead of 'failByAbort'; clean up test cases * updated USAGE EXAMPLE and TEST PLAN * removed count and logging of number of exceptions caught * BREATHING TEST and general test driver enhancements * add test case for 'FuzzTestHandlerGuard' and tighten up doc * addressing review comments; removed extraneous parameters * minor changes to logging * 'FuzzTestPreconditionTracker' testing * test driver for 'FuzzTestPreconditionTracker' * removed conditional #include; added '_IMP' methods; and in general addressing review feedback * further work addressing review comments on the test driver * addressing matric_build compilation issue * addressing matrix_build test run failures; and BDEVerify issues * add non-fuzzing 'bsls_preconditions.t' test; changed enums in 'bsls_fuzztest.t * reformatting * cleaning up the 'bsls_preconditions' component and test driver * addressing BDEVerify warnings * addressing BDEVerify issues * some explanation for '_IMP' versions * addressing BDEVerify issues * addressing BDEVerify issues * addressing bde_verify warnings * addressing review comments * further addressing review comments * name changes and addressing other review comments * revising test drivers; addressing review comments * addressing matrix_build issues * fixed matrix_build issues with C library on SunOS * adddressing 'strncmp' issue from matrix_build * address bde_verify issues and refine documentation * refined the component, removing redundant data member; replaced new macros with existing 'ASSERT_PASS/FAIL'; refined test driver * address bde_verify issues * addressing bde-verify issues and review comments * address matrix_build issues * replace ifdef inside of macro * handle matrix_build issue on Windows with LLVM function * trying to fix matrix_build problem on Windows * remove 'LLVMFuzzerTestOneInput' from both test drivers * addressing review comments, adding a new component 'bsls_fuzztestpreconditionviolation' to replace those places throwing 'AssertViolation' * address BDEVerify warnings * handle non-exc builds * strange issue with missing macros * handle dbg ufid * replace 'throw' w 'BSLS_THROW' * remove matrix_build warning for non-exc build * removing matrix_build warnings * fix matrix_build test run error * addressing review comments 1/n * addressing review comments 2/n, using new component 'bsls_bslsourcenameparserutil' * addressing some review comments * cleaning up * clean up and add fuzzing with preconditions to 'bdlt_calendar' * adding fuzzing w preconditions macros/tests * removing unwanted 'bsls_consteval' changes manually after rebase. * removing unwanted 'bsls_compilerfeatures' changes manually after rebase. * employ 'bdlt::FuzzUtil' in test driver * addressing review comments * addressing review comments; improve speed of fuzzing for 'Calendar' * addressing nested 'FuzzTestHandlerGuard' issue * Work in progress for fuzzing 'Calendar::isWeekendDay' * modifying fuzz test cases 9 and 10 for 'Calendar' * addressing review comments * clean up 'Calendar' test driver * addressing review comments * addressing review comments; removed unnecessary 'RAW' macros * addressing review comments * addressing review comments; modified USAGE EXAMPLE * responding to review comments * addressing review comments * addressing review comments * addressing review comments #2 * addressing review comments #3 * addressing review comments #4 * addressing review comments #5 * addressing review comments #6 * addressing review comments #7 * addressing 'bde_verify' issues * addressing review comments * addressing review comments #2 * addressing 'bde_verify' issues * addressing review comments * removing #includes and testing w matrix_build * removing #includes and testing w matrix_build #2 * Tidying up with a clean re-read * addressing bde_verify issues * responding to review comments * responding to review comments-2 Co-authored-by: Harold Bott <haroldbott@outlook.com>
Fixes sys/time.h ifdef logic to include all OS_UNIX other than
OS_LINUX and OS_AIX. Adds cast to ::times() clock_t return value
due to value being unsigned on OS_DARWIN.
Addresses issue reported in #5.