Skip to content
This repository has been archived by the owner on Jul 31, 2023. It is now read-only.

Build fix. #378

Merged
merged 2 commits into from
Aug 21, 2019
Merged

Build fix. #378

merged 2 commits into from
Aug 21, 2019

Conversation

g-easy
Copy link
Contributor

@g-easy g-easy commented Aug 21, 2019

Need rules_cc for abseil and build_bazel_rules_apple for grpc.

Need rules_cc for abseil and build_bazel_rules_apple for grpc.
@g-easy g-easy merged commit 033ae9a into census-instrumentation:master Aug 21, 2019
@g-easy g-easy deleted the build branch August 21, 2019 07:53
@@ -16,6 +16,13 @@ workspace(name = "io_opencensus_cpp")

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

# Build rules for C++ projects.
http_archive(
Copy link

Choose a reason for hiding this comment

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

For what it's worth, I would consider the fact that this dependency had to be added to your WORKSPACE a bug in the abseil-cpp repo. There doesn't appear to be anything stopping them from pulling this out to a deps() macro.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please correct me if I'm wrong but my understanding is that this will be required for all C++ projects after bazel 1.0(?) because rules like cc_library will move from native to skylark implementations.

Copy link

Choose a reason for hiding this comment

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

That may be the case for one hop. But suppose absl CPP requires rules_cc and your repo provides, say, a Golang tool. It's not intuitive to users of the Golang tool that they should have to pull in rules_cc despite never directly using a C++ rule.

This has already happened in the gRPC repo. We have to pull in a Golang-only tool used by Envoy even though we're only -- to my knowledge -- using some of their protos.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ditto. opencensus-cpp pulls in e.g. grpc-java, because opencensus-proto depends on it.

As for rules_cc specifically, I think opencensus-cpp will end up using them directly after bazel 1.0.

@@ -39,6 +46,20 @@ http_archive(
urls = ["https://github.com/google/benchmark/archive/master.zip"],
)

# Used by gRPC.
http_archive(
Copy link

Choose a reason for hiding this comment

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

You shouldn't need this particular http_archive invocation unless for some reason you really want master branch (which, btw, makes your build non-hermetic). You're already getting an instance from your call to grpc_deps(). It's just the load() and apple_rules_dependencies() invocations below that are necessary due to bazelbuild/bazel#1550.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! How does #379 look to you?

As for master, we are trying to follow Abseil's "live at head" philosophy. (...on our own master branch; whereas OpenCensus releases pin their deps)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants