-
Notifications
You must be signed in to change notification settings - Fork 275
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
Stamp scala_import jars #1160
Stamp scala_import jars #1160
Conversation
94555b6
to
4d485b0
Compare
Few problems:
|
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.
looks good in general. see one question inside.
FYI for other people watching- I was curious on why I didn't use symlink before and found out it was only introduced in 1.0 and even then it was experimental and required a bazel flag
@@ -4,8 +4,8 @@ load("//scala:scala.bzl", "scala_junit_test", "scala_test") | |||
|
|||
common_jvm_flags = [ | |||
"-Dplugin.jar.location=$(location //third_party/dependency_analyzer/src/main:dependency_analyzer)", | |||
"-Dscala.library.location=$(location //external:io_bazel_rules_scala/dependency/scala/scala_library)", | |||
"-Dscala.reflect.location=$(location //external:io_bazel_rules_scala/dependency/scala/scala_reflect)", | |||
"-Dscala.library.location=$(rootpath //external:io_bazel_rules_scala/dependency/scala/scala_library)", |
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.
don't remember the difference between location and rootpath. Do you remember?
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.
I don't think they are good for runtime information, but as we use them there's some info on them:
https://docs.bazel.build/versions/master/be/make-variables.html#predefined_label_variables
and
4b93af6
to
fec435d
Compare
7a115d0
to
bff248e
Compare
Status of various stamping techniques:
Currently the only way is to proceed with custom |
I think we should advance with custom ijar. |
Custom ijar source is under third_party, I don't think it's a security concern. I don't want to have opt-in, as it would be very confusing for users (buggy stamping, etc). |
ok |
"--nostrip_jar", | ||
"--target_label", | ||
ctx.label.name, | ||
symlink_file.path, |
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.
Why symlink is needed? Can't you just pass jar.path
?
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.
Because bazel does not allow modifications outside package
], | ||
) | ||
# | ||
#cc_binary( |
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.
Maybe remove this?
srcs = glob(["**"]), | ||
) | ||
|
||
#filegroup( |
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.
I'd say remove commented coded
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.
I left it commented out to have it easier to update if needed. Otherwise it's a bit harder to pick what's needed to be 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.
Looks good to me.
I didn't look at ijar/zlib code.
I though there is a need only for ijar, why there is code for zlib?
It's a source dependency used to process jar archives |
@liucijus thanks for your answers. LGMT |
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.
Thanks!
Probably very naive approach on stamping
scala_import
jars. @ittaiz WDYT?