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.

Sign up for GitHub

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

Require JavaInfo on deps, move scrooge srcjar to srcs #523

Merged
merged 9 commits into from
Jun 20, 2018
Merged

Require JavaInfo on deps, move scrooge srcjar to srcs #523

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
564e1bc
move scrooge srcjar to srcs
johnynek Jun 15, 2018
ad920a0
require JavaInfo, add a test to avoid thrift jars on classpath
johnynek Jun 20, 2018
8d63397
fix strict build, run lint fix
johnynek Jun 20, 2018
90dcb05
address review comments
johnynek Jun 20, 2018
d1a3d5e
Merge branch 'master' into oscar/put_scrooge_into_src
johnynek Jun 20, 2018
4dc2edd
merge master, fix tests
johnynek Jun 20, 2018
2a78628
use JavaInfo directly
johnynek Jun 20, 2018
eb50d82
fix tests, take N
johnynek Jun 20, 2018
026fdb7
fix take N+1, update README, make runtime_deps strict
johnynek Jun 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion scala/private/rule_impls.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,9 @@ def _lib(ctx, non_macro_lib):
# this information through. extra_information allows passing
# this information through, and it is up to the new_targets
# to filter and make sense of this information.
extra_information=_collect_extra_information(ctx.attr.deps),
# unfortunately, we need to see this for scrooge and protobuf to work,
# but those are generating srcjar, so they should really come in via srcs
extra_information=_collect_extra_information(ctx.attr.deps + ctx.attr.srcs),
Copy link
Member

Choose a reason for hiding this comment

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

I’m sorry but I don’t understand this. Where does scroog euse this?

Copy link
Member Author

Choose a reason for hiding this comment

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

search for all the extra information in the file. For instance: https://github.com/bazelbuild/rules_scala/blob/master/twitter_scrooge/twitter_scrooge.bzl#L85

Really this is something that should be replaced with #510 but that is staged behind a few layers of tech debt removal (a supported skylark function to compile scala and get a JavaInfo, and a toolchain that sets up all the things we need for scala so we don't have to copy-paste the scala setup for the ctx).

jars_to_labels = jars.jars2labels,
)

Expand Down
5 changes: 2 additions & 3 deletions twitter_scrooge/twitter_scrooge.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,12 +289,11 @@ def scrooge_scala_library(name,
with_finagle = with_finagle,
)

# deps from macro invocation would come via srcjar
# however, retained to make dependency analysis via aspects easier
scala_library(
name = name,
srcs = [srcjar],
Copy link
Contributor

Choose a reason for hiding this comment

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

does this work? i think the scala_library rule is restricted to only allow actual files in the srcs not targets?

Copy link
Contributor

Choose a reason for hiding this comment

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

(or does it intelligently realize the output file?... interesting)

Copy link
Member Author

Choose a reason for hiding this comment

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

it does use the output file here. We could be more precise and just list the output filename. That might actually solve the problem.... Maybe by putting the whole target we are sucking up all the deps.

deps = deps + remote_jars + [
srcjar, "//external:io_bazel_rules_scala/dependency/thrift/libthrift",
"//external:io_bazel_rules_scala/dependency/thrift/libthrift",
"//external:io_bazel_rules_scala/dependency/thrift/scrooge_core"
],
exports = deps + remote_jars + [
Expand Down