Skip to content
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

Build with Bazel. #232

Merged
merged 5 commits into from
Dec 18, 2017
Merged

Build with Bazel. #232

merged 5 commits into from
Dec 18, 2017

Conversation

qzmfranklin
Copy link
Contributor

This PR adds native Bazel BUILD file to glog, enabling glog to be used both as an external repo (as in git_repository) or included in source.

It is easy to test:

# Make sure bazel is installed
bazel build :glog

This PR also has support for building glog with gflags. Indeed, that is the default behavior. If one wishes to build glog without gflags, he can do that via the following patch:

diff --git a/BUILD b/BUILD
index 7bbd32d..d1f60d4 100644
--- a/BUILD
+++ b/BUILD
@@ -1,4 +1,4 @@
 licenses(['notice'])
 
 load(':bazel/glog.bzl', 'glog_library')
-glog_library()
+glog_library(with_gflags=0)

Copy link
Member

@drigz drigz left a comment

Choose a reason for hiding this comment

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

Some comments about avoiding leaking headers into the rules that depend on this. gflags/gflags#233 and the associated PRs have more info.

bazel/glog.bzl Outdated
'src/config.h.cmake.in',
],
outs = [
'/'.join([PACKAGE_NAME, 'config.h']) if PACKAGE_NAME else 'config.h',
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it will expose config.h to users of glog in the same way as gflags/gflags#233. There are two options to deal with it in gflags/gflags#234 and gflags/gflags#235.

WORKSPACE Outdated
tag = 'v2.2.1',
)

bind(
Copy link
Member

Choose a reason for hiding this comment

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

bind() is not recommended. glog.bzl should refer to @com_github_gflags_gflags//:gflags.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @drigz ,

Thanks a lot for these nice review comments! I completely did not notice them until today!

All your comments are extremely valid and valuable. Thanks!

Will reply one by one next.

bazel/glog.bzl Outdated
name = 'internal_headers',
hdrs = internal_headers,
includes = [
PACKAGE_NAME,
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason you're using PACKAGE_NAME everywhere? (PACKAGE_NAME is deprecated, by the way - use native.package_name() instead).

I think you could use copts = ["-Isrc"] or strip_include_prefix = "src" and then generate headers into src/glog/.

bazel/glog.bzl Outdated
],
hdrs = [
'src/base/mutex.h',
'src/demangle.h',
Copy link
Member

Choose a reason for hiding this comment

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

I think most of these should be in hdrs as they're not meant to be used by users of glog. See header inclusion checking.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given that hdrs are public interfaces, did you actually mean that most of these should be in srcs instead of hdrs?

bazel/glog.bzl Outdated
'src/glog/log_severity.h',
],
includes = [
'src',
Copy link
Member

Choose a reason for hiding this comment

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

This means that any users who use #include "utilities.h" to try to include their own utilities.h might accidentally pick up glog's utilities.h.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Super good point here. This, and the comment below, will be addressed together.

@qzmfranklin qzmfranklin deleted the bazel branch December 14, 2017 07:35
@qzmfranklin qzmfranklin reopened this Dec 14, 2017
This commit addresses a few issues:

    1.  No longer leak config.h in a way similar to
            gflags/gflags#233
        The solution of prefixing the path by 'glog_internal' is modified from
            gflags/gflags#234

    2.  No longer expose internal headers.

    3.  Replace PACKAGE_NAME with native.package_name()

    4.  Uers can choose namespaces via the newly added 'namespace' keyword.

    5.  Replace glob with explicitly listing of files.

    6.  Make the genrules more compact using pythonic list construction.
@qzmfranklin
Copy link
Contributor Author

@drigz : I believe at this point I addressed all your previous comments:

1.  No longer leak config.h in a way similar to
        gflags/gflags#233
    The solution of prefixing the path by 'glog_internal' is modified from
        gflags/gflags#234

2.  No longer expose internal headers that are not part of the public API.

3.  Replace PACKAGE_NAME with native.package_name()

4.  Uers can choose namespaces via the newly added 'namespace' keyword.

5.  Replace glob with explicitly listing of files.

6.  Make the genrules more compact using pythonic list construction.

7.  Remove usage of bind.

Tested on a Ubuntu 16.04 x86_64 machine using the gcc (Ubuntu 5.4.0-6ubuntu1~16.04.5) 5.4.0 20160609.

@shinh
Copy link
Collaborator

shinh commented Dec 15, 2017

Let me defer review and approval to @drigz as I'm not familiar with Bazel.

@qzmfranklin
Copy link
Contributor Author

@shinh
Thanks~ I believe I have addressed @drigz 's comments so far. But I would be happy for more iterations.

@drigz
Copy link
Member

drigz commented Dec 18, 2017

@qzmfranklin This all looks good to me. Thanks for the contribution!

@drigz drigz merged commit fc87161 into google:master Dec 18, 2017
@qzmfranklin
Copy link
Contributor Author

@drigz @shinh

Thanks a lot! Really appreciated it!

@yichuan1118
Copy link

I'm including glog as a dependency in my bazel project, but it is not working.

http_archive(
    name = "com_github_google_glog",
    strip_prefix = "glog-master",
    url = "https://github.com/google/glog/archive/master.zip",
)
bind(
    name   = "glog",
    actual = "@com_github_google_glog//:glog",
)

and currently getting the following error:

INFO: Analysed target //main:trainer (0 packages loaded).
INFO: Found 1 target...
ERROR: /private/var/tmp/_bazel_yichuan/c27d8b500f97d7e743edb996310c038c/external/com_github_google_glog/BUILD:5:1: C++ compilation of rule '@com_github_google_glog//:glog' failed (Exit 1)
In file included from external/com_github_google_glog/src/symbolize.cc:55:
external/com_github_google_glog/src/utilities.h:148:1: error: unknown type name '_START_GOOGLE_NAMESPACE_'
_START_GOOGLE_NAMESPACE_
^
external/com_github_google_glog/src/utilities.h:150:1: error: expected unqualified-id
namespace glog_internal_namespace_ {
^
external/com_github_google_glog/src/utilities.h:168:1: error: unknown type name 'int64'; did you mean 'google::int64'?
int64 CycleClock_Now();
^~~~~
google::int64
bazel-out/darwin-fastbuild/bin/external/com_github_google_glog/_virtual_includes/glog/glog/logging.h:94:17: note: 'google::int64' declared here
typedef int64_t int64;
                ^
In file included from external/com_github_google_glog/src/symbolize.cc:55:
external/com_github_google_glog/src/utilities.h:170:1: error: unknown type name 'int64'; did you mean 'google::int64'?
int64 UsecToCycles(int64 usec);
^~~~~
google::int64
bazel-out/darwin-fastbuild/bin/external/com_github_google_glog/_virtual_includes/glog/glog/logging.h:94:17: note: 'google::int64' declared here
typedef int64_t int64;
                ^
In file included from external/com_github_google_glog/src/symbolize.cc:55:
external/com_github_google_glog/src/utilities.h:170:20: error: unknown type name 'int64'; did you mean 'google::int64'?
int64 UsecToCycles(int64 usec);
                   ^~~~~
                   google::int64
bazel-out/darwin-fastbuild/bin/external/com_github_google_glog/_virtual_includes/glog/glog/logging.h:94:17: note: 'google::int64' declared here
typedef int64_t int64;
                ^
In file included from external/com_github_google_glog/src/symbolize.cc:55:
external/com_github_google_glog/src/utilities.h:175:1: error: unknown type name 'int32'; did you mean 'google::int32'?
int32 GetMainThreadPid();
^~~~~
google::int32
bazel-out/darwin-fastbuild/bin/external/com_github_google_glog/_virtual_includes/glog/glog/logging.h:92:17: note: 'google::int32' declared here
typedef int32_t int32;
                ^
In file included from external/com_github_google_glog/src/symbolize.cc:55:
external/com_github_google_glog/src/utilities.h:236:1: error: unknown type name '_END_GOOGLE_NAMESPACE_'
_END_GOOGLE_NAMESPACE_
^
external/com_github_google_glog/src/utilities.h:238:1: error: expected unqualified-id
using namespace GOOGLE_NAMESPACE::glog_internal_namespace_;
^
external/com_github_google_glog/src/symbolize.cc:931:1: error: unknown type name '_START_GOOGLE_NAMESPACE_'
_START_GOOGLE_NAMESPACE_
^
external/com_github_google_glog/src/symbolize.cc:934:1: error: expected unqualified-id
bool Symbolize(void *pc, char *out, int out_size) {
^
external/com_github_google_glog/src/symbolize.cc:939:1: error: unknown type name '_END_GOOGLE_NAMESPACE_'
_END_GOOGLE_NAMESPACE_
^
external/com_github_google_glog/src/symbolize.cc:941:7: error: expected unqualified-id
#endif
      ^
12 errors generated.

@drigz
Copy link
Member

drigz commented Apr 12, 2018

@yichuan1118 I can't reproduce the issue, and the continuous build on macOS doesn't have the same problem, so we'll need more info. Could you file a separate issue, specifying:

  • which version of glog you're using (ie specify a SHA1 for the http_archive instead of using master)
  • what operating system version you're using and what compiler version (clang --version) you have
  • the output of bazel version
  • whether this works from a minimal workspace (ie a directory only containing WORKSPACE, which only depends on gflags and glog, then try building with bazel build @com_google_glog//:glog)
  • the output of bazel build -s @com_google_glog//:glog

Thanks!

@yichuan1118
Copy link

@drigz Thank you for getting back to me. After some digging I found the issue is that the gflags I'm including is a bit too old, for some reason it is causing this issue. After upgrading to gflags master, everything is working now. Thank you!

@pdu
Copy link

pdu commented Apr 30, 2018

When are you going to release this as the new version? Because we have standards to only use the released tags.

durswd pushed a commit to durswd/glog that referenced this pull request Sep 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants