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

Make transitive dependencies available to Skylark - Make native provider available in Skylark #146

Closed
ekuefler opened this issue Apr 21, 2015 · 20 comments
Assignees
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: feature request

Comments

@ekuefler
Copy link
Contributor

Skylark currently makes its direct dependencies available via ctx.files.deps, but doesn't provide any way to get the classpath or full set of transitive dependencies required by the target. This information is critical for integrating with any javac-like tool that requires specifying a classpath, such as GWT or Groovy compilers.

The only workaround is to force these rules to specify all of their transitive dependencies in the BUILD file. This is very cumbersome and sometimes impossible, since transitive dependencies might not actually be visible to the target that needs them.

@laszlocsomor laszlocsomor assigned laurentlb and damienmg and unassigned laurentlb Apr 21, 2015
@laszlocsomor
Copy link
Contributor

The way to propagate data about the transitive closure is by using TransitiveInfoProviders. In Skylark these are just called providers.

I'm certain there's some way to get the classpath from the whole transitive closure because I think we do that in the java rules. Unfortunately I don't know how :( Summoning Damien to answer.

@laszlocsomor laszlocsomor added question P2 We'll consider working on this in future. (Assignee optional) labels Apr 21, 2015
@damienmg
Copy link
Contributor

Ok there is no way to do it nicely:
See https://github.com/google/bazel/blob/master/tools/build_rules/java_rules_skylark.bzl#L37 on how to do it.

Re-assigning to laurentlb for skylark feature.

@damienmg damienmg assigned laurentlb and unassigned damienmg Apr 21, 2015
@laurentlb
Copy link
Contributor

Each rule has to expose its providers to Skylark. So this should be implemented by rule owners.

@schroederc
Copy link
Contributor

Can we rename this issue since its topic has changed?

My issue was specifically about adding Skylark providers to the {java,cxx}_* native rules. This is needed to have proper support for Go's cgo, other FFIs, and the language analysis rules in Kythe.

@damienmg damienmg changed the title Make transitive dependencies available to Skylark Make transitive dependencies available to Skylark - Make native provider available externally May 18, 2015
@damienmg damienmg changed the title Make transitive dependencies available to Skylark - Make native provider available externally Make transitive dependencies available to Skylark - Make native provider available in Skylark May 18, 2015
@damienmg
Copy link
Contributor

Done. Please list the providers you would like to have access to so we can use this bug to track the list of provider to add.

@schroederc
Copy link
Contributor

From cc_library rules, we would need at least the CcLinkParams and CppCompilationContext.

@ekuefler
Copy link
Contributor Author

For java_library we definitely need at least the following:

Though I suspect several of the other providers in that package would be useful in general also.

Is there an example somewhere of a rule exposing providers to Skylark? If I can find an example I'd be happy to make equivalent changes for the Java rules.

@laurentlb
Copy link
Contributor

Some information is here:
https://groups.google.com/d/msg/bazel-discuss/SzCNb0kV_Sw/tJz3XxiUbTgJ

More specifically, for each of the files, add the annotation on the class:
@SkylarkModule(name = "whatever", doc = "")

Add an annotation on each method:
@SkylarkCallable(name = "name_of_the_field", doc = "", structField = true)

Access the providers:
p = provider(ctx.attr.my_java_label, "rules.java.JavaSourceJarsProvider")
p.name_of_the_field

Of course, this is experimental. We'll probably provide a nicer syntax in the future.

@schroederc
Copy link
Contributor

What would it take to get these annotations committed the master branch? These changes seem pretty simple to make, but I'd like to stay away from using a custom Bazel binary. I'm fine with using experimental features that may change, however.

@laurentlb
Copy link
Contributor

I personally don't know how the Java rules work, and I don't know which methods make or don't make sense to be exported. Also, it's maybe better to clean the API before making it public (it will be much more complicated later). And they need to be documented.

What you can do is to make this local change. Expose everything and try to see what is valuable. I can make public the functions that have an actual use-case.

@schroederc
Copy link
Contributor

The information exposed in https://github.com/schroederc/bazel/commit/9619db3b72df502836c42905b923a62a54f7fa2c is valuable to Kythe.

@ekuefler
Copy link
Contributor Author

I tried a similar change in https://github.com/ekuefler/bazel/commit/f1a737314aabed706d4f561d394c51fdc01e6317 to expose classpaths for the Groovy compiler. Accessing the provider works, but I run into an interesting problem:

java.lang.ClassFormatError: Absent Code attribute in method that is not native or abstract in class file org/codehaus/groovy/tools/FileSystemCompiler
    at java.lang.ClassLoader.defineClass1(Native Method)
    at java.lang.ClassLoader.defineClass(ClassLoader.java:760)
    at java.security.SecureClassLoader.defineClass(SecureClassLoader.java:142)
    at java.net.URLClassLoader.defineClass(URLClassLoader.java:455)
    at java.net.URLClassLoader.access$100(URLClassLoader.java:73)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:367)
    at java.net.URLClassLoader$1.run(URLClassLoader.java:361)
    at java.security.AccessController.doPrivileged(Native Method)
    at java.net.URLClassLoader.findClass(URLClassLoader.java:360)
    at org.codehaus.groovy.tools.RootLoader.oldFindClass(RootLoader.java:171)
    at org.codehaus.groovy.tools.RootLoader.loadClass(RootLoader.java:143)
    at java.lang.ClassLoader.loadClass(ClassLoader.java:357)
    at org.codehaus.groovy.tools.GroovyStarter.rootLoader(GroovyStarter.java:96)
    at org.codehaus.groovy.tools.GroovyStarter.main(GroovyStarter.java:128)

I'm guessing this is an ijar issue. JavaCompilationArgs.getCompileTimeJars seems to give me mostly ijars back. I assume this is what bazel uses on its javac classpath though, so I'm not sure why it would work there and not here since groovyc is just going through a standard java classloader. Is there a method somewhere in Java that gives me full jars instead of ijars?

@ekuefler
Copy link
Contributor Author

Also tried this for Java source jars and confirmed that it does what I want for GWT. Sent a PR with the changes - not sure if you want to do this or wait for syntactic changes in Skylark first.

@damienmg
Copy link
Contributor

damienmg commented Jun 1, 2015

Just a quick reviews of the Java providers that were asked. IMO they all makes sense and we should definitely accept change that exposes the one asked in that thread. We definitely also want the runtime classpath provider to be exposed (it is currently exposed but not documented).

For the ijars issue, you might want to use the runtime classpath which contains non-ijar jar. Though it might miss some "neverlink" dependencies.

@abergmeier
Copy link
Contributor

Can I vote for CcNativeLibraryProvider.java?
Currently I actually would like to have a CcIncludesProvider, though.

@bsilver8192
Copy link
Contributor

@abergmeier : Are you looking for something different than transitive_headers in CcSkylarkApiProvider.java? The information exposed for C++ isn't complete, but there is a fair amount of it.

@abergmeier
Copy link
Contributor

abergmeier commented Dec 1, 2016

@bsilver8192 Ah thanks, I kinda missed that.
I am looking to build flatbuffers with Bazel. For that I need to (in a rule):

  • generate .hpp files from .fbs
  • pass include directories to dependant
  • pass header files to dependant

Filed #2163 for that.

@calder
Copy link

calder commented May 15, 2018

PythonImportsProvider would be great. More generally, is there any reason for exposing native providers only upon request rather than just exposing them all now?

@hlopko hlopko assigned oquenchil and unassigned hlopko May 16, 2018
@laurentlb
Copy link
Contributor

Yes. Providers should be reviewed / cleaned up before we expose them. Once a provider is exposed, it's hard to modify it and make it evolve, so we should be comfortable supporting it.

@hlopko
Copy link
Member

hlopko commented Jan 11, 2019

This is fixed now by @oquenchil's work on #4570.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Rules-CPP Issues for C++ rules type: feature request
Projects
None yet
Development

No branches or pull requests