Stamp scala_import jars #1160

liucijus · 2020-12-04T11:53:04Z

Probably very naive approach on stamping scala_import jars. @ittaiz WDYT?

liucijus · 2020-12-09T07:56:41Z

Few problems:

java_common.stamp_jar seems to be duplicating some Manifest attributes
due to symlinking, all rules that depend on toolchain deps need to have incompatible_use_toolchain_transition = True, set to prevent host and target dependencies mixing. I'll do it with a separate PR.

ittaiz

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

ittaiz · 2020-12-09T09:32:37Z

third_party/dependency_analyzer/src/test/BUILD

@@ -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)",


don't remember the difference between location and rootpath. Do you remember?

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

bazelbuild/bazel#2475 (comment)

liucijus · 2021-01-13T07:30:21Z

Status of various stamping techniques:

java_common.stamp_jar calls ijar tool, which incorrectly handles MANIFEST sections
singlejar does not preserve existing attributes
custom ijar works fine, but requires to bring in modified ijar sources

Currently the only way is to proceed with custom ijar until bazelbuild/bazel#12730 is fixed

ittaiz · 2021-01-13T07:57:02Z

I think we should advance with custom ijar.
How complicated will it be to only have custom ijar on opt-in?
I'm concerned that if some orgs are really strict on security then they might have a problem with the custom ijar and they might be blocked on upgrades.
If we can have this ability (and so also custom ijar requirement) be opt-in then maybe we have the best of both worlds.

liucijus · 2021-01-13T08:08:49Z

I think we should advance with custom ijar.
How complicated will it be to only have custom ijar on opt-in?
I'm concerned that if some orgs are really strict on security then they might have a problem with the custom ijar and they might be blocked on upgrades.
If we can have this ability (and so also custom ijar requirement) be opt-in then maybe we have the best of both worlds.

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).

ittaiz · 2021-01-13T08:14:46Z

ok

simuons · 2021-01-22T08:25:22Z

scala/scala_import.bzl

+            "--nostrip_jar",
+            "--target_label",
+            ctx.label.name,
+            symlink_file.path,


Why symlink is needed? Can't you just pass jar.path?

Because bazel does not allow modifications outside package

simuons · 2021-01-22T08:33:16Z

third_party/java_tools/ijar/BUILD

+    ],
+)
+#
+#cc_binary(


Maybe remove this?

simuons · 2021-01-22T08:33:47Z

third_party/java_tools/ijar/BUILD

+    srcs = glob(["**"]),
+)
+
+#filegroup(


I'd say remove commented coded

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.

simuons

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?

liucijus · 2021-01-22T09:04:17Z

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

simuons · 2021-01-22T09:37:44Z

@liucijus thanks for your answers. LGMT

ittaiz

Thanks!

google-cla bot added the cla: yes label Dec 4, 2020

liucijus force-pushed the stamp-scala-import branch from 94555b6 to 4d485b0 Compare December 7, 2020 13:28

liucijus mentioned this pull request Dec 9, 2020

Use incompatible_use_toolchain_transition in all rules #1164

Merged

ittaiz reviewed Dec 9, 2020

View reviewed changes

Vaidas Pilkauskas added 3 commits December 17, 2020 10:28

Stamp scala_import jars

6f29bd8

Use target providers from stamped jars

67bfdb3

Fix analyzer tests

75ad28d

liucijus force-pushed the stamp-scala-import branch from 4b93af6 to fec435d Compare January 6, 2021 08:12

Vaidas Pilkauskas added 2 commits January 11, 2021 11:39

Use singlejar directly instead of ijar to stamp jars

85f9c8c

Add scala import stamping test

bff248e

liucijus force-pushed the stamp-scala-import branch from 7a115d0 to bff248e Compare January 11, 2021 11:30

Vaidas Pilkauskas added 4 commits January 11, 2021 14:05

Use execpath instead of location

dda5965

Use timestamp normalization for reproducible stamping

93384c0

Add test to ensure Manifest attributes are not removed

58df994

Use custom ijar for stamping

51b1bc3

Vaidas Pilkauskas added 2 commits January 13, 2021 09:31

Add comments regarding ijar bug

aa4a413

Fix default label and make ijar implicit dep

427d207

liucijus marked this pull request as ready for review January 13, 2021 07:37

simuons reviewed Jan 22, 2021

View reviewed changes

third_party/java_tools/ijar/BUILD

],

)

#

#cc_binary(

Copy link

Collaborator

simuons Jan 22, 2021

Choose a reason for hiding this comment

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

Maybe remove this?

simuons reviewed Jan 22, 2021

View reviewed changes

simuons approved these changes Jan 22, 2021

View reviewed changes

ittaiz approved these changes Jan 22, 2021

View reviewed changes

ittaiz merged commit 939fa4c into bazelbuild:master Jan 22, 2021

liucijus mentioned this pull request Jan 25, 2021

How to debug "weird" error messages #553

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Stamp scala_import jars #1160

Stamp scala_import jars #1160

liucijus commented Dec 4, 2020

liucijus commented Dec 9, 2020

ittaiz left a comment

ittaiz Dec 9, 2020

liucijus Jan 11, 2021

liucijus commented Jan 13, 2021 •

edited

Loading

ittaiz commented Jan 13, 2021

liucijus commented Jan 13, 2021

ittaiz commented Jan 13, 2021

simuons Jan 22, 2021

liucijus Jan 22, 2021

simuons Jan 22, 2021

simuons Jan 22, 2021

liucijus Jan 22, 2021

simuons left a comment

liucijus commented Jan 22, 2021

simuons commented Jan 22, 2021

ittaiz left a comment

Stamp scala_import jars #1160

Stamp scala_import jars #1160

Conversation

liucijus commented Dec 4, 2020

liucijus commented Dec 9, 2020

ittaiz left a comment

Choose a reason for hiding this comment

ittaiz Dec 9, 2020

Choose a reason for hiding this comment

liucijus Jan 11, 2021

Choose a reason for hiding this comment

liucijus commented Jan 13, 2021 • edited Loading

ittaiz commented Jan 13, 2021

liucijus commented Jan 13, 2021

ittaiz commented Jan 13, 2021

simuons Jan 22, 2021

Choose a reason for hiding this comment

liucijus Jan 22, 2021

Choose a reason for hiding this comment

simuons Jan 22, 2021

Choose a reason for hiding this comment

simuons Jan 22, 2021

Choose a reason for hiding this comment

liucijus Jan 22, 2021

Choose a reason for hiding this comment

simuons left a comment

Choose a reason for hiding this comment

liucijus commented Jan 22, 2021

simuons commented Jan 22, 2021

ittaiz left a comment

Choose a reason for hiding this comment

liucijus commented Jan 13, 2021 •

edited

Loading