Parse settings from Cargo metadata #302
Conversation
|
Why does this CL have a 57k LOC added diff? |
|
@acmcarther This PR contains changes from others I'm hoping to get through before it. So it'll appear large until then. I'll be marking the PRs ready for review in the order I'd planned on rolling them out but you can also follow the chain in the tickets noted in the description. |
But to directly answer that. It's due to the changes in #300 where cargo metadata dumps were added for testing. Unfortunately, some tests use very large packages resulting in large json files. |
Much smaller now that #300 is not in this CL. This will also continue to shrink. |
|
Documentation has been updated in this PR as per a discussion here: #277 (comment) |
|
@acmcarther This is a draft PR. I open draft PRs mainly for reference and high level conversation. The state of most of my changes right now are that they're based on other changes that I plan to open PRs for (if they're not open already). This change is 2 PRs ahead of master. |
|
I came here because you referenced this PR in the other PR as an explanation of a design decision. |
Yeah, my message was in response to #302 (comment) Which I'm happy to talk about but not quite ready to do so since the reasoning for that is in another PR. But I thought it'd be valuable for you to see the state of things here at a high level. |
|
Just a heads up, I'm going to be rebasing this branch. I will stop rebasing once this is marked "Ready for review". |
|
@acmcarther This PR is now ready for review. |
| // Check for duplication errors | ||
| if duplicate_binary_deps.len() > 0 { | ||
| return Err(anyhow!( | ||
| "Duplicate `raze.binary_deps` values detected accross various crates: {:?}", |
There was a problem hiding this comment.
Hmm..
I think we should permit duplicate binary deps provided they match. This serves as a record for humans as well, and they might be confused having to decide which toml to put the binary dependency in.
(i dont feel strongly about this)
There was a problem hiding this comment.
I think if something is used by multiple packages, then it should go in workspaces' metadata. I think allowing users to specify it in multiple packages promotes some sloppy copy/pasting of configurations. I'd like for this to be well defined. I'd opt to leave it as is but if you feel stronger about this I can make the change since the benefit of your suggestion is that users can see more relevant information about their package.
PS: Ultimately, my hope is that the need for this functionality will some day go away (if Cargo ever figures out what it wants to do about these things).
There was a problem hiding this comment.
I think the right answer here depends on the expected mental model. If users associate the binary dependency with an individual sub-crate, then it might make more sense for this setting to permit duplicates, since you might have multiple crates using the given binary dependency. If they view it as boilerplate for raze which is mostly independent of the subcrate in question then it might make more sense to prohibit duplicates (though, in that case I'd argue it ought to be required to be in the workspace root toml).
I still don't feel strongly about this though, so final decision up to you (though I'd recommend trying to find a place to document this restriction in the README if it isn't already documented).
There was a problem hiding this comment.
I've updated the documentation. I do think you're instincts are toward the better overall outcome here, but I would rather keep the functionality of binary dependencies very minimal and find a way to remove it. Maybe I'll try to drive some RFCs forward in cargo to try and get this resolved.
Anyway, I think it's a much smaller change to make interactions here more elegant and I'd love to find out that people feel strongly enough about it to make a PR for it 😄
|
@acmcarther two open questions for you but otherwise ready for your eyes again. |
|
@acmcarther I think I've addressed all the open conversations. Please let me know if I missed anything or if you have any other feedback 🙏 edit: Also, ready for review. |
|
LGTM |
This change enables settings to be parsed from Cargo metadata to take full advantage of the changes introduced in #274. This also improves upon the changes in #276 to allow users to specify
CrateSettingsto be specified in the relevant packages when dealing with workspace projectsThis also contains updates to the top level README. A rendering can be found here
Blocked by #306