Work In Progress: Quick prototype of a cargo_crate repository rule #100

damienmg · 2018-06-14T21:49:04Z

This rule would allow cargo raze to just output a bzl file in remote mode
and then rely on the rule to generate the correct BUILD files.

An exemple of rule that cargo raze could generate would looks like:

cargo_crate(
  name = "ansi_term",  # The crate name
  version = "0.11.0",  # The crate version
  # List of resolved dependency version.
  # If a dependency is needed by the library and absent from this list
  # the cargo_crate rule will emit a warning and not add the dependency in
  # the build rule, making it potentially unable to compile.
  locked_deps = {"winapi": "0.3.5"},
  # Other optional attributes:
  #  - flags: contains a list of rustc flags to add to the rust_{library,binary} rules.
  #  - data: contains the data attribute to propagate to the rust_{library,binary} rules.
  #  - sha256: SHA-256 checksum of the crate, recommended since it enhance Bazel caching.
)

To-do:

Stabilize API
Document
Add automated tests

Fixes #2

This change add a 'triplet' information to the toolchain that is then used to specify the target platform to Rust.

This rule would allow cargo raze to just output a bzl file in remote mode and then rely on the rule to generate the correct BUILD files. An exemple of rule that cargo raze could generate would looks like: ```python cargo_crate( name = "ansi_term", # The crate name version = "0.11.0", # The crate version # List of resolved dependency version. # If a dependency is needed by the library and absent from this list # the cargo_crate rule will emit a warning and not add the dependency in # the build rule, making it potentially unable to compile. locked_deps = {"winapi": "0.3.5"}, # Other optional attributes: # - flags: contains a list of rustc flags to add to the rust_{library,binary} rules. # - data: contains the data attribute to propagate to the rust_{library,binary} rules. # - sha256: SHA-256 checksum of the crate, recommended since it enhance Bazel caching. ) ``` To-do: [ ] Stabilize API [ ] Document [ ] Add automated tests

acmcarther · 2018-06-14T22:36:14Z

rust/crate_repository.bzl

+
+crate_repository = repository_rule(
+    _impl,
+    attrs = {


Nit: Consider including the toolchain or target triple as well to be asserted against

flags and locked_deps are target-specific.

Do you mean to have flags & locked_deps being selectable by platform? I don't really see where the target triple can be injected here. That's why I set the target triple in the toolchain rather than trying to inject the --target flag in the BUILD file.

Note that AFAIK there is no way to get the toolchain at this stage since it is not resolved yet when we are doing repository loading.

I initially assumed that these would be target specific repository rules since the attrs are target-specific. Presumably multiple resolved trees of crates could avoid bringing in the same crate twice using the native.existing_rule mechanism you added in another PR. It isn't clear to me what happens with the BUILD files though...

It's possible that instead the target switching could be baked into the attrs somehow though. This provides a more intuitive interface at the cost of some additional complexity.

acmcarther · 2018-06-14T22:38:39Z

rust/crate_repository.bzl

+    if result.stderr:
+        print(result.stderr)
+
+crate_repository = repository_rule(


I'm not totally clear on this -- Does this need to retain some mechanism to override a dep (e.g. instead of using some generated build target dep, accept a target directly)?

No, that something I need to add to that PR.

acmcarther · 2018-06-14T22:46:39Z

rust/cargo_manifest_to_build.py

+if __name__ == "__main__":
+    json = json.load(StringIO.StringIO(subprocess.check_output([
+        "cargo",
+        "metadata",


This invocation strikes me as "fishy". You're largely using locally generated crate metadata, and expecting it to match what cargo-raze used to generate the exact set of dependencies.

In isolation, you cannot trust the metadata emitted by cargo metadata -- you need to run this for the entire workspace and then select the metadata for a single crate.

Sorry I fail to follow here. Each crate declare its direct deps and we rely on external source to provide us the locked version, why would this invocation be wrong? I meant it barely exposes the information in the Cargo.toml file in a JSON file, it does not do any resolution.

The locked crate version isn't the only piece of non-local state that needs to be brought in from external sources. You also need to bring in the set of features wanted by all upstream entities.

Consider a scenario where you have the following dependency chain:
A -(wants)-> B -> C

They use the default feature set. Now, consider a new crate D such that
A -> D -> C
... Where D adds a feature directive to it's C where ["with_serde"] is a wanted C feature.

This is what I called a "non-local" effect -- this new feature flag must also be provided to the C repository rule declaration... with the addition of the new locked serde deps.

As a final wrench to throw in -- this can happen for some targets and not others if C is a [target.*.dependencies] target-specific dependency of D.

acmcarther · 2018-06-14T22:47:27Z

Do you want to have a higher bandwidth discussion about this (GVC/Something else)?

I suspect you're stepping into a landmine here by assuming that metadata will be present that won't be, or that you can run these cargo commands independently on a per crate basis.

acmcarther

Last two I think.

It's taking me a while to wrap my head around this and the interaction that it is supposed to have with cargo-raze. This is complicated stuff (because Cargo is complicated)!

I think it's largely workable, but we need to be very confident about what data is available here versus what needs to be provided by the caller (because it cannot be known in isolation).

acmcarther · 2018-06-14T22:55:38Z

rust/cargo_manifest_to_build.py

+        data = "\n    data = r\"\"\"%s\"\"\"," % ctxt["data"]
+    for d in json["dependencies"]:
+        if d["name"] not in ctxt["resolved_deps"]:
+            sys.stderr.write("WARNING: Cannot resolve dependency on crate '%s'\n" % d["name"])


This isn't necessarily erroneous.

If cargo-raze determines that the given combination of features and target indicate that a dependency should be omitted, then it will not be present in the set of resolved deps.

EDIT s/a feature should/a dependency should/

Right maybe we should add a "n/a" value that explicitely says we should skip that deps. That way people that right themselves the target will get the warning if they omit a dep?

acmcarther · 2018-06-14T22:57:05Z

rust/cargo_manifest_to_build.py

+        "other": "%s%s" % (out_dir_tar, data),
+        "deps": resolve_deps([d for d in json["dependencies"] if not d["kind"]], ctxt),
+        "build_deps": resolve_deps([d for d in json["dependencies"] if d["kind"] == "build"], ctxt),
+        "features": json["features"].keys(),


Experimentally, I found that crate["features"] yielded from cargo metadata as described below is never going to have the required features, because they cannot be identified in isolation. I think they need to be supplied by the caller (of the repository_rule) on a per-target basis with the wider crate context in mind

This behavior was copied from cargo raze. We can definitely provide a way to override it from the attributes but it would diverge from the way cargo raze works.

There is a difference between what cargo raze does and what you're doing here. Cargo raze runs the metadata analysis once for the whole tree with all of the workspace context present. If I understand what you're doing here completely, you're running a similar process once per crate brought in.

damienmg · 2018-06-16T15:16:21Z

@acmcarther: getting on VC might be a bit difficult as I am in Munich, but feel free to ping me on hangout (my ldap is dmarting)

acmcarther · 2018-06-21T19:58:33Z

Discussion in rust-lang/cargo#5122 is topical regarding some of the nuances of depenencies and features as reported by cargo metadata.

damienmg · 2018-06-27T09:39:36Z

So some more though after yesterday VC:

Features propagation is really close in cargo to the feature_flag in Bazel as it can propagate upward but it doesn't seem you can declare a feature_flag attribute in Skylark at the moment. /cc @serynth: is there a plan to expose more feature flags to Skylark?
According to comment Emit Resolve.features_sorted in "cargo metadata" rust-lang/cargo#5122, they would love to move to a dependency resolution first then let the build phase handle feature related parts which would match Bazel model with the build is responsible for configuration transition and dependency resolution / fetching is a step that happens before that (except that Bazel download only a dependency if you actually needs it during the build, that's just an optimisation).

My view is that there should be minimal generated stuff to check in. At the moment with cargo-raze, there is a bunch of BUILD file whose content is, IIUC, entirely determined by the crate's Cargo.toml file. The only part that depends on reverse dependency is which feature to build (which is exactly the feature_flag) and for which we could generate X target from the cargo_crate rule.

I also find it nicer on the UX side to have most of the stuff baked into Bazel so the file cargo raze generate is still editable by hand afterward.

programmablereya · 2018-06-27T13:07:47Z

Re: feature_flags:
We'd like to expose more of the feature flag functionality to Skylark, but there's no specific plan at the moment. The current goal is to get feature flags a little more stable and reduce the amount of waste they produce before opening them up further.

That said, the ability to transmit information from target to dependencies (opposite the normal target to dependers direction) is more generally provided by configuration, of which feature flags are a limited subset. AFAIK, Skylark configuration is on its way, though I have no idea offhand what the plan is for that - @juliexxia might know better. Feature flags being more exposed to Skylark might happen along a similar timeline, as configuration is even more powerful than feature flags.

damienmg · 2018-10-26T13:48:37Z

I am going to close this one until feature flag are there, I don't think we gain much vs the current cargo raze until then.

UebelAndre · 2020-07-06T03:41:08Z

@damienmg any updates here?

damienmg added 2 commits June 14, 2018 23:26

Set the Rust target platform triplet using the toolchain description

aad5ce0

This change add a 'triplet' information to the toolchain that is then used to specify the target platform to Rust.

googlebot added the cla: yes label Jun 14, 2018

damienmg mentioned this pull request Jun 14, 2018

Produce a proposal for composable workspaces google/cargo-raze#52

Open

acmcarther mentioned this pull request Jun 14, 2018

Set the Rust target platform triplet using the toolchain description #99

Closed

acmcarther reviewed Jun 14, 2018

View reviewed changes

acmcarther mentioned this pull request Jun 21, 2018

CrateContext.features needs to be specialized using the target_triple google/cargo-raze#53

Open

damienmg closed this Oct 26, 2018

damienmg deleted the crate_repository branch November 10, 2018 19:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Work In Progress: Quick prototype of a cargo_crate repository rule #100

Work In Progress: Quick prototype of a cargo_crate repository rule #100

damienmg commented Jun 14, 2018 •

edited

Loading

acmcarther Jun 14, 2018

damienmg Jun 16, 2018

acmcarther Jun 17, 2018

acmcarther Jun 14, 2018

damienmg Jun 16, 2018

acmcarther Jun 14, 2018

damienmg Jun 16, 2018

acmcarther Jun 17, 2018 •

edited

Loading

acmcarther commented Jun 14, 2018

acmcarther left a comment

acmcarther Jun 14, 2018 •

edited

Loading

damienmg Jun 16, 2018

acmcarther Jun 14, 2018

damienmg Jun 16, 2018

acmcarther Jun 17, 2018

damienmg commented Jun 16, 2018

acmcarther commented Jun 21, 2018

damienmg commented Jun 27, 2018

programmablereya commented Jun 27, 2018

damienmg commented Oct 26, 2018

UebelAndre commented Jul 6, 2020

Work In Progress: Quick prototype of a cargo_crate repository rule #100

Work In Progress: Quick prototype of a cargo_crate repository rule #100

Conversation

damienmg commented Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmcarther Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

acmcarther commented Jun 14, 2018

acmcarther left a comment

Choose a reason for hiding this comment

acmcarther Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

damienmg commented Jun 16, 2018

acmcarther commented Jun 21, 2018

damienmg commented Jun 27, 2018

programmablereya commented Jun 27, 2018

damienmg commented Oct 26, 2018

UebelAndre commented Jul 6, 2020

damienmg commented Jun 14, 2018 •

edited

Loading

acmcarther Jun 17, 2018 •

edited

Loading

acmcarther Jun 14, 2018 •

edited

Loading