Skip to content

Consolidating external dependencies - #26

Open
tusharpm wants to merge 5 commits into
emilk:masterfrom
tusharpm:external
Open

Consolidating external dependencies#26
tusharpm wants to merge 5 commits into
emilk:masterfrom
tusharpm:external

Conversation

@tusharpm

Copy link
Copy Markdown
Contributor

This PR contains

  • the changes to collect Glog dependent projects (glog_bench and glog_example) under one directory.
  • the build script build_and_run.cmake (instead of shell), to use on Windows also.
  • A README.md explaining the key advantages of this approach, the build steps and a sample output (taken from Windows).

This allows running the glog_* tests on all platforms (with CMake). The same can be followed further for the internal tests (loguru_bench and loguru_example).

@emilk

emilk commented Jan 6, 2017

Copy link
Copy Markdown
Owner

Impressive improvements! However, this PR causes problems on my Mac:

loguru/external > cmake -P build_and_run.cmake

...

/Users/emilk/kod/loguru/external/glog/src/utilities.cc:141:7: error: use of undeclared identifier 'IsFailureSignalHandlerInstalled'
  if (IsFailureSignalHandlerInstalled()) {
      ^
/Users/emilk/kod/loguru/external/glog/src/utilities.cc:254:17: warning: 'syscall' is deprecated: first deprecated in macOS 10.12 - syscall(2) is unsupported; please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost(). [-Wdeprecated-declarations]
    pid_t tid = syscall(__NR_gettid);
                ^
/usr/include/unistd.h:733:6: note: 'syscall' has been explicitly marked deprecated here
int      syscall(int, ...);

The glog benchamrks/examples on the master branch both works (in those cases I am uses pre-compiled glog 0.3.4 from macports). Any ideas what is going wrong here? Maybe the submodule should be the 0.3.4 tag?

@tusharpm

tusharpm commented Jan 7, 2017

Copy link
Copy Markdown
Contributor Author

[Copy pasting from my reply to the email - GitHub notifications didn't pick that one up]

Thanks for writing back. I’ll try to address your concern to the best of my ability.

The primary problem I faced with glog v0.3.4 was that it didn’t have a CMakeLists.txt file. It’s not surprising considering the release is almost two years old (will be this March). This makes it difficult to be able to build it with the same commands across platforms (Linux, macOS and Windows). I chose the latest master because it has CMake support to build on all platforms (I tried Linux and Windows; evidently missed macOS).

The error you describe is in glog code. I’ll check if it builds independently on my mac.

(Just ran the tests): It builds fine (with the warnings though) on my machine (Sierra 10.12.2). To help you further, I need more details from your build environment – for instance the compiler you use (Apple clang? Version?) and any external factors that may have affected the build, like the GFlags namespace option, etc.

@emilk

emilk commented Feb 5, 2017

Copy link
Copy Markdown
Owner

Has this PR been superseded by the merging of #28? Can I close this?

@tusharpm

tusharpm commented Feb 5, 2017

Copy link
Copy Markdown
Contributor Author

[happened again with GitHub notification]
No. These PRs are mutually exclusive. This one modifies the build procedure of the glog-dependant projects in loguru to enable them on all platforms. #28 builds and tests loguru (with CI integration).

I believe we can address the concerns you had brought up earlier and merge this one separately. Meanwhile, I can try to keep this fast-forwardable.

tusharpm added 4 commits May 19, 2018 10:41
- Add Git submodules for `Glog` (master - require `CMakeLists.txt`) and `Gflags` (v2.2.0).
- Collect all glog dependent targets in loguru in `external/test`.
- Add `CMakeLists.txt` for building the libraries `glog` and `gflags` and the test projects `glog_bench` and `glog_example`.
- Add a CMake script `build_and_run.cmake` (wrapper) for building and running the tests.
- Add a `README.md` file explaing the intent and the steps to run the external tests locally.
- glog_bench in Release configuration produces better results on Windows
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants