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

Add `--with` option to cargo run #1763

Closed
wants to merge 2 commits into from

Conversation

Projects
None yet
5 participants
@vojtechkral
Copy link
Contributor

vojtechkral commented Jun 27, 2015

Implementing what I had in mind in #1726

There are likely imperfections in design / implementation, I'm opening this PR mostly to get feedback :)

Motivation / usage examples:

$ cargo run -w gdb
     Running `gdb target/debug/hello`
GNU gdb (GDB) 7.9.1
<snip>
(gdb) r foobar
Starting program: target/debug/hello foobar
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
Hello!
Arguments:
target/debug/hello foobar
[Inferior 1 (process 7568) exited normally]

$ cargo run -w strace -- -e trace=write {} foobar
     Running `strace -e trace=write target/debug/hello foobar`
write(1, "Hello!\nArguments:\n", 18Hello!
Arguments:
)    = 18
write(1, "target/debug/hello foob"..., 36target/debug/hello foobar
) = 36
+++ exited with 0 +++
@rust-highfive

This comment has been minimized.

Copy link

rust-highfive commented Jun 27, 2015

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jun 29, 2015

Nice idea! I'm somewhat uneasy about taking the unix-ism of {} == "self", but other than that this does seem like a pretty useful command.

Thoughts @rust-lang/tools?

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Jun 30, 2015

@alexcrichton Maybe I could argue it's std::fmt-ism, but honestly unix find command was my inspiration :)

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jul 5, 2015

☔️ The latest upstream changes (presumably #1783) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jul 6, 2015

Another alternative for doing this is something along the lines of xargs which has -I arg where arg is then replaced with the name of the executable (basically making the {} configurable)

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Jul 7, 2015

I could implement that, not problem, but I'm a little concerned about an -I option only being relevant for the -w option...

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Aug 25, 2015

☔️ The latest upstream changes (presumably #1933) made this pull request unmergeable. Please resolve the merge conflicts.

@kanru

This comment has been minimized.

Copy link

kanru commented Nov 17, 2015

@vojtechkral hi, are you still working on this?

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Nov 17, 2015

@kanru Well I haven't looked at this in a while because this PR hasn't really caught much attention :) but I could resolve the conflicts I suppose.

@kanru

This comment has been minimized.

Copy link

kanru commented Nov 17, 2015

Just really want this feature so if you were not working on this, I'm gonna ;)

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Nov 17, 2015

@kanru I'll resolve the merge conflicts ASAP (wchich seems easy enough) and test if the implementation still works. Apart from that, what would you propose to do? If you have some comments/criticism concerning implementation, feel free to let me know.

@kanru

This comment has been minimized.

Copy link

kanru commented Nov 17, 2015

@vojtechkral Instead of adding -I option I would add --with-args which specify the arguments for the --with option. So your example will become cargo run -w strace --with-args '-e trace=write' foobar. It's more intuitive, IMO.

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Nov 17, 2015

@kanru All of the arguments following -- are supposed to go to strace, refer to the line cargo outputs:

Running `strace -e trace=write target/debug/hello foobar`

@vojtechkral vojtechkral force-pushed the vojtechkral:run-with branch from eb5922f to 2dad9ee Nov 17, 2015

@kanru

This comment has been minimized.

Copy link

kanru commented Nov 18, 2015

@vojtechkral but foobar is actually the argument to hello. So one would normally do

cargo run foobar

When one wants to debug:

cargo run foobar -w strace --with-args '-e trace=write'

or

cargo run -w strace --with-args '-e trace=write' -- foobar

And --with-args is optional. What do you think?

@vojtechkral

This comment has been minimized.

Copy link
Contributor Author

vojtechkral commented Nov 18, 2015

but foobar is actually the argument to hello.

Yes, but only because strace passes it to hello. cargo -w doesn't pass any arugments to hello at all, it actually can't since it doesn't run the hello process at all, it runs strace instead (in this example) and passes arguments to strace and then strace runs hello and passes some arguments to it. Same with gdb.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 4, 2015

☔️ The latest upstream changes (presumably #2192) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Feb 26, 2016

This has been inactive for some time now, so I'm going to close, but feel free to resubmit with a rebase!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.