Skip to content

Conversation

@sarutak
Copy link
Member

@sarutak sarutak commented Dec 16, 2018

The following is a part of arrow/util/logging.cc.

#ifdef ARROW_USE_GLOG
typedef google::LogMessage LoggingProvider;
#else
typedef CerrLog LoggingProvider;
#endif

As you see, when ARROW_USE_GLOG is defined, glog is intended to be used but it's not never defined and glog is never used.

I've fixed this by adding add_definition command when CMake variable ARROW_USE_GLOG is ON.

@sarutak sarutak changed the title [C++] [ARROW-4049] Arrow never use glog even though glog is linked. ARROW-4049 [C++] Arrow never use glog even though glog is linked. Dec 16, 2018
@sarutak sarutak changed the title ARROW-4049 [C++] Arrow never use glog even though glog is linked. ARROW-4049: [C++] Arrow never use glog even though glog is linked. Dec 16, 2018
@codecov-io
Copy link

Codecov Report

Merging #3196 into master will increase coverage by 1.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3196      +/-   ##
==========================================
+ Coverage   86.42%   87.48%   +1.06%     
==========================================
  Files         504      446      -58     
  Lines       69623    65862    -3761     
==========================================
- Hits        60172    57620    -2552     
+ Misses       9354     8242    -1112     
+ Partials       97        0      -97
Impacted Files Coverage Δ
go/arrow/array/table.go
go/arrow/math/uint64_amd64.go
go/arrow/internal/testing/tools/bool.go
go/arrow/internal/bitutil/bitutil.go
go/arrow/memory/memory_avx2_amd64.go
go/arrow/array/null.go
go/arrow/datatype_nested.go
go/arrow/array/string.go
go/arrow/math/uint64_avx2_amd64.go
go/arrow/array/builder.go
... and 50 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ac047b2...87be741. Read the comment docs.

Copy link
Member

@wesm wesm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1. Thanks for catching this!

@wesm wesm closed this in 77d3a46 Dec 16, 2018
wesm pushed a commit that referenced this pull request Dec 20, 2018
After #3196, a potential bug appears.
If we use glog installed instead of downloading one at build time and the installed glog is linked to gflags, linker error can be occurred.

I modified ThirdpartyToolchain.cmake to add a dependency from glog to gflag.

Author: Kousuke Saruta <sarutak@oss.nttdata.com>

Closes #3234 from sarutak/ARROW-4052 and squashes the following commits:

3c65cbe <Kousuke Saruta> Modified ThirdpartyToolchain.cmake to add a dependency from glog to gflag
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.

3 participants