Support for Binary dependencies #227
Conversation
|
@damienmg As I mentioned in other pull requests, this is the thing I was working on. Because this change includes the others, it'd be fine to ignore it for now and focus on the others. I also feel this provides functionality similar to #222. I'll need to dig deeper at what the changes are there that were waiting for the recent |
|
Just to note, only the last 3 commits of this PR are the actual change |
|
@damienmg Ok, this should now only contain the changes relevant to this pull request |
|
This is a bit too much details for me and I would be more comfortable if @smklein or @acmcarther would review this one. |
|
Maybe if there's no movement on ehuss/cargo-clone-crate#1 the code could be incorporated into this project. I would like to try to avoid that if possible but I also don't feel that change will realistically be merged 😞 |
|
RustSec/cargo-lock#68 published edit: A |
|
I think I'll need to provide a mechanism to save [raze]
binary_lockfiles_dir = "//cargo/lockfiles"Note that this would follow the same placement rules as When you run If edit: I implemented something like this as I felt it was an important enough feature for build reproducibility. I'll update the top description |
|
Moving some of the context from the top description into a comment This pull request includes #226 #225 and #224 and is the implementation of #218. There are a few outstanding things that should likely be resolved before merging this. See ehuss/cargo-clone-crate#1 and RustSec/cargo-lock#68. |
|
@damienmg @smklein @acmcarther I think this pull request is ready for review. The only hesitation is that #230 should probably get closed first so 27afe6f can be removed from the list of commits. |
|
@smklein @acmcarther This change is now ready for review. |
|
@smklein Hey, do you know when you might get a chance to review this? 😅 |
|
Hey, sorry for the delay. I've been occupied with work, which has left me
less time to work on this open source project as I'd like :( I'll see if I
can find some time today.
…On Tue, Sep 29, 2020 at 11:39 AM UebelAndre ***@***.***> wrote:
@smklein <https://github.com/smklein> Hey, do you know when you might get
a chance to review this? 😅
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#227 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAY3T2PNZGNJ5OGDH2IGCFTSIH5TRANCNFSM4RT5MTKQ>
.
|
|
@smklein Thank you so much! I don't mean to pressure you, I was just looking to hear from you and find out if you felt like you had the bandwidth to do a review in the nearish future 😅 I'm pretty excited about this feature though so I hope the changes are good! 😄 |
There was a problem hiding this comment.
@acmcarther , I still think you should take a look - this only has partial overlap with my contributions to Raze, so I think it would be good to have some of your perspective on "feature-wise, is this something we want", and I can help review the lower-level details.
Top-level comments so far:
- If you depend on a binary then commands are inconsistent rust-lang/cargo#8708 implies this binary crates might not be supported long-term by Cargo - specifically, the comment "I don't know if there was ever any intent to support or not support it. Given that it is mostly broken now (and has never worked), a warning would probably help those who stumble on it."
Given this tendency, IMO it makes sense to implement this binary dependency feature in the most non-invasive way possible - I've added some comments regarding refactors for clarity.
-
Can we add some documentation for this feature, possibly in the README, under "handling unconventional crates"?
-
Thanks for the tests!
|
|
||
| // Note: This will need to change if the dependency version changes | ||
| let wasm_bindgen_path = | ||
| Path::new("./external/remote_binary_dependencies__wasm_bindgen_cli__0_2_68/cargo_bin_wasm_bindgen"); |
There was a problem hiding this comment.
Could we parse this as an environment variable and pass it from the build system?
There was a problem hiding this comment.
I can't think of an easy way off the top of my head, but would that simplify anything?
|
@smklein Addressed some feedback and asked some questions. Ready for another review 😄 Edit: I didn't see your last comment. Yes, I can definitely update the README |
|
@acmcarther I primarily have questions for you but also addressed some feedback. I know your time is limited but would greatly appreciate another review so I can get this pull request closed out 😄 |
|
Made a really brief pass, but I'll need to sit down and dedicate a block of time to address the other comments (and provide my promised substantive comments). |
|
(In general, despite my grumble grumble about how the abstractions are breaking down, I value new features over satisfying my personal taste in abstraction design, so please don't be too discouraged. As in my prior message, I'll set aside a block of time to get into the nuts and bolts of this PR soon.) |
|
@acmcarther Thanks! Looking forward to your feedback 😄 |
|
@acmcarther @smklein Hey do you guys have time today to do another review 😅 🙏 |
|
@acmcarther @smklein Hey, friendly ping for another review 😅 Do you know if you'll have time this week? |
|
I don't know what to do about the fact that this PR is still flagged as |
|
Sorry for my lack of responsiveness, I operate on Valve time (or Elon Musk time), not human time. Time apparently is very fluid for me. You're stuck in the "changes required" stage because you need an explicit LGTM. I think addressing all the requested changes is not sufficient. Anyway, I haven't gained any new insight in the time I spent being lazy, so I'll just review again (right now) and call it good. |
Good ol' Elon time 😆
Good to know, thanks!
@acmcarther You have no idea how excited I am to get these changes through!!! I really hope they're good 🙏 😅 |
There was a problem hiding this comment.
I appreciate the effort you've put in here. I'm gonna say some really annoying stuff though.
This is kind of unprecedented for me, but I'm going to insist you split this PR up into more manageable pieces. I've got many issues with the approach here and I'm kind of overwhelmed trying to structure them in a productive way without a tiresome amount of iteration.
Here's a general framework for how this PR could be split up in a way that I could productively review it.
- Update the dependencies of cargo-raze. This will be an easy review and submit.
- Implement support for generating metadata in "in-place". I'm not certain I'll actually approve this. I might instead ask you to move the responsibility of moving stuff into tempdir outside of the metadata fetcher. Basically, your preferred behavior would be come the actual behavior, and the "bonus stuff" around tempdirs would get moved out.
- Extend MetadataFetcher to support loading metadata necessary for (future) planning of binary dependencies. I'm not exactly sure how this should look, but one important point here is that we need additional metadata during planning for binary dependencies that isn't currently available. We should identify what that is and add it in a discrete PR.
- Leverage the additional metadata to extend generation to include binary dependencies.
- Add the smoke-tests.
Some of the particular issues (not covered, or covered obliquely or covered directly above) that motivate this request:
- The smoke test files being included here are kind of overwhelming.
- I don't think the planner should be generating files or performing remote requests. The behavior that lives here should probably live in metadata fetcher. That class has precedent for downloading files, network requests, etc. Conceptually, that class should be a "ball of all metadata needed for planning". If planner needs to go get a bunch more stuff, then our abstractions have failed us.
- I'm not sure how i feel about fields being moved around between WorkspacePlanner, CrateContext, etc. I wanted to address this, but felt a bit overwhelmed by the rest of the PR so I didn't get a chance to devote a lot of thought to it yet.
- I raised this earlier, but I'm not really comfortable with how the "clone" and "checksum" functions which perform network requests and file operations currently live in metadata.rs, but are primarily used by planning code. With (2), metadata.rs is probably the right place, but right now it feels arbitrary.
- All together, I feel like these "crate fetching" and "checksum" features deserve a proper abstraction. Something like a trait which represents the "main idea" of what they do, with an implementation to back it. This enables testing of the types that depend on this functionality, without performing the side effects.
- This is really minor, but we've let the
cyclomatic complexitynesting and general complexity of many of these functions get out of control. Inproduce_crate_contextsthere's afilter_mapwith a body that's ~50 lines long, at ~5th level of indentation in the containing function.
EDIT: wordsmithing
impl/src/metadata.rs
Outdated
| )?; | ||
|
|
||
| let crate_dir = dir.join(format!("{}-{}", name, version)); | ||
| match crate_dir.exists() { |
There was a problem hiding this comment.
I'm upgrading this from optional to Style Nit.
I kind of hate these matches-over-boolean, and there are a lot of them here. For consistency, I ask them all to be updated.
|
@acmcarther Ok, with those PR's merged it reduces the footprint of this change a bit more. I think this is now ready to resume discussions in earlier comments. |
|
Can we move smoke tests and examples somewhere else. That's a major gripe I have. (i recognize that they represent the test coverage for this feature... but maybe we need some unit tests if they're load bearing) |
Removed those commits. |
|
Holy cow you've done it. It's not 20k delta anymore! Thank you very much! I'm just about out of time for now to review, but I'll be glad to review this in detail ASAP (hopefully later today, but maybe 10AM tomorrow) |
impl/src/metadata.rs
Outdated
|
|
||
| pub fn fetch_crate_src( | ||
| registry_url: &str, | ||
| dir: &Path, |
There was a problem hiding this comment.
I really can't tell if you've deliberately or accidentally ignored these comments, and it bothers me.
impl/src/metadata.rs
Outdated
| let dir = tempfile::TempDir::new().unwrap(); | ||
| let lockfile_dir = tempfile::TempDir::new().unwrap(); | ||
|
|
||
| let (path, _files) = fetch_crate_src( |
There was a problem hiding this comment.
It is important to me that we make sure no unit tests are cloning git repositories.
This will probably require you to create abstractions which can inject dependencies. Specifically, cargo_clone::Cloner probably needs to be wrapped and stubbed for testing.
The larger problem here is that you've not really created any abstractions at all.
|
I might just LGTM this after a couple of cycles. Reviewing this PR has made me realize that there is a huge amount of code debt. |
|
#227 (comment) What comments? |
|
@acmcarther I don't understand why some comments don't have a field for me to reply to them If possible can I ask that you always create comments that have a section for me to directly reply to. It bothers me that you think I'm deliberately ignoring your feedback. There's a lot on this issue and it's difficult for me to keep track. If I see a comment that I can resolve, It helps me track that as an action item. Once all comments are resolved, then I would request another review. I'm doing my best here and am not trying to be rude or irritate anyone |
|
#227 (comment) I'm gathering metadata in the same place |
|
#227 (comment) I mocked the endpoints this would attempt to reach out to. This should never be doing any git operations btw. Do you still feel https://docs.rs/cargo-clone-crate/0.1.3/cargo_clone/struct.Cloner.html should be wrapped and mocked? |
There was a problem hiding this comment.
Well, whatever. If this is idiomatic (which, I think I'm coming to believe it is), then I think I hate modern Rust.
I have lots opinions about things (pattern matching over booleans, ".1" at the end of things, weird arrangement of responsibilities, you including ridiculously huge lambdas in filter_map), but I see you're the person that prepared #212, so I expect you to be better educated and more invested on the direction of things than I am and for that reason I trust your judgment in terms of where responsibilities should be and what is idiomatic in this language.
Someday I really would like to rewrite this whole thing and fix (what I see as) cruft that has accumulated due to my lack of good discipline in reviewing, but who know when that will happen, and I expect my code would be idiosyncratic w.r.t. the way the rest of the ecosystem writes code.
That all said. please try to prepare much smaller PRs in the future.
@acmcarther I'm still pretty new to Rust and have learned a lot in contributing to this project. I definitely still have a lot to learn though. I do agree with a lot of your feedback here and I have already made a few branches with some refactoring done that is targeted at creating clear separation of responsibility among the different modules. Though that being said, I'm definitely going to be having a stronger focus on smaller/more focused pull requests. I see a lot of potential in this project and want to do what I can to improve it. Do you think you could open a draft PR or an issue ticket outlining some of the things that bother you most about the current state of the project? I'd be happy to take a look and submit some PRs 😄 |
|
@acmcarther I've squashed the changes in this PR and rebased #240 on top of these changes. |
|
Thank you for your continued work here and elsewhere in the repo. I really appreciate it, and I think users of this project appreciate it even more Brain dump on future direction of the project follows. I might turn this into more specific actions and PRs I prepare, but if not, at least this is here: The real original sin of cargo-raze was the failure to write the original code with unit tests in mind. That means dependency injection where necessary, and clearly delineated responsibilities. Instead, once the lack of unit tests because a real practical problem (because things would break), I added this "smoke-test" thing, which has become load bearing, and in some cases makes code review a major hassle. If I were (motivated) to clean the repo up in a vacuum, the north star would be to make all of the code testable and well tested for the purpose of guiding refinement of the design. In more concrete terms this means:
Through this process, it would become more clear what abstractions are currently doing too much (not abiding by SRP), and I think also we would end up with an end product which is easier for developers to hack on, because they'd be able to start with TDD. They would figure out what is not being generated and add it to the functional and unit tests, then update the implementation to make it happen. Beyond code health, I think it's really important to get this project building with bazel as well, so that this can be brought in as a regular bazel dependency, compiled, and then used as part of genrules. In the past, I explored what doing this "late generation of BUILD files" might look like, but I ran into some fundamental issues with where this generation would happen in the bazel lifecycle and I got a bit disheartened. A long time ago, the model this project followed was generating EDIT: |
|
@acmcarther Thank you so much! Would you mind if I turned that message into an issue and we continued that conversation there? Also, is there anything more I should do for this PR? |
|
I updated the dependencies one last time. https://crates.io/crates/httpmock shipped version |
|
My comment probably represents four separate issues (which may or may not already exist)
|
|
Merging now. I'll cut a release that includes this and other recent changes. |
|
@acmcarther Thank you so so much!!!! I feel like some of the issues could be cleaned up or consolidated already. It might be worth doing a pass on them 😅 |
|
@acmcarther also, I'd like to use the changes this PR to update |
This change introduces support for
Binary dependencies. If you take a look at #218, an issue is outlined wherecargotreats binary dependencies differently than library dependencies and therefore, targets will not be generated when usingcargo raze. There is now support for a new field inCargo.tomlfiles called[raze.binaries]. See the following example:By adding this field and using
genmode = "Remote"(notably not "Vendored"), Raze will now download the crate from the registry specified in the settings, generate a lock file for it, and add the dependencies and it's metadata to the active raze session and include this metadata in the resulting Bazel files. An example of this can be seen in//examples/remote/binary_dependencies/...within this pull request.This change will also create a
lockfilesdirectory in theworkspace_pathfor the lockfiles of binary dependencies. The files written will be namedCargo.{crate}-{version}.lock. If one of these files exists in the lockfiles directory, wheneverrazeis run again it will inject a lockfile with a matching name given the aforementioned pattern even if the create has a lockfile bundled with it. If no lockfile is present in the lockfiles directory, the bundled lockfile or a newly generated one will be installed there at the end of a run.Another example of this change in use can be seen in bazelbuild/rules_rust#411.