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

repository_ctx is unable to get another repository_rule's attributes #1704

Closed
jin opened this issue Aug 30, 2016 · 10 comments
Closed

repository_ctx is unable to get another repository_rule's attributes #1704

jin opened this issue Aug 30, 2016 · 10 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) type: documentation (cleanup)

Comments

@jin
Copy link
Member

jin commented Aug 30, 2016

I'm currently trying to implement a couple of repository_rule rules in Skylark.

My maven_rules.bzl file looks like this:

def maven_jar_impl(repo_ctx):
   ...

maven_jar_attrs = { 
  server: attr.label(), # refers to a maven_server target
  ... 
}

maven_jar = repository_rule(
  maven_jar_impl, 
  attrs=maven_jar_attrs, 
  ...
)

def maven_server_impl(repo_ctx):
  ...

maven_server_attr = { 
  url=attr.string(),
  ...
}

maven_server = repository_rule(
  maven_server_impl,
  attrs=maven_server_attr,
  ...
)

How do you retrieve the url attribute of the referenced maven_server target within maven_jar_impl, hopefully via repo_ctx? I've looked around and found native.existing_rules() and native.existing_rule(name), but they do not work in the analysis phase.

@damienmg
Copy link
Contributor

There is no way to do that currently.

@damienmg damienmg added type: feature request P2 We'll consider working on this in future. (Assignee optional) and removed type: documentation (cleanup) labels Aug 30, 2016
@damienmg damienmg added this to the 0.6 milestone Aug 30, 2016
@damienmg
Copy link
Contributor

Oh sorry, it should be possible. native.existing_rule should just work but only at the macros level, so you can wrap maven_jar as a macro that calls the real maven_jar passing along the url to the actual maven_jar

@damienmg
Copy link
Contributor

damienmg commented Aug 30, 2016

I will leave it as a documentation issue.

@kchodorow
Copy link
Contributor

you can wrap maven_jar as a macro that calls the real maven_jar passing along the url to the actual maven_jar

What?

@jin
Copy link
Member Author

jin commented Aug 30, 2016

I got this to work:

In WORKSPACE

load(":maven_rules.bzl", "maven_jar_macro")

# this has to come before maven_jar_macro declarations
maven_server( 
    name = "maven_uk_server",
    url = "http://uk.maven.org/maven2"
)

maven_jar_macro(
    name = "com_google_guava_guava",
    artifact = "com.google.guava:guava:18.0",
    sha1 = "cce0823396aa693798f8882e64213b1772032b09",
    repository = "http://repo.exist.com/maven2",
    server = "maven_uk_server"
)

maven_rules.bzl

def maven_jar_macro(name, artifact, sha1, server, repository, visibility=None):
  print(native.existing_rule(server)["url"]) # this prints "http://uk.maven.org/maven2"
  maven_jar( # maven_jar is the actual repository_rule
      name = name,
      visibility = visibility,
      artifact = artifact,
      sha1 = sha1,
      repository = repository
  )

However, the maven_server rule now needs to come before the maven_jar_macro rule for it to be included in native.existing_rules. This makes the WORKSPACE file rules positional.

@jin
Copy link
Member Author

jin commented Aug 30, 2016

@damienmg Why is native.existing_rules available in the macros level but not during WORKSPACE analysis? Wouldn't the rules and macros be parsed into existing_rules dictionary prior to WORKSPACE analysis?

@damienmg
Copy link
Contributor

something along this should work:

maven_server = repository_rule(...)
_maven_jar = repository_rule(... attrs = { "server_url": attr.string(),  ... } ...)

def maven_jar(server=None, **kwargs):
  if server:
    server_rule = native.existing_rule(server)
    server_url = server_rule["url"]
  else:
    server_url = "repo.maven.apache.org"
  _maven_jar(server = server_url, **kwargs)

@damienmg
Copy link
Contributor

Ok cross-post. So seems like @jin understood what I meant :)

This is how skylark is made. It was not necessary to access them from other rule before. We could probably add it to repository_ctx. This need to be thought through though.

@jin
Copy link
Member Author

jin commented Aug 30, 2016

It still feels a little fragile and not obvious that the maven_server declaration needs to come before maven_jar.

What are the implications of allowing native.existing_rules() to be called during the analysis phase?

jin added a commit to jin/bazel that referenced this issue Sep 8, 2016
This is an initial implementation of the maven_jar rule in Skylark,
targeted at the FRs in issue bazelbuild#1410.

Attributes `name`, `artifact`, `repository`, `sha1` have been
implemented, but not `server`.

This implementation uses `wget` as the underlying fetch mechanism for remote
artifacts to simplify dependencies. My original implementation made use
of an underlying call to the `mvn` binary and its `dependency:get`
plugin, but it brought along complexities with parsing `pom.xml` files
and creating local repositories within Bazel's cache. My personal
opinion here is that it's easier to build up from `wget` than to trim
down complexities when using `mvn`.

With regards to server, there are some limitations with retrieving a
maven_server's attribute at Loading Phase without the use of hacky macros
(issue bazelbuild#1704), and even if macros are used, the maven_server is not treated
as an actual dependency by maven_jar, hence it will not get analyzed during
Analysis Phase. There is a test (`test_unimplemented_server_attr`) to
ensure that the error message to shown to users if they use the server
attribute with this rule.

I will have to put more work into implementing maven_server
appropriately, and possibly proposing an API review of maven_jar in
another change set.

Change-Id: I64166dd251d5d268b525dc219cef424a5b5534a1
jin added a commit to jin/bazel that referenced this issue Sep 15, 2016
This is an initial implementation of the maven_jar rule in Skylark, targeted at
the FRs in issue bazelbuild#1410.

Attributes `name`, `artifact`, `repository`, `sha1` have been implemented, but
not `server`.

Implemented a wrapper around the maven binary to pull dependencies from
remote repositories into a directory under {output_base}/external.

Caveat: this rule assumes that the Maven dependency is installed in the
system. Hence, the maven_skylark_test integration tests are tagged with
"manual" because the Bazel CI isn't configured with the Maven binary
yet.

Added a serve_not_found helper for 404 response tests.

Added a `maven_local_repository` rule to fetch the initial maven
dependency plugin for bazel tests.

With regards to server, there are some limitations with retrieving a
maven_server's attribute at Loading Phase without the use of hacky macros
(issue bazelbuild#1704), and even if macros are used, the maven_server is not treated as
an actual dependency by maven_jar. There is a test
(`test_unimplemented_server_attr`) to ensure that the error message to shown to
users if they use the server attribute with this rule.

I will have to put more work into implementing maven_server appropriately, and
possibly proposing an API review of maven_jar in another change set.

Change-Id: I167f9d13835c30be971928b4cc60167a8e396893
bazel-io pushed a commit that referenced this issue Sep 23, 2016
**Experimental**

This is an initial implementation of the maven_jar rule in Skylark, targeted at
the FRs in issue #1410.

Implemented a wrapper around the maven binary to pull dependencies from
remote repositories into a directory under {output_base}/external.

Attributes `name`, `artifact`, `repository`, `sha1` have been implemented,
but not `server`.

Caveat: this rule assumes that the Maven dependency is installed in the
system. Hence, the maven_skylark_test integration tests are tagged with
"manual" and commented out because the Bazel CI isn't configured with
the Maven binary yet.

Added a serve_not_found helper for 404 response tests.

Usage:

```
load("@bazel_tools//tools/build_defs/repo:maven_rules.bzl", "maven_jar")

maven_jar(
    name = "com_google_guava_guava",
    artifact = "com.google.guava:guava:18.0",
    sha1 = "cce0823396aa693798f8882e64213b1772032b09",
    repository = "http://uk.maven.org/maven2",
)
```

With regards to server, there are some limitations with retrieving a
maven_server's attribute at Loading Phase without the use of hacky macros
(issue #1704), and even if macros are used, the maven_server is not treated as
an actual dependency by maven_jar. There is a test (`test_unimplemented_server_attr`)
to ensure that the error message to shown to users if they use the server
attribute with this rule.

--
Change-Id: I167f9d13835c30be971928b4cc60167a8e396893
Reviewed-on: https://bazel-review.googlesource.com/c/5770
MOS_MIGRATED_REVID=133971809
@damienmg
Copy link
Contributor

damienmg commented Mar 7, 2017

There is no real action we can take on this one, closing.

@damienmg damienmg closed this as completed Mar 7, 2017
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) type: documentation (cleanup)
Projects
None yet
Development

No branches or pull requests

4 participants