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.

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

using Classpath shrinker to warn on indirect targets #1

Closed
wants to merge 24 commits into from

Conversation

nadavwe
Copy link

@nadavwe nadavwe commented Jun 28, 2017

this is written in top of bazelbuild#227.
@natansil - we should probably rebase on master.
this still fails - we have something unexpected regarding the output targets, need to investigate.

@ittaiz @or-shachar

Stephen Twigg and others added 3 commits June 19, 2017 17:16
The key change is that the scala provider has been completely
restructured to match the structure of the JavaProvider. It no longer
tracks exports separately but instead has the following fields:

* outputs = contains all the outputs generated by the current rule
(nothing or ijar and class_jar); however, no rules here actually use it.

* compile_jars = contains all the jars dependent rules should compile
with. Thus, ijar and exports.compile_jars

* transitive_runtime_jars = contains all the jars needed to handle this
target at runtime. Thus, class_jar plus (deps + exports +
runtime_deps).transitive_runtime_jars

The created java provider (used by dependent native java rules) uses
just compile_jars and transitive_runtime_jars.

In general, this change was seamless. The major exception is if you were
specifically relying on only exports being re-exported by specifically
gen_scrooge_srcjar. Beforehand, any jars produced by direct dependencies
were not being exported to dependents but now they are. Note that, if
you were using scrooge_scala_library instead, nothing should change.

Other changes:
* Use depset instead of set. (set docs already say set should be
considered deprecated and just points to depset anyway)

* Tests amended to exploit that JavaProvider is properly constructed.
Note that things are still a little strange until Bazel releases
incorporate some fixes to JavaProvider and native provider rules.
Generally, only deps for java_library and java_binary work at the
moment.
Remove scala_exports_to_java
Prefer using JavaProvider over scala provider
  (can probably avoid using scala provider completely?)
@nadavwe
Copy link
Author

nadavwe commented Jun 28, 2017

relates to bazelbuild#235

@natansil
Copy link

@nadavwe , there were two bugs related to string encoding/escaping.
first, we encoded the jars and targets with ":" instead of "," to the temp file read by the worker.
second, specifically for targets, they contain ":", themselves, so they needed to be escaped...

both are fixed now

@ittaiz
Copy link

ittaiz commented Jun 29, 2017 via email

@nadavwe
Copy link
Author

nadavwe commented Jun 29, 2017

@ittaiz

  1. please do - i think this is a bit too POC code yet - it should have major beautification refactoring.
  2. please review in the other repository anyway and we can decide on copying later.

@ittaiz
Copy link

ittaiz commented Jun 29, 2017 via email

Copy link

@ittaiz ittaiz left a comment

Choose a reason for hiding this comment

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

falling asleep. didn't finish. didn't want you to be blocked in the meanwhile

@@ -0,0 +1 @@
/Users/natans/Work/rules_scala/bazel-genfiles/external/scala/_ijar/scala-compiler/external/scala/lib/scala-compiler-ijar.jar:/Users/natans/Work/rules_scala/bazel-genfiles/external/scala/_ijar/scala-library/external/scala/lib/scala-library-ijar.jar
Copy link

Choose a reason for hiding this comment

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

why do you need this? only for tests? take a look at $location expansion on bazel.build

| org.apache.commons.lang3.ArrayUtils.EMPTY_BOOLEAN_ARRAY.length
|}
""".stripMargin
val commonsPath = Coursier.getArtifact(commons)
Copy link

Choose a reason for hiding this comment

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

this looks like a really problematic pattern for a bazel build. why not have "commons" in the deps?

deps = ["//main:dependency_analyzer",
"@io_get_coursier_coursier_cache//jar",
"@io_get_coursier_coursier//jar",
"@org_scalaz_scalaz_concurrent_2_11//jar",
Copy link

Choose a reason for hiding this comment

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

why do you need this? also, why do you need the scalaz stuff?

natansil and others added 19 commits July 3, 2017 12:42
instead of overloading jvm_flags for both scalac and executable (breaking change)
* Restructure scala provider, add JavaProvider

The key change is that the scala provider has been completely
restructured to match the structure of the JavaProvider. It no longer
tracks exports separately but instead has the following fields:

* outputs = contains all the outputs generated by the current rule
(nothing or ijar and class_jar); however, no rules here actually use it.

* compile_jars = contains all the jars dependent rules should compile
with. Thus, ijar and exports.compile_jars

* transitive_runtime_jars = contains all the jars needed to handle this
target at runtime. Thus, class_jar plus (deps + exports +
runtime_deps).transitive_runtime_jars

The created java provider (used by dependent native java rules) uses
just compile_jars and transitive_runtime_jars.

In general, this change was seamless. The major exception is if you were
specifically relying on only exports being re-exported by specifically
gen_scrooge_srcjar. Beforehand, any jars produced by direct dependencies
were not being exported to dependents but now they are. Note that, if
you were using scrooge_scala_library instead, nothing should change.

Other changes:
* Use depset instead of set. (set docs already say set should be
considered deprecated and just points to depset anyway)

* Tests amended to exploit that JavaProvider is properly constructed.
Note that things are still a little strange until Bazel releases
incorporate some fixes to JavaProvider and native provider rules.
Generally, only deps for java_library and java_binary work at the
moment.

* Using JavaProvider, requires Bazel 0.5.2

Remove scala_exports_to_java
Prefer using JavaProvider over scala provider
  (can probably avoid using scala provider completely?)

* update minimum bazel version to 0.5.2
note we need 0.5.2 now.
* don't put signatures in fat jar

* Add tests
The key change is that the scala provider has been completely
restructured to match the structure of the JavaProvider. It no longer
tracks exports separately but instead has the following fields:

* outputs = contains all the outputs generated by the current rule
(nothing or ijar and class_jar); however, no rules here actually use it.

* compile_jars = contains all the jars dependent rules should compile
with. Thus, ijar and exports.compile_jars

* transitive_runtime_jars = contains all the jars needed to handle this
target at runtime. Thus, class_jar plus (deps + exports +
runtime_deps).transitive_runtime_jars

The created java provider (used by dependent native java rules) uses
just compile_jars and transitive_runtime_jars.

In general, this change was seamless. The major exception is if you were
specifically relying on only exports being re-exported by specifically
gen_scrooge_srcjar. Beforehand, any jars produced by direct dependencies
were not being exported to dependents but now they are. Note that, if
you were using scrooge_scala_library instead, nothing should change.

Other changes:
* Use depset instead of set. (set docs already say set should be
considered deprecated and just points to depset anyway)

* Tests amended to exploit that JavaProvider is properly constructed.
Note that things are still a little strange until Bazel releases
incorporate some fixes to JavaProvider and native provider rules.
Generally, only deps for java_library and java_binary work at the
moment.
@nadavwe
Copy link
Author

nadavwe commented Aug 13, 2017

changes were merged to upstream in bazelbuild#243.

@nadavwe nadavwe closed this Aug 13, 2017
@ittaiz ittaiz deleted the classpath-shrinker branch August 22, 2017 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants