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

Supply crate metadata to rustc and uniqueify --extern'ed crates #47

Merged
merged 13 commits into from
Mar 8, 2018
17 changes: 13 additions & 4 deletions rust/rust.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def _determine_lib_name(name, crate_type, toolchain, lib_hash=""):
extension = None
if crate_type in ("dylib", "cdylib", "proc-macro"):
extension = toolchain.dylib_ext
Expand All @@ -216,8 +219,9 @@ def _determine_lib_name(name, crate_type, toolchain):
+ "please file an issue!") % crate_type)


return "lib{name}{extension}".format(name=name,
extension=extension)
return "lib{name}-{lib_hash}{extension}".format(name=name,
lib_hash=lib_hash,
extension=extension)

def _crate_root_src(ctx, file_names=["lib.rs"]):
if ctx.file.crate_root == None:
Expand All @@ -236,10 +240,14 @@ def _rust_library_impl(ctx):
# Find toolchain
toolchain = _find_toolchain(ctx)

# Determine unique hash for this rlib
output_hash = _determine_output_hash(lib_rs);

# Output library
rust_lib_name = _determine_lib_name(ctx.attr.name,
ctx.attr.crate_type,
toolchain);
toolchain,
output_hash)
rust_lib = ctx.actions.declare_file(rust_lib_name)
output_dir = rust_lib.dirname

Expand All @@ -257,6 +265,7 @@ def _rust_library_impl(ctx):
crate_type = ctx.attr.crate_type,
src = lib_rs,
output_dir = output_dir,
output_hash = output_hash,
depinfo = depinfo)

# Compile action.
Expand Down
9 changes: 8 additions & 1 deletion rust/toolchain.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ ZIP_PATH = "/usr/bin/zip"

# Utility methods that use the toolchain provider.
def build_rustc_command(ctx, toolchain, crate_name, crate_type, src, output_dir,
depinfo, rust_flags=[]):
depinfo, output_hash=None, rust_flags=[]):
"""
Constructs the rustc command used to build the current target.
"""
Expand All @@ -26,6 +26,10 @@ def build_rustc_command(ctx, toolchain, crate_name, crate_type, src, output_dir,
# Construct features flags
features_flags = _get_features_flags(ctx.attr.crate_features)

extra_filename = ""
if output_hash:
extra_filename = "-%s" % output_hash

return " ".join(
["set -e;"] +
depinfo.setup_cmd +
Expand All @@ -37,6 +41,9 @@ def build_rustc_command(ctx, toolchain, crate_name, crate_type, src, output_dir,
"--crate-name %s" % crate_name,
"--crate-type %s" % crate_type,
"-C opt-level=3",
# Disambiguate this crate from similarly named ones
"-C metadata=%s" % extra_filename,
"-C extra-filename='%s'" % extra_filename,
"--codegen ar=%s" % ar,
"--codegen linker=%s" % cc,
"--codegen link-args='%s'" % ' '.join(cpp_fragment.link_options),
Expand Down
11 changes: 5 additions & 6 deletions test/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -12,11 +12,11 @@ load(
)

rule_test(
name ="hello_lib_rule_test",
generates = ["libhello_lib.rlib"],
name = "hello_lib_rule_test",
generates = ["libhello_lib--1343350674.rlib"],
provides = {
"rust_lib": "/libhello_lib.rlib>\\?$",
"transitive_libs": "^\\[\\]$"
"rust_lib": "/libhello_lib--1343350674.rlib>\\?$",
"transitive_libs": "^\\[\\]$",
},
rule = "@examples//hello_lib:hello_lib",
)
Expand All @@ -29,7 +29,6 @@ rule_test(

rule_test(
name = "greeting_rule_test",
generates = ["libhello_lib.rlib"],
generates = ["libhello_lib--1343350674.rlib"],
rule = "@examples//hello_lib:hello_lib",
)

22 changes: 22 additions & 0 deletions test/conflicting_deps/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package(default_visibility = ["//visibility:public"])

load(
"//rust:rust.bzl",
"rust_library",
"rust_test",
)

rust_library(
name = "conflicting_deps",
srcs = ["src/lib.rs"],
deps = [
"//test/conflicting_deps/first_crate:example_name_conflict"
],
)

rust_test(
name = "conflicting_deps_test",
deps = [
":conflicting_deps",
],
)
15 changes: 15 additions & 0 deletions test/conflicting_deps/first_crate/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
package(default_visibility = ["//visibility:public"])

load(
"//rust:rust.bzl",
"rust_library",
)

rust_library(
name = "example_name_conflict",
srcs = ["src/lib.rs"],
deps = [
"//test/conflicting_deps/first_crate/second_crate:example_name_conflict"
],
)

12 changes: 12 additions & 0 deletions test/conflicting_deps/first_crate/second_crate/BUILD
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
package(default_visibility = ["//visibility:public"])

load(
"//rust:rust.bzl",
"rust_library",
)

rust_library(
name = "example_name_conflict",
srcs = ["src/lib.rs"],
deps = [],
)
5 changes: 5 additions & 0 deletions test/conflicting_deps/first_crate/second_crate/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
// This crate's name conflicts with its dependent but this should work OK.

pub fn example_conflicting_symbol() -> String {
"[from second_crate]".to_owned()
}
10 changes: 10 additions & 0 deletions test/conflicting_deps/first_crate/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
// This crate's name conflicts with its dependency but this should work OK.

extern crate example_name_conflict;

pub fn example_conflicting_symbol() -> String {
format!(
"[from first_crate] -> {}",
example_name_conflict::example_conflicting_symbol()
)
}
22 changes: 22 additions & 0 deletions test/conflicting_deps/src/lib.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
extern crate example_name_conflict;

// This crate depends on a pair of dependencies (transitively) that have the same name. This should
// work OK.

pub fn example_conflicting_symbol() -> String {
format!(
"[from main lib] -> {}",
example_name_conflict::example_conflicting_symbol()
)
}

#[cfg(test)]
mod tests {
#[test]
fn symbols_all_resolve_correctly() {
assert_eq!(
::example_conflicting_symbol(),
"[from main lib] -> [from first_crate] -> [from second_crate]".to_owned()
);
}
}