Deduplicate rule impls, fix rust_doc_test (#117) #122

mfarrugi · 2018-08-21T03:33:36Z

This is reasonable as is, but I will work through the doc rules as well.

Open to any and all nits, comments on naming, etc, as this is pretty subjective refactoring stuff.

Functional Changes:

runfiles is now populated for libraries, which should be meaningless
progress messages are a little different
rust_doc_test is no longer a noop :)

mfarrugi · 2018-08-21T03:36:10Z

rust/rustc.bzl

+    for dep in deps:
+        if hasattr(dep, "crate_info"):
+            # This dependency is a rust_library
+            rlib = dep.crate_info.output


this chunk is just a move, except for these crate_info usages

mfarrugi · 2018-08-21T03:37:39Z

rust/rustc.bzl

+
+    compile_inputs = (
+        crate_info.srcs +
+        ctx.files.data +


it feels a little weird for this to be in both compile_inputs and runfiles, but oh well

We'll need to come up with a real solution to runfiles -- probably related to including a crate in rules_rust for loading them.

I've said as much before (in different terms) but I suspect we need a crisper boundary between compile-time data and run-time data. The cc_embed_data approach described here: #79 (comment) makes sense to me for that purpose, and we could package a crate in rules_rust for accessing data packaged into an rlib in that manner.

Longer term thinking, of course....

Does the default 'data' attr imply runtime data? If so we can add compile_data in short order to differentiate, without dealing w/ embedding the data more directly

mfarrugi · 2018-08-21T03:40:31Z

Test failures in test/BUILD, forgot about that..

mfarrugi · 2018-08-21T15:08:06Z

rust/rust.bzl

    working_dir = "."
    depinfo = _setup_deps(
-        target.deps,
-        target.name,
+        [ctx.attr.dep],


This bit was pretty off, setup was wrong and none of it was being exercised because rustdoc wasn't being called with --test

mfarrugi · 2018-08-21T15:10:41Z

rust/rustc.bzl

+        else:
+            fail("rust targets can only depend on rust_library or cc_library targets.")
+
+    transitive_libs = depset([c.output for c in transitive_crates]) + transitive_staticlibs + transitive_dylibs


split up collecting deps / creating flags, hard to separate more though

mfarrugi · 2018-08-21T15:31:24Z

Looks like doc_test is missing some runfiles (at best), won't have time to fix this for a little bit.

Other than that this should be good to go, @acmcarther.

mfarrugi · 2018-08-22T02:06:22Z

@buchgr should I be able to see the failed test logs in the source.cloud.google.com ui? If not, and they are visible on the buildkite page, can I get access to that?

acmcarther

Some really light initial comments.

The refactoring here at least superficially looks kinda involved, so I want to give it a better look when I can find some time to sit down and do a review. Maybe 12 or 24 hours from now.

acmcarther · 2018-08-22T15:27:35Z

rust/rustc.bzl

+
+_setup_deps = setup_deps
+
+CrateInfo = provider(


Should this be hoisted out on top of the other functions? I don't feel strongly about this but it does feel hidden.

Is per-field documentation appropriate? I'm thinking specifically the "type" and "output" fields could use some extra detail

Sure, it sees enough usage; at some point it felt attached to the rustc_compile_action and I was just shy of splitting out rustc.bzl / rustdoc.bzl

Yeah, when this settles down I will add more documentation (though I miss static typing here too).

acmcarther · 2018-08-22T15:28:13Z

rust/rustc.bzl

+          will be symlinked into the .deps dir from the runfiles tree.
+
+    Returns:
+      Returns a struct containing the following fields:


Are all of these fields supposed to have an explanation?

I didn't touch any documentation since I consider this still in flux; In general I think some of these docs are boilerplatey though.

Oh my bad, ack.

acmcarther · 2018-08-22T15:32:21Z

test/conflicting_deps/second_crate/BUILD

@@ -7,6 +7,6 @@ load(

 rust_library(
    name = "example_name_conflict",
-    srcs = ["src/lib.rs"],
+    srcs = ["lib.rs"],


Side question: What are your personal feelings on ditching "src/" here? I've been thinking on how source files in rust sort of automatically "require" other local files to exist if they have module declarations, and I've been wondering if maybe we should take a more serious look at this at some point.

Initially, I was in a camp of "abandoning crate organization conventions", but I know repeatedly disregarding language conventions isn't really a convincing attitude, so I wanted to run through the thought exercise of what "embracing" those conventions might look like.

I'm interested to hear your thoughts, and we can move this into a proper issue if you'd like.

I have mostly been writing BUILD per dir, and avoiding the extra noise of src/ and main/ dirs.

I don't think src and main dirs work as well for company-sized or polyglot projects, but maven has precipitated a similar structure in enterprise java land. I have also seen enough crates set explicit values for lib / main paths that I don't feel guilty doing this.

acmcarther · 2018-08-23T15:55:13Z

rust/rust.bzl

    )
-    depinfo = _rust_test_common(ctx, test_binary)
+    info = _rust_test_common(ctx, bench_binary)


Nit: Only this "info" changed names? The others stayed as "depinfo"?

This is the rustc_compile_action struct output, not depinfo; I agree it is not particularly clear either way

acmcarther · 2018-08-23T16:00:12Z

rust/rustc.bzl

@@ -8,9 +8,9 @@ ZIP_PATH = "/usr/bin/zip"

 def _get_rustc_env(ctx):
    version = ctx.attr.version if hasattr(ctx.attr, "version") else "0.0.0"
-    v1, v2, v3 = version.split(".")
+    v1, v2, v3 = version.split(".", 2)


Nit: v1, v2, v3 : major_ver, minor_ver, patch_ver?

will change to major, minor, patch =

acmcarther · 2018-08-23T16:01:02Z

rust/rustc.bzl

+          will be symlinked into the .deps dir from the runfiles tree.
+
+    Returns:
+      Returns a struct containing the following fields:


Oh my bad, ack.

acmcarther · 2018-08-23T16:04:32Z

rust/rustc.bzl

+    deps_dir = working_dir + "/" + name + ".deps"
+    setup_cmd = ["rm -rf " + deps_dir + "; mkdir " + deps_dir + "\n"]
+    for lib in transitive_libs:
+        setup_cmd += [_create_setup_cmd(lib, deps_dir, in_runfiles)]


Nit: I know you didn't directly touch this here, but I think this is a good opportunity to rename this function. It only does one specific setup thing, and it might get called more than once which doesn't match the intuition derived from the function name.

Candidate: _(create_)?dep_symlink_cmd

acmcarther · 2018-08-23T16:09:02Z

rust/rustc.bzl

+
+    compile_inputs = (
+        crate_info.srcs +
+        ctx.files.data +


We'll need to come up with a real solution to runfiles -- probably related to including a crate in rules_rust for loading them.

I've said as much before (in different terms) but I suspect we need a crisper boundary between compile-time data and run-time data. The cc_embed_data approach described here: #79 (comment) makes sense to me for that purpose, and we could package a crate in rules_rust for accessing data packaged into an rlib in that manner.

Longer term thinking, of course....

acmcarther · 2018-08-23T16:14:25Z

rust/rust.bzl

-            crate_root = dep.crate_root,
-            crate_type = crate_type,
-            deps = dep.rust_deps,
+        crate = ctx.attr.deps[0].crate_info


Nit: crate -> parent_crate

Just to better describe the relationship between this rust_test rule and the rust_library that it wraps.

acmcarther · 2018-08-23T16:17:59Z

rust/rustc.bzl

+
+    # nb. Crates are linked via --extern regardless of their crate_type
+    link_flags += ["--extern " + crate.name + "=" + deps_dir + "/" + crate.output.basename for crate in direct_crates]
+    link_flags += ["-l dylib=" + _get_lib_name(lib) for lib in transitive_dylibs.to_list()]


Seems kind of a shame that we can't get rid of the _get_lib_name function and just use CrateInfo, but it makes sense that we might still need it for non-rust deps.

acmcarther · 2018-08-23T16:19:13Z

test/BUILD

-    # TODO(katre): Remove the fork of test_rules when the fixed version is released.
-    #"@bazel_tools//tools/build_rules:test_rules.bzl",
-    ":test_rules.bzl",
+    "@bazel_tools//tools/build_rules:test_rules.bzl",


Thanks for fixing this!

mfarrugi · 2018-08-23T21:48:18Z

I'll go ahead and skip the tests failing REB, pending being able to see the logs at all, and tidy the rest.

mfarrugi · 2018-08-25T15:21:51Z

rust/rustc.bzl

        _out_dir_setup_cmd(ctx.file.out_dir_tar) +
-        _get_rustc_env(ctx) + [
-            "LD_LIBRARY_PATH=%s" % _get_path_str(_get_dir_names(toolchain.rustc_lib)),


@acmcarther fyi I believe these became removable because of the newer rustup-style arrangement of the toolchain

This test fails with: error[E0463]: can't find crate for --> test/chained_direct_deps/mod3.rs:4:1 | 4 | extern crate mod1; | ^^^^^^^^^^^^^^^^^^ can't find crate A git bisect indicate that bazelbuild#122 is the culprit change.

Before bazelbuild#122 the test was wrongly passing.

…155) * Test for failure to import crate in rust_doc_test. This test fails with: error[E0463]: can't find crate for --> test/chained_direct_deps/mod3.rs:4:1 | 4 | extern crate mod1; | ^^^^^^^^^^^^^^^^^^ can't find crate A git bisect indicate that #122 is the culprit change. * fix: doc_test Before #122 the test was wrongly passing. * Fix rustdoc_test by using the dependency's providers Instead of trying to recreate the DepInfo provider, use directly the dep's DepInfo. It allows to correctly add all the direct deps to the command line. * Exclude rust_doc_test from RBE All other rust_doc_test are already excluded.

mfarrugi added 4 commits August 20, 2018 23:25

works

7845266

Move things to a CrateInfo provider.

21326c5

Rename build_rustc_command

2127828

buildifier

b9a684d

mfarrugi requested a review from acmcarther August 21, 2018 03:33

googlebot added the cla: yes label Aug 21, 2018

mfarrugi commented Aug 21, 2018

View reviewed changes

mfarrugi added 6 commits August 21, 2018 10:57

use CrateInfo better in rustdoc_impl

3413fbb

Make rust_doc_test not a noop...

5f22cf1

misc

23ef7e6

refactor _setup_deps

bfa5174

more misc

76852ab

fix test breaks

e5ba36a

mfarrugi changed the title ~~(wip) Deduplicate rule impls (#117)~~ Deduplicate rule impls, fix rust_doc_test (#117) Aug 21, 2018

remove errant comments / files

f399ff5

mfarrugi commented Aug 21, 2018

View reviewed changes

bikeshedding

ced8943

acmcarther reviewed Aug 22, 2018

View reviewed changes

acmcarther approved these changes Aug 23, 2018

View reviewed changes

mfarrugi commented Aug 25, 2018

View reviewed changes

mfarrugi added 2 commits August 25, 2018 11:22

Skip rust_doc_tests on RBE.

1af2030

Add docs and renames as per review.

d0c77c0

mfarrugi merged commit eb46819 into bazelbuild:master Aug 25, 2018

mfarrugi mentioned this pull request Aug 25, 2018

Runfiles Tests are failing on remote execution #112

Closed

damienmg mentioned this pull request Nov 11, 2018

Fix rust_doc_test with transitive dependencies and add tests for it #155

Merged

damienmg added a commit to damienmg/rules_rust that referenced this pull request Nov 11, 2018

fix: doc_test

f51c987

Before bazelbuild#122 the test was wrongly passing.

mfarrugi deleted the marco-consolidate-compile branch October 19, 2020 03:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate rule impls, fix rust_doc_test (#117) #122

Deduplicate rule impls, fix rust_doc_test (#117) #122

mfarrugi commented Aug 21, 2018 •

edited

Loading

mfarrugi Aug 21, 2018

mfarrugi Aug 21, 2018

acmcarther Aug 23, 2018

mfarrugi Aug 23, 2018

mfarrugi commented Aug 21, 2018

mfarrugi Aug 21, 2018

mfarrugi Aug 21, 2018

mfarrugi commented Aug 21, 2018

mfarrugi commented Aug 22, 2018

acmcarther left a comment

acmcarther Aug 22, 2018

mfarrugi Aug 23, 2018

acmcarther Aug 22, 2018

mfarrugi Aug 23, 2018

acmcarther Aug 23, 2018

acmcarther Aug 22, 2018

mfarrugi Aug 23, 2018

acmcarther Aug 23, 2018

mfarrugi Aug 25, 2018

acmcarther Aug 23, 2018

mfarrugi Aug 23, 2018

acmcarther Aug 23, 2018

acmcarther Aug 23, 2018

acmcarther Aug 23, 2018

acmcarther Aug 23, 2018

acmcarther Aug 23, 2018

acmcarther Aug 23, 2018

mfarrugi commented Aug 23, 2018

mfarrugi Aug 25, 2018

Deduplicate rule impls, fix rust_doc_test (#117) #122

Deduplicate rule impls, fix rust_doc_test (#117) #122

Conversation

mfarrugi commented Aug 21, 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

mfarrugi commented Aug 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfarrugi commented Aug 21, 2018

mfarrugi commented Aug 22, 2018

acmcarther left a 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

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

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

mfarrugi commented Aug 23, 2018

Choose a reason for hiding this comment

mfarrugi commented Aug 21, 2018 •

edited

Loading