-
Notifications
You must be signed in to change notification settings - Fork 48
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
Fix com.intuit.player:j2v8
transitive deps
#256
Changes from all commits
66b690f
a6bafc5
a615337
c1e4fdc
6f1858e
dd571f3
b4b4979
29580fd
180dd67
ababee1
d0c58e8
1462191
780becf
77af9f2
28d22ff
5db4688
eafff92
1fbe6d5
54f569a
2b68604
78345ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,36 @@ | ||||
def _build_constants_impl(repository_ctx): | ||||
_BUILD_FILE = """ | ||||
# DO NOT EDIT: automatically generated for _build_constants rule | ||||
filegroup( | ||||
name = 'files', | ||||
srcs = glob(['**']), | ||||
visibility = ['//visibility:public'] | ||||
) | ||||
""" | ||||
repository_ctx.file("BUILD", _BUILD_FILE, False) | ||||
|
||||
version = repository_ctx.read(repository_ctx.attr.version_file) | ||||
|
||||
_CONSTANTS_FILE = """ | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is this building version constants for the whole repo? i'm curious to see what this looks like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yep! This is what usage looks like: Line 2 in 78345ac
This is required because the new There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Inspiration in thread: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now that i'm reading this again, it's not dynamic or I guess i'm asking what's the diff between this and creating that output ourselves There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm, I'm not sure I understand what you mean. It's dynamic in the sense that it makes the version defined in our The only thing that's hardcoded is the |
||||
# DO NOT EDIT: automatically generated for _build_constants rule | ||||
VERSION = \"{version}\" | ||||
""" | ||||
|
||||
repository_ctx.file( | ||||
"constants.bzl", | ||||
_CONSTANTS_FILE.format(version = version), | ||||
False, | ||||
) | ||||
|
||||
_build_constants = repository_rule( | ||||
implementation = _build_constants_impl, | ||||
attrs = { | ||||
"version_file": attr.label( | ||||
default = Label("//:VERSION"), | ||||
allow_single_file = True, | ||||
), | ||||
}, | ||||
) | ||||
|
||||
def build_constants(): | ||||
_build_constants(name = "build_constants") |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,9 @@ j2v8_platform("macos") | |
|
||
j2v8_platform("linux") | ||
|
||
# TODO: These should probably be AARs? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. there's no android specific resource here, right? I think i stripped all that out in order to fit the debugger in the jvm repo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The main problem here is they rely on transitive AARs, so if you try to pull the JAR into a Gradle project, it'll technically work, but will blow up when it tries to process the transitive deps. So, the idea is to make this an Android only artifact up front to make it clearer where this is meant to be used. That being said, I'm not sure that's a great way to do it. From looking at a lot of the Android libs, a lot of them are JARs if they don't have any bundled Android resources. |
||
j2v8_platform("android") | ||
|
||
j2v8_platform("android-debug") | ||
|
||
j2v8_platform("all") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this part work now? i noticed you removed the dep for
android_binary
, I'm not following how this source is fixedThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Somewhat hilariously, I think this is because it's a transitive dependency of
com.github.AlexTrotsenko:j2v8-debugger
, so it gets pulled into the@maven
workspace.I could be wrong, and maybe it just works now, but that's the main thing that changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😅