Should scala_import return jar output file provider (in addition to java_common provider)?

[natansil]

During work on this PR I've noticed that location expansion of jars exposed by scala_import fails:
For example
$(location @com_google_guava_guava_21_0//jar) fails with label '@com_google_guava_guava_21_0//jar:jar' in $(location) expression expands to no files. when the jar target is of type rules_scala

So it seems that scala_import is missing a feature for exposing jar output files.

@lberki are bazel rules idiomatically expected to output files?

[lberki]

$(location <target>) expands to the files to build (i.e. DefaultInfo(files=) of the rule in question. Apparently, scala_import doesn't have that?

[natansil]

@lberki
It does not have it explicitly,
From the documentation it seems that scala_import should have it implicitly, no?

[lberki]

Sorry for being vague; What I meant is that scala_import probably doesn't put the right files into the files field of DefaultInfo.

[natansil]

@lberki,
Let me see if I understand,

This is the relevant code from scala_import:

def _scala_import_impl(ctx):
...
    return struct(
        scala = struct(
          outputs = struct (
              jars = intellij_metadata
          ),
        ),
        jars_to_labels = jars2labels,
        providers = [
            _create_provider(current_jars, transitive_runtime_jars, jars, exports)
        ],
    )

def _create_provider(current_target_compile_jars, transitive_runtime_jars, jars, exports):
  test_provider = java_common.create_provider()
  if hasattr(test_provider, "full_compile_jars"):
      return java_common.create_provider(
          use_ijar = False,
          compile_time_jars = depset(transitive = [current_target_compile_jars, exports.compile_jars]),
          transitive_compile_time_jars = depset(transitive = [jars.transitive_compile_jars, current_target_compile_jars, exports.transitive_compile_jars]) ,
          transitive_runtime_jars = depset(transitive = [transitive_runtime_jars, jars.transitive_runtime_jars, current_target_compile_jars, exports.transitive_runtime_jars]) ,
      )
  else:
      return java_common.create_provider(
          compile_time_jars = current_target_compile_jars,
          runtime_jars = transitive_runtime_jars + jars.transitive_runtime_jars,
          transitive_compile_time_jars = jars.transitive_compile_jars + current_target_compile_jars,
          transitive_runtime_jars = transitive_runtime_jars + jars.transitive_runtime_jars + current_target_compile_jars,
      )

Do you suggest to return another provider (DefaultInfo) in addition to java_common, and assign the jars to the its files field?

[lberki]

Yep. That would also help with Bazel doing something useful if e.g. a scala_import rule ends up in the srcs of a genrule. (i.e. then the imported jars would be there)

[jjudd]

I ran into this issue and noticed #477 to refactor scala_import. I opened a PR to #477's branch to add a DefaultInfo with files: https://github.com/andyscott/rules_scala/pull/1

There's no test. I can add one. I'm interested in getting people's feedback, considering I'm not very familiar with DefaultInfo and that the documentation is a bit sparse.

[ittaiz]

Do you have a problem this would solve? If not let’s please not add this to 477 which is already a hard PR to review due to the rewrite

…

On Fri, 13 Apr 2018 at 6:24 James Judd ***@***.***> wrote: I ran into this issue and noticed #477 <#477> to refactor scala_import. I opened a PR to #477 <#477 branch branch to add a DefaultInfo with files: andyscott#1 <https://github.com/andyscott/rules_scala/pull/1> There's no test. I can add one. I'm interested in getting people's feedback, considering I'm not very familiar with DefaultInfo and that the documentation is a bit sparse. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#476 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIF8NcAkesf67bAPG97vKVJ_cqMVrPks5toBqEgaJpZM4TMh9C> .

[jjudd]

I perhaps incorrectly assumed that #477 was close to being merged, so I added a PR there in an effort to avoid merge conflicts. I did not intend to make things more difficult :)

I'm happy to close my PR to #477 and wait to open another PR for this issue until #477 is merged.

It does solve a problem for us, but I have this fix on our fork, so I have no problem with waiting.

[natansil]

Thank you @jjudd for writing this PR.
This change probably means that com_google_guava_guava_21_0_with_file can be removed in favor of com_google_guava_guava_21_0.

It would be nice to remove this duplication