Supply crate metadata to rustc and uniqueify --extern'ed crates #47
Conversation
|
This needs a touch more work to handle static/cc dependencies. Good test! EDIT: now fixed. |
acmcarther
force-pushed
the
acm-hash-input-libs
branch
from
d42339c
to
c948734
Compare
rust/rust.bzl
Outdated
| @@ -156,7 +169,7 @@ def _setup_deps(deps, name, working_dir, allow_cc_deps=False, | |||
| symlinked_libs += [dep.rust_lib] + dep.transitive_libs | |||
| link_flags += [( | |||
| "--extern " + dep.label.name + "=" + | |||
| deps_dir + "/" + dep.rust_lib.basename | |||
| deps_dir + "/" + _hashed_lib_path(dep.rust_lib) | |||
There was a problem hiding this comment.
Can we hash the libname in the output instead? ie. rust_library.outputs = _hash_lib_name
I'm not clear on whether the callback api makes the path available, but it would simplify this logic a bit.
There was a problem hiding this comment.
Can you elaborate a bit on this suggestion? I think I have an understanding: You'd like to see the output (rlib|so|...) themselves be uniqueified, rather than doing this at link time?
There was a problem hiding this comment.
The way this is written rust_library.rust_lib is only ever consumed here, and it's incorrect to depend on the non-hashed rlib, so it's better to not expose the non-hashed rlib.
Somewhat premature, but this also repeats the _hashed_lib_path function in each dependent instead of once in the library.
(iirc .so and .a should not be hashed, since their symbols could conflict)
There was a problem hiding this comment.
L168 and L169 should also be referencing the hashed path (for compiler inputs to be 100% correct), and then you don't need to modify _setup_cmd either.
There was a problem hiding this comment.
OK: Done. Please take a look
|
Summary of changes:
I'm not sure if the example is appropriate. Its not really useful except for demonstrating the capability to do what this PR enables. EDIT: I might have a counterexample for my assertion in point (2), not sure if it matters though. |
rust/rust.bzl
Outdated
| # TODO(acmcarther): Use the toolchain to guarantee uniqueness by platform. | ||
| srcs_concat = "".join([src for src in srcs]) | ||
| rlib_hash = repr(hash(srcs_concat)) | ||
| # TODO(acmcarther): Use the toolchain to guarantee uniqueness by platform. |
|
I think it makes sense to hide the example as much as possible since it's not really something we want to encourage, but I'm not sure how the CI is setup to run (will it pickup test/example_tests?). lgtm otherwise edit: found https://github.com/bazelbuild/continuous-integration/blob/master/jenkins/jobs/configs/rules_rust.json so it can go anywhere |
| @@ -198,7 +198,10 @@ def _find_crate_root_src(srcs, file_names=["lib.rs"]): | |||
| return src | |||
| fail("No %s source file found." % " or ".join(file_names), "srcs") | |||
|
|
|||
| def _determine_lib_name(name, crate_type, toolchain): | |||
| def _determine_output_hash(lib_rs): | |||
| return repr(hash(lib_rs.path)) | |||
There was a problem hiding this comment.
@mfarrugi Can you take another look at this PR, specifically this line? I again changed the way the uniquifying hash is generated as the available information changed.
Before: hash output rlib name
Now: hash full lib.rs path (since the rlib name contains the hash in it)
Can you see any downsides in this approach or perhaps have a better thing to hash?
There was a problem hiding this comment.
I can think of the degenerate case where multiple targets use the same lib.rs, so maybe hash(name, lib.rs path) or Label? Though I don't recall what info is available.
There was a problem hiding this comment.
I don't think the name is useful as a component as a hash collision will only matter if the crate names are already the same...
I don't think changing this function later will be breaking or onerous, so I'll go ahead and get this submitted.
This PR applies an important change required to allow rustc to accept rlibs that have the same name. This issue arises commonly when compiling dependencies based on Cargo build plans, since having multiple versions of a crate in a dependency tree is acceptable there.