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

Automatically add --manifest-path when -x and -C are combined #219

Closed
jaskij opened this issue Dec 29, 2022 · 3 comments
Closed

Automatically add --manifest-path when -x and -C are combined #219

jaskij opened this issue Dec 29, 2022 · 3 comments

Comments

@jaskij
Copy link

jaskij commented Dec 29, 2022

To preface: I'm not a cargo-watch user, but I've been in a discussion with one, helping them work around some issues where the sanest workaround is an out-of-tree build (TLDR: bind mounts in Docker on anything but Linux have longstanding performance issues, OP in the Reddit discussion cut their build time by something close to 20x).

What I proposed, as a solution to their issue, is a Dockerfile that looks like this:

RUN mkdir -p /build
WORKDIR `/src`
ENTRYPOINT cargo watch -C /build -x run --manifest-path=/src/Cargo.toml -- --your-arg

It seems reasonable to add that --manifest-path argument automatically if -C is present. Perhaps do not change the behavior of existing -x, but rather add a new flag, -X.

@passcod
Copy link
Member

passcod commented Dec 29, 2022

The current implementation of -C is actually aberrant, as it changes the workdir globally before doing anything. The intent was always to change it so the workdir was only changed for the command, so all the paths given on the command line and implied by defaults still make sense. As a result while the suggestion is appreciated I'm not going to introduce a new option to work around something that will go away soonish :)

Also off topic but I wouldn't recommend using both -x/-s and the -- command style, especially as complements to one another. It's an interesting pattern but only works incidentally, so it's probable it will break in the future, which is not great for scripts or commands baked into docker images.

Finally running cargo watch as PID 1 is not great at the moment, they might want to wrap a lightweight supervisor around it if they run into any issues! ...though I think ENTRYPOINT does this with a shell already in this syntax? I don't recall. Just something to keep in mind anyway.

@passcod passcod closed this as not planned Won't fix, can't repro, duplicate, stale Dec 29, 2022
@jaskij
Copy link
Author

jaskij commented Dec 29, 2022

As a result while the suggestion is appreciated I'm not going to introduce a new option to work around something that will go away soonish :)

It's a shame, since it seems like one of the very few possible workarounds for running cargo-watch in Docker with bind mounted source. Which isn't that unusual. But I understand trimming the cruft, and stuff that doesn't match the rest of the project.

Finally running cargo watch as PID 1 is not great at the moment, they might want to wrap a lightweight supervisor around it if they run into any issues! ...though I think ENTRYPOINT does this with a shell already in this syntax? I don't recall. Just something to keep in mind anyway.

Hah, good to know, I might pass it to OP in that Reddit thread.

@passcod
Copy link
Member

passcod commented Dec 29, 2022

I'll add some advice from this issue in the readme, that may help a bit.

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

No branches or pull requests

2 participants