Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Behave more like other cargo commands in a workspace context #1170

Open
quasi-coherent opened this issue Oct 4, 2024 · 5 comments
Open

Behave more like other cargo commands in a workspace context #1170

quasi-coherent opened this issue Oct 4, 2024 · 5 comments

Comments

@quasi-coherent
Copy link

quasi-coherent commented Oct 4, 2024

Feature Description

... Or at least more comprehensive examples/documentation for different cargo workspace scenarios?

The examples seem just a bit more than trivial, and the documentation is kind of brief on this topic. An aside though: the documentation is fantastic in general, as is this project, so thank you. I look forward to using this by default when I understand it better.

Describe The Solution You'd Like

Change the behavior of workspace? Maybe change its name? Expand documentation and examples at least?

There are a few closed issues that touch on this, but not in a satisfying way. Here's one where the behavior of cargo make in a workspace is said to be by design. But it's the opposite of what I'd expect, seemingly a common trap, and because of that to do some things seems odd and others appear to be impossible, at least for someone like me with only introductory experience.

In the linked issue, you state that workspace = true is convenient because it runs, e.g., cargo b for every member of the workspace. At least for me though, the only things I would want to run for literally every workspace member already do that by default or with a flag: cargo b without a flag builds default-members, cargo clippy --workspace, cargo fmt (unqualified) are on the whole workspace. You have to specify cargo b -p a-specific-crate to change that.

It's probably common to have a default-members be the libs/bins that are only dependencies of other members (maybe they're naturally more "service" code or something). Or, another probably common thing is to declare workspace members as workspace dependencies. Or yet another common thing is to have a façade crate at the root that collects/exports public things from other members (like sqlx, axum, clap, ...). In all of these scenarios, cargo c will check the whole workspace (or an intentional subset of it) all at once in a unified way. Having workspace = true and a task that runs cargo c will iterate one-by-one over every member, running that individually; a task that runs cargo c --workspace actually does exactly the same thing... So workspace = true is very different from --workspace for cargo c (and build, and clippy, ...).

But that's for those commands, which I'm gathering are special in some way because it appears they're not expected to be an actual task defined in a workspace member's Makefile.toml. On the other hand, that is expected of a task invoking the sqlx command: with workspace = true and a command that runs cargo sqlx [OPTIONS], it starts with the first member and 404 exits saying that task can't be found. But of course it can't, because that command is completely irrelevant for that crate. Why would I need to define even a no-op task?

So, confusingly, I have a task in my root Makefile.toml:

[tasks.sqlx-check]
workspace = false # <- the confusing part
command = "cargo"
args = ["sqlx", "--prepare", "--check", "--workspace"]

Otherwise, do I have to have a Makefile.toml with a [tasks.sqlx-check] for every member and for all but the ones I care about, it does nothing? In any case, it would happen that workspace = true would have the exact opposite effect of what that's trying to do. The workspace members share a cache of "stuff" in the root and the command is creating it; running the task like workspace = true does would create a separate cache for each member.

In fact, in one workspace I have, where there are fifteen members of it, I only really do workspace-level things (or at least on default-members), yet I think I only would like to have workspace = true for one task out of ~25 that's on two of the fifteen members. But that's not working. These two are responsible for code generation for the workspace, and each have a local Makefile.toml with a task gen that does its specific thing; this root task

[tasks.gen]
workspace = true
env = { "CARGO_MAKE_WORKSPACE_INCLUDE_MEMBERS" = ["gen/thing", "gen/other-thing"] }
run_task = { name = "gen", fork = true } # something else? only guessing this from docs

ignores the INCLUDE directive, because the first thing it does is it tries to run the task in the first member and 404 exits saying that task can't be found. And like above, of course it can't be found because, as was the case for it with the sqlx task, a gen task is also completely irrelevant for that crate.

I don't know of a property for the task table that would do something like -p does for cargo commands, where it will run a thing for only that package and not the workspace. So where I'm sitting, my option is to have a task that changes the working directory and then calls cargo make from the member in a @shell script or something. Which obviously is a complete hack and can't really be the way it's intended to work -- calling cargo make from within a process created by calling cargo make. Or I guess I could write that task as a script that calls cargo make --makefile ... twice. I don't know, I must be missing something.

Anyway, I imagine the suggestion in the title is extreme: workspace = false runs tasks on default-members, workspace = true runs a command from the root with --workspace (that doesn't even make sense most of the time, I imagine), there's a new option package = .../packages = [...] that works over one/a list of members. It's hard to reconcile that with member Makefile.toml files (though I do not see any way right now to even use such a thing). But calling the option workspace conveys the exact opposite of what it actually does in my experience so far. So maybe the feature request is some version of:

  1. Rename workspace? Don't make it the default? I honestly can't think of a scenario in a workspace where setting default_to_workspace = false wasn't the first thing I did. Or maybe advertise a time where you would actually want that? Which leads to maybe the most realistic feature request...
  2. Expand the documentation? There's not very much on this that's practically useful. Examples do not really make sense, at least to me, have no comment/explanation, and they're contrived anyway. I currently am out of ideas for the codegen example above because I can't find anything even peripherally related in issues or in documentation.
@sagiegurari
Copy link
Owner

thanks for the feedback.
just short history leason, cargo make provided workspace support before all those cargo tooling.
there was no way to run fmt for all members and such which is part of the reason i created this tool.

than the obvious happened, as you stated, people wanted to run something in the workspace root and not for the members which is why i called it workspace but maybe the name isn't great after all....

by default the mindset is, if you have a workspace and you want to run test, you want to run test for every member and thats the default behavior.
on the otherhand plain root level tasks not related to a member are a bit more rare so they are not the default.

having said that, if i remember there is a way to change that behavior globally in the config section.

so my guess going forward, what you really want is improved docs? or do you still feel something more is needed?

if its docs only, would you like yo take a shpt at it?
while keeping in mind that readme is generated from content.md and nav.md

@quasi-coherent
Copy link
Author

Thank you for your quick reply, @sagiegurari! I definitely expected something like that to be the impetus. I haven't been playing close attention, but it does feel like it's relatively recent that the workspace is more common.

With tests too, there's of course cargo t --workspace, but I usually have one crate that is a member of the workspace, where all tests for all members live. cargo t -p the-test-crate runs all the tests. Or another workspace I have goes the façade crate route, where the root Cargo.toml is a package itself, and then tests/ in root behave like normal. I don't know if either approach is clearly/obviously "the way" to do it.

But I'd be happy to consider the feature implemented if there was just more clarity in the documentation and in the examples for "real world" workspace + cargo make setup. And I'd be happy to contribute that! But there's one problem. I still don't know anything about what that is... I'm still as confused as I was when I gave the two "real world" examples from my own experience above.

I'm not sure I would think of myself as knowledgeable enough to contribute documentation if I knew what the right strategy is supposed to be in those cases, but I might not have opened this issue if I did.

@sagiegurari
Copy link
Owner

would you like to give an example repo of yours, the problem and expected outcome?

@quasi-coherent
Copy link
Author

Sure! It's not something I can share directly, and there's a lot going on in the actual workspace, but I can try to come up with a minimally representative example later today or this weekend.

@quasi-coherent
Copy link
Author

would you like to give an example repo of yours, the problem and expected outcome?

Hi again! Just wanted to say I haven't disappeared. I got something that works to an acceptable definition of "works" and then had to change gears. I still have some outstanding questions, so I'll spend some time just as soon as I can and get a good enough example up. Thanks again for your attention!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants