Add support for depending on shared libraries on linux. #61
Conversation
|
Can one of the admins verify this patch? |
|
@damienmg can someone take a look at this? I'm not that confident this is how it should be done. |
|
+David and Marcel
…On Wed, Nov 15, 2017, 12:45 PM Marco Farrugia ***@***.***> wrote:
@damienmg <https://github.com/damienmg> can someone take a look at this?
I'm not that confident this is how it should be done.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#61 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADjHf3PI6Xtb676uYYjH5-VJxqkMydWUks5s201QgaJpZM4QbFkX>
.
|
|
Would love any expert bazel eyes on this. |
rust/rust.bzl
Outdated
| @@ -49,11 +49,12 @@ rust_repositories() | |||
| [Cargo](https://crates.io/). | |||
| """ | |||
|
|
|||
| load(":toolchain.bzl", "build_rustc_command", "build_rustdoc_command", "build_rustdoc_test_command") | |||
| load(":toolchain.bzl", "build_rustc_command", "build_rustdoc_command", "build_rustdoc_test_command", "relative_path") | |||
There was a problem hiding this comment.
Keep line length to 80 chars.
rust/toolchain.bzl
Outdated
| @@ -101,6 +106,19 @@ def build_rustdoc_test_command(ctx, toolchain, depinfo, lib_rs): | |||
| depinfo.search_flags + | |||
| depinfo.link_flags) | |||
|
|
|||
| # @TODO(review) How should this be done? cc_* logic seems much more involved. | |||
There was a problem hiding this comment.
@dslomov Can you help with this portion of the review? Thanks.
There was a problem hiding this comment.
@mhlopko Can you help take a look at this part of the review?
There was a problem hiding this comment.
In particular, I'm concerned about handling the placement of .so's emitted by setting crate_type=dylib (which is supported by another PR).
For depending on cc_* rule dylibs I don't think there is any concern, but the cc_* logic is suspiciously complex.
There was a problem hiding this comment.
Did your concerns about this get resolved? Either way, we probably don't want to submit this with this TODO and below comment intact.
There was a problem hiding this comment.
No, but I haven't had issues and neither has anyone else, so that's good enough for now.
rust/toolchain.bzl
Outdated
| path_parts = path.split("/") | ||
| return [part for part in path_parts if part != "."] | ||
|
|
||
| def relative_path(src_path, dest_path): |
There was a problem hiding this comment.
If this function is internal to this bzl file, prefix with _
There was a problem hiding this comment.
It's also used in rust/rust.bzl
mfarrugi
force-pushed
the
pr-native-dylib
branch
from
3bc477a
to
37398ae
Compare
|
Can one of the admins verify this patch? |
|
How do I get CI to run on this? Maybe would have caught acmcarther/cargo-raze#31 |
|
ok to test |
rust/toolchain.bzl
Outdated
| # Construct features flags | ||
| features_flags = _get_features_flags(ctx.attr.crate_features) | ||
|
|
||
| return " ".join( | ||
| ["set -e;"] + | ||
| # If TMPDIR is set but not created, rustc will die. | ||
| ["if [[ -v TMPDIR ]]; then mkdir -p $TMPDIR; fi;"] + |
There was a problem hiding this comment.
"-v" is apparently only available since bash 4.2, and "[[ ]]" is apparently not portable.
Try "if [ ! -z "${TMPDIR+x}" ];" ?
|
CI requires this to work for osx (but not windows?), which requires a very different approach to setting rpath. I would appreciate some bazel-er input before reinventing how cpp linking is done in skylark. cc acmcarther/cargo-raze#27 I stand surprised :) |
|
Going to need to skip the osx tests for this, not sure how to do that yet. |
|
I've been building from under a rock apparently. This change is what makes depending on system libraries work! I've been silently using this in my side project, and when I went to use bazelbuild/rules_rust:master, everything broke e.g: mfarrugi@: can you remind me what the next steps are on this PR? From what I can tell:
I don't have the best background for (2) and (3), but I can read up enough for a passable code review and maybe help come up with something functional enough for now. |
|
I'm mostly caught up at this point on the way this works. The dep tracking strikes me as sloppy (in particular, the way we're tracking [libs, transitive_libs, transitive_dylibs, and transitive_staticlibs]), but I suspect that might just be my unfamiliarity with the problem space. I've got a couple of comments mostly around error handling and being a little more explicit about assumptions, and I'm curious what the resolution to the |
rust/toolchain.bzl
Outdated
| path_parts = path.split("/") | ||
| return [part for part in path_parts if part != "."] | ||
|
|
||
| def relative_path(src_path, dest_path): |
There was a problem hiding this comment.
Nit: Does it make sense to visually separate this public function from the private functions by moving it to the top of the file?
There was a problem hiding this comment.
I think there are enough of these utility methods to pull out a utils.bzl, should I hold off on that?
rust/toolchain.bzl
Outdated
| return [] | ||
|
|
||
| if toolchain.os != 'linux': | ||
| print("Runtime linking is not supported on {}, but found {}".format( |
There was a problem hiding this comment.
Perhaps this should fail instead of print.
There was a problem hiding this comment.
This builds correctly, but run will fall. My thinking was that this technically works, and is easier to iterate on for users and contributors.
There was a problem hiding this comment.
CI says I'm wrong, so I'll fail here and ignore it in ci builds. (https://source.cloud.google.com/results/invocations/46ee1fc3-222d-491f-bbb7-24d208119f66/log)
rust/rust.bzl
Outdated
| def _get_lib_name(lib): | ||
| """ Returns the name of a library artifact, eg. libabc.a -> abc""" | ||
| libname, ext = lib.basename.split(".", 2) | ||
| return libname[3:] |
There was a problem hiding this comment.
Is it worth checking to make sure that lib.startswith("lib")?
rust/toolchain.bzl
Outdated
| @@ -101,6 +106,19 @@ def build_rustdoc_test_command(ctx, toolchain, depinfo, lib_rs): | |||
| depinfo.search_flags + | |||
| depinfo.link_flags) | |||
|
|
|||
| # @TODO(review) How should this be done? cc_* logic seems much more involved. | |||
There was a problem hiding this comment.
Did your concerns about this get resolved? Either way, we probably don't want to submit this with this TODO and below comment intact.
It is kinda sloppy, but a proper fix for #78 will likely end up rewriting a lot of that to clean it up (at least from some preliminary hacking I have in a local branch). IMHO it probably makes sense to keep it this way for now, since it's a logical extension of how it already works, and then clean it all up in one go when a fix for #78 is tackled. (Sorry for the drive-by review comments, when I get some free time I'm planning to take a whack at solving #78 and it conflicts enough with this that one of them needs to go first.) |
|
@acmcarther this is up to date again |
|
|
@acmcarther sorry about that, was running against Can I get added to the auto-ci whitelist to streamline changes a little bit more? |
|
@acmcarther do we want anything else here? If so, lmk and we can let #84 go ahead next while I work that out (cc @bobsomers ). |
|
Regd #61 (comment) I don't actually know how to add someone to the whitelist and https://github.com/bazelbuild/continuous-integration/blob/master/buildkite/README.md#pull-requests wasn't immediately enlightening. @buchgr: What's the org-wide attitude toward adding people to the CI whitelist? Is there a suggested approach to doing that, or perhaps is "ci whitelist" considered roughly equivalent to maintainer status anyway? Regd #61 (comment) This looks good to me, but there is a merge conflict. Can you fix that and tag me again and I'll ship. |
|
@acmcarther you can add someone to the whitelist by adding him as a collaborator of rules_rust on Github. |
|
I have added @mfarrugi as a collaborator (Settings -> Teams & Collaborators). Need to accept the invite and then CI should just work. |
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
|
@acmcarther merged, not sure what exactly bot is unhappy with though. |
|
A Googler has manually verified that the CLAs look good. (Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.) |
|
#61 (comment) @buchgr: Thank you for the quick attention! I'll remember the collaborator process and use it in the future. |
This change allows
rust_libraryandrust_binaryto depend on native shared libraries.This requires 2 additional steps, collecting shared libraries from cc_library and rust_library, and setting the rpath in executables.
I added an example of this, and tested more extensively on another project.
I am not sure how much work is left to support mac+windows or rust dylibs.