Support crate_type file extensions more fully #38

acmcarther · 2017-02-22T04:31:27Z

Removes the hard coded rust_lib output, and declares the output file within the rust_library_impl. This lets us vary the file extension based on the toolchain -- OSX produces .dylib where linux would produce .so, for example.

Original description follows:

Tentatively addresses #36.

WIP: Need tests and understand platform implications

@davidzchen: Do we need the additional complexity of a hidden build rule? This change seems to work on my platform to produce dylibs that can be loaded correctly.

I've no idea how to handle different platforms. I added it as an additional attr, but I think a config_setting probably makes more sense.

davidzchen · 2017-02-23T01:06:18Z

rust/rust.bzl

@@ -167,6 +188,19 @@ def _get_features_flags(features):
    features_flags += ["--cfg feature=\\\"%s\\\"" % feature]
  return features_flags

+def _get_crate_type_and_target_outputs(name, crate_type, platform):
+  extension = LIBRARY_AND_PLATFORM_TO_EXTENSION[crate_type or "rlib", platform or "linux"]


Break up this line. See: https://bazel.build/versions/master/docs/skylark/bzl-style.html

This is done (and was done a while ago)

Hoping posting this comment will complete the review.

davidzchen · 2017-02-23T01:09:07Z

rust/rust.bzl

-    outputs = {
-        "rust_lib": "lib%{name}.rlib",
-    },
+    outputs = _get_crate_type_and_target_outputs,


This does not seem idiomatic Skylark. Also, it is unclear to me how this would actually work where we pass in a function for the outputs parameter of rule. Is this an obscure Bazel/Skylark feature? I do not see it documented as a way to specify outputs in both the Skylark documentation and the Skylark Cookbook.

@laurentlb @dslomov Can you comment on this?

This is a problem on our side, that should be documented. I believe @fweikert is the one who implemented the callback for outputs.

I didn't implement the feature (it has been in Bazel since April 2015), but I have a pending change that improves the documentation for implicit outputs in general (including callbacks).

Currently, the documentation of the "outputs" parameter mentions callbacks:
https://bazel.build/versions/master/docs/skylark/lib/globals.html#rule

Ah! Good to know. Thanks for the clarification. Looking forward to the documentation.

I was curious where I even got this from.... I made this change a while ago for my own use but couldn't remember how I'd found this feature, let alone find it again in the documentation.

Thanks all!

davidzchen

Also, please rebase and resolve merge conflicts.

damienmg · 2017-02-24T16:26:15Z

Removing myself as David can perfectly manage this review

davidzchen · 2017-02-24T21:17:59Z

@acmcarther This change LGTM and will be good go after rebasing and the minor style comment above. Thanks for working on this!

acmcarther · 2017-02-24T21:26:30Z

@davidzchen Do you have any advice on resolving the platform issue? I'm not confident "platform" should be part of the build rules, but I'm also not sure I want to even solve the problem in this PR.

We could kick the can down the road by assuming everyone is linux... That's still an improvement over the current state, which just expected an rlib output...

davidzchen · 2017-02-27T06:12:21Z

@acmcarther The only way I am aware of to get the host platform as a string would be to use ctx.fragments.cpp.cpu, but that requires ctx, which is only available in the rule implementation function. In this case, we want that information in the outputs callback. I also tried setting the default for the platform attribute to a select based on the //rust:darwin and //rust:k8 config_settings:

_rust_library_attrs = {
    ...
    "platform": attr.string(
        default = select({
            "//rust:darwin": "darwin",
            "//rust:k8": "linux",
        })),
}

However, this does not work because default only accepts a string.

@damienmg, @fweikert - Is it currently possible to get this information from a outputs callback? If not, what is the most idiomatic way to implement this behavior?

acmcarther · 2017-03-07T02:40:00Z

Rebased
Addressed comments
Added dylib building rule

@davidzchen Rebase killed the commit your comment was on. Can you ack the review?

I'm pretty unsatisfied with the current platform situation, but I think we can improve the situation later.

acmcarther · 2017-03-31T01:54:03Z

@davidzchen This is ready, but I can't address your review comment because the commit is gone. PTAL

davidzchen · 2017-04-24T08:40:44Z

Sorry for the late reply. Let's go ahead and get this committed. Can you add a TODO comment to address the platforms issue in the previous comments and rebase?

…nism

acmcarther · 2017-05-31T22:43:20Z

Rebased. I still cant clear up the "requested changes" snippet. Looks to be stuck...

EDIT: Oops, test is failing. Fixing.

acmcarther · 2017-05-31T22:50:35Z

OK:

This is broken in a non-trivial way. The (new?) build bot is also building for darwin. This PR adds a rust_library of type "dylib" that is producing a .dylib file on darwin. This PR was attempting to kick platform detection down the road by including a platform attr. That's not going to work in this situation.

@davidzchen: Do you have any suggestions on proceeding? I can make the required change, but it might take a while. I don't think theres a "short cut" here -- rustc will always produce platform dependent outs for dylibs, and thats a sore point for Bazel.

mfarrugi · 2017-11-03T22:04:33Z

This looks like it's also a blocker for dynamically linking cc_libs, as https://github.com/bazelbuild/rules_rust/blob/master/rust/rust.bzl#L169 also takes the shortcut of assuming only static linking.

Is this stalled on anything in particular, @davidzchen?

acmcarther · 2017-11-03T22:07:57Z

This needs a second look to either make it smarter (w.r.t. platform specific file extensions), or a trick to get the tests to pass.

I'll take a look at updating this PR this weekend.

EDIT: added "specific file"

mfarrugi · 2017-11-04T19:01:31Z

@acmcarther I started adding support for linking to native dylibs, but bazel run and bazel test don't appear to pass LD_LIBRARY_PATH to the binary.

Did this change actually support running or testing crates downstream of dylibs? It would be straightforward to add a wrapper similar to the bench rule, but I don't want to conflict with this change or redo work.

mfarrugi · 2018-02-22T02:28:47Z

@acmcarther What were the test failures or the fix for this? CI doesn't have logs for it.

acmcarther · 2018-02-22T02:50:33Z

@mfarrugi Expected output file not produced under OSX. rustc is producing xyz.dylib, where xyz.so is expected.

Need to figure out a trick (perhaps using the toolchain) to dictate the expected output file.

EDIT:
To elaborate, I suspected that "callback" mechanism I'm using here to use attrs for the purpose of determining the output might also be extendable further to using the toolchain to determine the extension. I tried it locally though, and found that I didn't have access to the toolchain. Might be a minor oversight in the definition of the callback -- haven't had a chance to pursue further.

acmcarther · 2018-02-22T17:15:10Z

There are three moving parts as I see it here:

The selected toolchain
The desired output target (e.g. linux,macos,windows)
The desired crate type

(1) dictates the defaults for all crate types. (2) selects and overrides the values decided by (1), while (3) narrows the output down to a single possible value. (2) and (3) are already knowable within the output filepath determining callback, but I can't find any mechanism to determine (1). I dropped a line in bazel-discuss to see what might be available.

A totally different approach could be evaluated as well. Rustc does support an -o OUTPUT_NAME directive, which should let us force the file to have a consistent extension. This isn't easy to use for two reasons though

Using -o requires us to specify the full file path, not just a directory as we're currently doing.
I believe rustc relies on the output file extension to know how to handle the dependency.

If (2) is true, we'd need to retain metadata with the rust_library to know how to turn it back into something rustc knows how to link.

acmcarther · 2018-02-22T18:27:45Z

Apparently (from the bazel-discuss post) you do not need to specify the outputs directly -- you can specify them in the impl with a call to "declare_outputs". This lets us avoid the callback shenanegans entirely and defer this bit of planning until the impl. By the time we enter the impl, we have all the attrs and the toolchain.

Will update with this fix some time today.

Not available after rebase.

acmcarther · 2018-02-23T05:54:55Z

Tests now pass! @mfarrugi: PTAL at your earliest convenience. Hopefully this works with #61 with minimal changes.

acmcarther · 2018-02-23T05:57:22Z

README.md

+          The type of crate to be produced during library compilation. This
+          list closely matches Cargo's own notion of crate-type, and the
+          available options are "lib", "rlib", "dylib", "cdylib", "staticlib",
+          and "proc-macro".


'proc-macro' isn't tested in this PR as the required crate dependencies are impractical to import without cargo-raze.

For the record, the required crates are

extern crate proc_macro; extern crate syn; extern crate quote;

EDIT: oops, github doesn't support comments to selected text.

mfarrugi · 2018-02-23T15:14:40Z

rust/rust.bzl

+    extension = toolchain.dylib_ext
+  elif crate_type == "staticlib":
+    extension = toolchain.staticlib_ext
+  elif crate_type in ("rlib", "lib", "proc-macro"):


proc-macro should be a dylib (though I don't know if rustc actually cares)

Ah this is important. We're relying on rustc to "produce the right thing" without us telling it what to make. Indeed, it is producing a dylib in this case, so we need to expect a dylib. Not sure how I became under the impression that this was an rlib -- my local code pre-osx fix used dylib here.

Fixed.

acmcarther · 2018-02-23T22:06:48Z

ok to test

EDIT: Not sure what this check wants from me..

buchgr · 2018-02-25T11:22:27Z

@acmcarther apologies for the inconvenience. Bazel is in the process of moving to a new CI system, called Buildkite. I have sent an invite to your @google.com e-mail address. Once you are registered please click on the "Details" link in the "Verify Pull Request" status section of this PR. This will unblock CI and run build and test. We'll be able whitelist googlers soon.

acmcarther · 2018-02-25T11:51:58Z

@buchgr No problem, thanks for helping resolve the issue.

I suspected it was some form of webui after leafing through https://github.com/bazelbuild/continuous-integration and not seeing a secret code phrase.

buchgr · 2018-02-25T12:48:05Z

@acmcarther all future pull requests from you should trigger CI without a verification step. I have added an ACL passed on github collaborators. external contributors will require a project collaborator to approve it. please file an issue on our continous-integration repo if that's not the case

googlebot added the cla: yes label Feb 22, 2017

davidzchen requested review from laurentlb, damienmg, dslomov and davidzchen February 23, 2017 01:05

davidzchen reviewed Feb 23, 2017

View reviewed changes

davidzchen previously requested changes Feb 23, 2017

View reviewed changes

damienmg removed their request for review February 23, 2017 22:15

damienmg assigned davidzchen Feb 23, 2017

davidzchen removed request for laurentlb and dslomov February 24, 2017 21:15

acmcarther force-pushed the acm-support-crate_type branch from f1fa909 to de66abc Compare March 7, 2017 02:29

acmcarther force-pushed the acm-support-crate_type branch from 8d60706 to ebad356 Compare March 31, 2017 01:53

acmcarther added 2 commits May 31, 2017 15:41

Fix support for crate_type w.r.t. output file extension

d102ae5

Add TODO to consider a more sophisticated file extension naming mecha…

86cab07

…nism

acmcarther force-pushed the acm-support-crate_type branch from ebad356 to 86cab07 Compare May 31, 2017 22:41

acmcarther mentioned this pull request Sep 19, 2017

Exposing a rust library to C code #55

Closed

acmcarther mentioned this pull request Jan 26, 2018

Add rust_plugin rule to support Rust compiler plugins #40

Closed

acmcarther mentioned this pull request Feb 13, 2018

Problem with cgmath acmcarther/cargo-raze#30

Closed

acmcarther mentioned this pull request Feb 22, 2018

Rust bazel rules acmcarther/cargo-raze#27

Closed

acmcarther added 4 commits February 22, 2018 20:23

Merge master

291cc15

HACK

e6055be

More!

a81e76a

Docs

35735d7

acmcarther commented Feb 23, 2018

View reviewed changes

mfarrugi approved these changes Feb 23, 2018

View reviewed changes

Fix proc-macro

e9a6f6d

acmcarther mentioned this pull request Feb 23, 2018

add new bazel ci configuration #73

Merged

Merge branch 'master' into acm-support-crate_type

f684399

whats your deal, man

ef8879f

acmcarther force-pushed the acm-support-crate_type branch from 4fbd8e0 to ef8879f Compare February 23, 2018 22:12

acmcarther merged commit c9ef636 into bazelbuild:master Feb 26, 2018

acmcarther deleted the acm-support-crate_type branch February 26, 2018 21:10

acmcarther mentioned this pull request Mar 14, 2018

rust_library should produce non-rlib crates with correct file extensions #36

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support crate_type file extensions more fully #38

Support crate_type file extensions more fully #38

acmcarther commented Feb 22, 2017 •

edited

Loading

davidzchen Feb 23, 2017

acmcarther Mar 31, 2017

davidzchen Feb 23, 2017

damienmg Feb 23, 2017

fweikert Feb 24, 2017

davidzchen Feb 24, 2017

acmcarther Feb 24, 2017

davidzchen left a comment

damienmg commented Feb 24, 2017

davidzchen commented Feb 24, 2017

acmcarther commented Feb 24, 2017

davidzchen commented Feb 27, 2017

acmcarther commented Mar 7, 2017

acmcarther commented Mar 31, 2017

davidzchen commented Apr 24, 2017

acmcarther commented May 31, 2017 •

edited

Loading

acmcarther commented May 31, 2017

mfarrugi commented Nov 3, 2017

acmcarther commented Nov 3, 2017 •

edited

Loading

mfarrugi commented Nov 4, 2017

mfarrugi commented Feb 22, 2018

acmcarther commented Feb 22, 2018 •

edited

Loading

acmcarther commented Feb 22, 2018 •

edited

Loading

acmcarther commented Feb 22, 2018

acmcarther commented Feb 23, 2018

acmcarther Feb 23, 2018 •

edited

Loading

mfarrugi Feb 23, 2018

acmcarther Feb 23, 2018

acmcarther commented Feb 23, 2018 •

edited

Loading

buchgr commented Feb 25, 2018 •

edited

Loading

acmcarther commented Feb 25, 2018

buchgr commented Feb 25, 2018

Support crate_type file extensions more fully #38

Support crate_type file extensions more fully #38

Conversation

acmcarther commented Feb 22, 2017 • edited Loading

Original description follows:

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

davidzchen left a comment

Choose a reason for hiding this comment

damienmg commented Feb 24, 2017

davidzchen commented Feb 24, 2017

acmcarther commented Feb 24, 2017

davidzchen commented Feb 27, 2017

acmcarther commented Mar 7, 2017

acmcarther commented Mar 31, 2017

davidzchen commented Apr 24, 2017

acmcarther commented May 31, 2017 • edited Loading

acmcarther commented May 31, 2017

mfarrugi commented Nov 3, 2017

acmcarther commented Nov 3, 2017 • edited Loading

mfarrugi commented Nov 4, 2017

mfarrugi commented Feb 22, 2018

acmcarther commented Feb 22, 2018 • edited Loading

acmcarther commented Feb 22, 2018 • edited Loading

acmcarther commented Feb 22, 2018

acmcarther commented Feb 23, 2018

acmcarther Feb 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acmcarther commented Feb 23, 2018 • edited Loading

buchgr commented Feb 25, 2018 • edited Loading

acmcarther commented Feb 25, 2018

buchgr commented Feb 25, 2018

acmcarther commented Feb 22, 2017 •

edited

Loading

acmcarther commented May 31, 2017 •

edited

Loading

acmcarther commented Nov 3, 2017 •

edited

Loading

acmcarther commented Feb 22, 2018 •

edited

Loading

acmcarther commented Feb 22, 2018 •

edited

Loading

acmcarther Feb 23, 2018 •

edited

Loading

acmcarther commented Feb 23, 2018 •

edited

Loading

buchgr commented Feb 25, 2018 •

edited

Loading