Migrate `cargo-raze` into `rules_rust` #590

UebelAndre · 2021-02-16T16:10:31Z

This PR moves cargo-raze into this repo. There is a lot of functionality that is coupled between the two projects that I feel it makes sense to have the code in one place.

This is the simplest form of a migration I can think of. It moves all the code into cargo/cargo_raze and adds the directory to .bazelignore so it doesn't affect existing CI.

A future change should be made to better integrate this into CI but that can be a separate PR with tracked changes and more realistically reviewable PRs.

cc: @acmcarther @damienmg @illicitonion @mfarrugi @dfreese @hlopko

* Support './' in lib.path targets * Add normal target to test

* WIP * Add option * Fix whitespace somewhere

* Complain to stderr about unused raze settings. * Provide more useful help message.

Fixes issue #1

…nternals) (#28) Refactor planning in the manner of `cargo metadata` (but keep using internals)

* Integrate the examples into cargo-raze * Add the remote examples as well * Add a README doc for remote examples * Fix the build after adding remote README * Remove examples for PR * Enable -f on `rm` so no error if dirs don't exist * Explicitly reset working directory back to original * Rename examples/README.md * Move the command exists check to a function * Make sure `command_exists` actually checks for existence * DIR -> REPO_ROOT

* Fixes for #36 * Updates for Cargo.toml * Fix some pathing issues with smoke-test.sh * Fix up travis not liking the new packaging

* Add the sha256 to the generated http_archive This is good practice to have them and we get them for free from Cargo. * Remove unused warnings * Update Cargo.lock The previous one doesn't build on my platform. * Address acmcarther@ suggestions

Now that rust_library/rust_binary supports it.

Using native.existing_rules allow to use several crates.bzl file that would import the same crates without failing with duplicate rules.

* Pass 2 on remote git targets Previously this worked with the example because it actually resolved to crates.io even for "git" packages. The example was included before it was ready, and only because it relied exclusively on things already on crates.io did it compile in the first place. Tests back to compiling and running true -> True Fix a pathing issue Fix some unused imports * Suggested changes

* fix it * fix

* WIP: RM separate remote_ files * Simplify build_path slightly * More progress * Fix a test, save a life * Put the remote_crates back

* Update README.md Update the examples location now that they're inline to the main repository. * Update README.md Add back the OpenSSL example

* Wip the first * Wip the second * Clean code -- terrible formatter * Handle a lot of nits, but not the failing tests * Fix broken index insertion (debug_uggh) and rendering * Fix bug where root crate was expected to be vendored * Fix crate_context.build_path and update missing root crate test * Fix minor bugs with file generations * Fix tests and update some context var names

This rule is never used and was renamed in recent rust rules into rust_bechmark.

…ctory (#62) * Patch examples with latest cargo-raze generated output * Update deps, fix deprecation warnings, up to 0.1.0

UebelAndre · 2021-02-16T21:40:52Z

IMHO raze can be cleanly separated in its own repository, and only depend on the public interface of rules_rust. If changes to rules_rust break raze too often, we need to discuss our backwards compatibility policy.

It's not that changes to rules_rust breaks cargo-raze. It's more that someone wants to implement some new functionality that needs to be done in rules_rust and then setup in cargo-raze or cargo-raze was used to generate something but doesn't support some functionality so a change must be made there in order to update/re-generate something in rules_rust.

Again, very interested in your workflow. While I think merging the two repos would be a good idea, It's not (currently) my burning passion. I like the idea that the rules has a standardized tool so that all workflows can, to a more thorough degree, be brought to the same place and everyone can benefit from questions/changes.

UebelAndre · 2021-02-16T22:50:00Z

One other thing of note, I think if there's considerations for adding something like cargo_universe from #2 (comment) to rules_rust then I think it'd also be beneficial to have cargo_raze in here as well since it's a dependency of cargo_universe.

* `rust_rules_workspace_name` now defaults to `rules_rust` * Renamed `io_bazel_rules_rust` to `rules_rust` throughout the workspace * Updated branch name for rules_rust

…gnores (#368)

A bug was fixed where the "CARGO" environment variable wasn't being when looking for a cargo binary. This has been corrected and is now in use.

…t's config_settings (#372)

google-cla · 2021-02-20T15:46:35Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2021-02-20T15:57:13Z

All (the pull request submitter and all commit authors) CLAs are signed, but 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 by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

UebelAndre · 2021-02-20T15:59:22Z

@hlopko Thanks to @dfreese I've been able to drop most of the commits going into this change. There are only two commits that are noteworthy now. The merge commit (c965cee) and a commit that adds the new repository to .bazelignore (f5dd0ac).

dfreese · 2021-02-21T18:11:59Z

I've always wondered if it's worth having rules_rust focus explicitly on rustc, and then have other things implemented on top of that. What has been hard, I think is that rules_rust didn't expose enough of an interface for people to build things like clippy or protobuf integration on top of it.

The architecture that is described in #596, which @hlopko has been working towards with rust_common (providing a more rule_cc-like interface to rules rust) starts to solve some of those issues.

This PR is probably an opportune time to think about drawing some clear lines about what is rust and what is cargo.

Where it has been intermixed has led to some weird results, such as rules_proto being stuck on a particular crate version, where updating it would be a breaking change. Proper versioning in rules_rust would probably help (though it has a larger API surface than I would like).

UebelAndre · 2021-02-21T18:40:34Z

I've always wondered if it's worth having rules_rust focus explicitly on rustc, and then have other things implemented on top of that. What has been hard, I think is that rules_rust didn't expose enough of an interface for people to build things like clippy or protobuf integration on top of it.

I 100% agree. Though, I feel there can be a clear separation of responsibility within the same git repository. I think adding this to the cargo directory is an appropriate thing to do if that's where we're going to put "Cargo-esque" things.

dfreese · 2021-02-21T20:52:01Z

I've always wondered if it's worth having rules_rust focus explicitly on rustc, and then have other things implemented on top of that. What has been hard, I think is that rules_rust didn't expose enough of an interface for people to build things like clippy or protobuf integration on top of it.

I 100% agree. Though, I feel there can be a clear separation of responsibility within the same git repository. I think adding this to the cargo directory is an appropriate thing to do if that's where we're going to put "Cargo-esque" things.

True, but that makes versioning the project a little more tricky if the major sem-ver is attached to both the rust and the cargo, and particularly cargo-raze APIs.

UebelAndre · 2021-02-21T20:56:24Z

True, but that makes versioning the project a little more tricky if the major sem-ver is attached to both the rust and the cargo, and particularly cargo-raze APIs.

Given that only cargo-raze has any useful notion of versioning, I think that's something we can solve or set the standard for going forward fairly easily as it likely won't disrupt existing users. I personally don't think this is an issue and would imagine we tag releases with the thing actually being released (cargo or rustc rules). Or is that not a reasonable approach? I thought Google was known for monorepos, how is versioning handled internally?

acmcarther · 2021-02-23T04:43:12Z

I haven't had the time to ~~do literally anything recently~~ review the comments PR, but driveby comments anyway. Sorry if this tramples on quality discussion that covers these points already.

This mirrors my response to an email to me from @hlopko that I felt should generally be public.

In general I'm against this PR, not because I have some nuanced vision for the future of rules_rust, but primarily because I don't like how cargo-raze deals with the cargo-bazel integration, and would like there to be a vacuum of sorts to leave room for something "better" to fill the space. I think having this moved into rules_rust (by varying degrees, depending on whether it's in the repo "proper", or just lives in a contrib directory, or something else) solidifies this tool as the canonical way of things. I'm not super excited about that possibility.

I think a large part of any compatibility concern can be mitigated by having rules_rust include some test suite that builds a toy application using cargo-raze.

EDIT: https://github.com/facebookincubator/reindeer looks pretty interesting!

UebelAndre · 2021-02-23T04:54:33Z

I totally get that moving cargo-raze into this repo makes that tool the canonical way to interface with external crates, but I think that's a very realistic thing to do and something the rules should provide some solution for. If cargo-raze is not going to be a part of that, I'd love to hear proposals for what is. I think cargo-raze works fantastically and would be totally fine in the well defined world of "post #596". But I don't quite care what the solution is (even though I've spent a considerable amount of time on cargo-raze), I just care that there's some solution that doesn't expect users to tediously hand author their WORKSPACE dependencies.

damienmg · 2021-02-24T08:07:36Z

Hi,

Side-rant: for some reason github seems to hide some earlier comments :/

I also feel mixed about this PR. While I would love to see a canonical way to handle the cargo eco-system I also feel mixed with the current cargo raze approach which has problem scaling with monorepos (maintain 1000s of cargo files) and requires to write dependencies in a different language than the bazel one.

My view of the next step world would be where bazel is responsible for the download and compilation and maybe doing dependency resolution using a tool integrated with cargo (though I am pretty sure cargo graph resolution will blow up at some point). Cargo raze could be the tool doing the dependency resolution but I am not sure it is needed (could be using just the output of cargo instead).

The final world I see would probably require a custom dependency resolution tooling. I.e. I see a final usable states to have a set of depedency declared in a WORKSPACE file and pull their transitive deps the same way Cargo does but that requires also Bazel to get its story straight regarding external dependencies (and to be honest, I don't feel like there is any hope for a satisfying solution in the forseeable future).

hlopko · 2021-02-24T13:33:43Z

(I wrote to the Google Open Source Program Office, will report when I hear back).

Ok let's try to move forward with the discussion. After some thinking I came to the following conclusions:

If the decision was solely on me, I'd say no, reason being raze owners don't think raze is the best we can do
But the decision is not solely on me, and even if I could (which I don't think I do) I wouldn't veto the PR moving raze to rules_rust
But I will not champion the PR

Andre, if you can find a champion, and OSPO approves my proposal, feel free to move forward.

I'd recommend to:

Create a new top level package for raze (cargo is there for cargo build script runner and it doesn't make sense to me to put both these into the same directory; I'd be fine with moving cargo build script runner elsewhere, just not both in the single package)
Split the import into multiple PRs - 1) raw cp, no modifications 2) make it build in one or more PRs, 3) re-upload pending PRs from raze repo into rules_rust
Ideally we want to maintain git history in the process
Migrate raze issues into rules_rust issues; create a raze label

UebelAndre · 2021-02-24T14:52:37Z

* Create a new top level package for raze (cargo is there for cargo build script runner and it doesn't make sense to me to put both these into the same directory; I'd be fine with moving cargo build script runner elsewhere, just not both in the single package)

I would strongly advocate for this going into the cargo package. Put it in a top level package solidifies it as the canonical way to interact with external crates. I see this as purely a cargo tool and I think the cargo package is the right place as a result. If there is a rule that has no reliance on cargo but can do the same task, it can go in it's own package

* Split the import into multiple PRs - 1) raw cp, no modifications 2) make it build in one or more PRs, 3) re-upload pending PRs from raze repo into rules_rust

There's currently only 1 change other than a straight merge and it's the .bazelignore. I'd be happy to do that in a separate PR people feel strongly that there should absolutely be no other changes in the PR.

* Ideally we want to maintain git history in the process

I believe this PR is doing that.

* Migrate raze issues into rules_rust issues; create a raze label

Makes sense but I also don't think it's a blocker for any merge.

Andre, if you can find a champion, and OSPO approves my proposal, feel free to move forward.

Given that seemingly all "champions" are here and have said they don't like, it doesn't seem like this change is going to go through. Additionally, I have no idea how to interface with OSPO and don't think that's clear to anyone outside of Google.

I'm just going to close this because of the amount of push back it's getting. I'm happy to continue any conversations here or in some other form for things people would like to clarify or dig deeper into.

damienmg · 2021-02-24T15:27:58Z

Thanks for opening that discussion and driving it @UebelAndre, just to clarify the position of most seems like we would like to see a better approach to handling crate than the one cargo raze offer. If you have a good idea on how to deal with the problem, that would be a very interesting discussion to drive.

tschuett · 2021-02-24T15:34:12Z

rules_go has a different approach:
https://github.com/buchgr/bazel-remote/blob/3aa7b05f1657a647ed11f2104af4897c6cb65df8/WORKSPACE#L55

hlopko · 2021-02-26T08:23:44Z

I heard back from the OSPO, they approved the move. Not relevant anymore here it seems but it can be useful in the future.

+1 to what Damien said. Thank you @UebelAndre for pushing for this (and for all the other work you're doing for this repository!). It seems there is an opportunity to come up with a better solution. The first step would be to collect all the various (and maybe conflicting) requirements people have and come up with a solution that satisfies all. Maybe you can build on the momentum of this PR to collect more information and see if it would be interesting for you to drive the effort.

Thanks again!

acmcarther and others added 30 commits March 4, 2018 16:02

Run Rustfmt over everything (#9)

f0ddd97

Update README.md (#11)

eae2a98

Support './' in lib.path targets (#13)

c08631b

* Support './' in lib.path targets * Add normal target to test

Add data_attr for defining rust_library data dependencies (#10)

9293a28

* WIP * Add option * Fix whitespace somewhere

Up to 0.0.19

9c33c3b

Complain to stderr about unused raze settings. (#18)

4970262

* Complain to stderr about unused raze settings. * Provide more useful help message.

Update skipped_deps docs

1a8e659

Fixes issue #1

Follow my own best practices (#26)

88fdc20

Add cargo-raze-examples smoketest (#27)

1a1f2ab

Refactor planning in the manner of cargo metadata (but keep using i…

308d297

…nternals) (#28) Refactor planning in the manner of `cargo metadata` (but keep using internals)

Up to 0.0.20

0b28d00

Prettify metadata test fixture (#30)

7466537

Add RUSTC as env_var to buildscripts (#33)

2f65244

Update examples for f453818

941a3ca

Swap Genmode::Vendored and Genmode::Remote docs

a0c3432

Packaging update (#40)

0bec1af

* Fixes for #36 * Updates for Cargo.toml * Fix some pathing issues with smoke-test.sh * Fix up travis not liking the new packaging

Add missing license header files (#46)

ca9321b

Fix link to settings.rs

c864bbf

Add version number to imported crates (#49)

b69f242

Now that rust_library/rust_binary supports it.

Remote: do not create http repository if it already exists. (#50)

aadff8b

Using native.existing_rules allow to use several crates.bzl file that would import the same crates without failing with duplicate rules.

Reverts google/cargo-raze#33 (#37)

930d8a8

* fix it * fix

Remove remote templates and use additional data in context (#45)

19afaef

* WIP: RM separate remote_ files * Simplify build_path slightly * More progress * Fix a test, save a life * Put the remote_crates back

Update README.md (#55)

b90dfea

* Update README.md Update the examples location now that they're inline to the main repository. * Update README.md Add back the OpenSSL example

Run the formatter in isolation

2e4128f

Remove rust_bench_test from generated imports (#60)

ab679d2

This rule is never used and was renamed in recent rust rules into rust_bechmark.

Rel 0.1.0: Update deps, fix deprecation warnings, patch examples dire…

03e2996

…ctory (#62) * Patch examples with latest cargo-raze generated output * Update deps, fix deprecation warnings, up to 0.1.0

UebelAndre and others added 8 commits February 19, 2021 15:50

rust_rules_workspace_name now defaults to rules_rust (#323)

bd6ad86

* `rust_rules_workspace_name` now defaults to `rules_rust` * Renamed `io_bazel_rules_rust` to `rules_rust` throughout the workspace * Updated branch name for rules_rust

Up to 0.10.0 [via publish-new-version.sh]

efc3a83

Handle buildifier out-of-order-load by moving load statements add i…

3394691

…gnores (#368)

Update dependencies in examples to fix windows compatibility (#370)

302f6da

Fixed missing "CARGO" environment variable uses (#371)

f8162dd

A bug was fixed where the "CARGO" environment variable wasn't being when looking for a cargo binary. This has been corrected and is now in use.

Cargo raze tests now use cargo via the toolchain. (#369)

9ecec16

Prefixed 3rd party dependencies with cargo_raze__ and use rules_rus…

0cbe0ef

…t's config_settings (#372)

Fixed windows bug in examples repository rule

fd33d67

UebelAndre added 2 commits February 20, 2021 07:54

Merge remote-tracking branch 'raze/main' into cargo-raze

c965cee

Ignore cargo_raze workspace

f5dd0ac

UebelAndre mentioned this pull request Feb 22, 2021

Add ARCHITECTURE.md #596

Merged

UebelAndre closed this Feb 24, 2021

UebelAndre deleted the cargo-raze branch February 25, 2021 13:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate `cargo-raze` into `rules_rust` #590

Migrate `cargo-raze` into `rules_rust` #590

UebelAndre commented Feb 16, 2021 •

edited

Loading

UebelAndre commented Feb 16, 2021

UebelAndre commented Feb 16, 2021

google-cla bot commented Feb 20, 2021

google-cla bot commented Feb 20, 2021

UebelAndre commented Feb 20, 2021

dfreese commented Feb 21, 2021

UebelAndre commented Feb 21, 2021

dfreese commented Feb 21, 2021

UebelAndre commented Feb 21, 2021

acmcarther commented Feb 23, 2021 •

edited

Loading

UebelAndre commented Feb 23, 2021

damienmg commented Feb 24, 2021

hlopko commented Feb 24, 2021

UebelAndre commented Feb 24, 2021

damienmg commented Feb 24, 2021

tschuett commented Feb 24, 2021

hlopko commented Feb 26, 2021

Migrate cargo-raze into rules_rust #590

Migrate cargo-raze into rules_rust #590

Conversation

UebelAndre commented Feb 16, 2021 • edited Loading

UebelAndre commented Feb 16, 2021

UebelAndre commented Feb 16, 2021

google-cla bot commented Feb 20, 2021

google-cla bot commented Feb 20, 2021

UebelAndre commented Feb 20, 2021

dfreese commented Feb 21, 2021

UebelAndre commented Feb 21, 2021

dfreese commented Feb 21, 2021

UebelAndre commented Feb 21, 2021

acmcarther commented Feb 23, 2021 • edited Loading

UebelAndre commented Feb 23, 2021

damienmg commented Feb 24, 2021

hlopko commented Feb 24, 2021

UebelAndre commented Feb 24, 2021

damienmg commented Feb 24, 2021

tschuett commented Feb 24, 2021

hlopko commented Feb 26, 2021

Migrate `cargo-raze` into `rules_rust` #590

Migrate `cargo-raze` into `rules_rust` #590

UebelAndre commented Feb 16, 2021 •

edited

Loading

acmcarther commented Feb 23, 2021 •

edited

Loading