WIP: Refactor cargo commands #182

mackwic · 2017-10-02T17:51:04Z

I push this PR now for easier review + early discussions.

Todo:

extract the command building into their own testable module
create a Command bridge that we can inspect before calling the true Command builder
there is probably an opportunity to have a utils module, used by the cargo module
remove dead code
?

EDIT: Oh I forgot to add my new nick I got from the RustFest:

Sincerly,
The Test Dude

- Add a CommandBridge which is a testable command builder - Move Target struct in its own module - Add cargo::commands module which contains the logic of building cargo commands

mackwic · 2017-10-02T17:51:17Z

I am currently is quite puzzled by the compilation error if I uncomment the line 35 of cargo/commands.rs. I'll find a way to fix that quickly

mgattozzi

I always like a good refactor. I left some comments but this is definitely a good stab at making the commands more flexible. I'd also like to hear what @steveklabnik and @euclio think as well.

mgattozzi · 2017-10-02T20:30:08Z

src/cargo/command_bridge.rs

+        key_str.push(key.as_ref());
+        value_str.push(value.as_ref());
+
+        self.env.insert(key_str, value_str);


You can just use to_os_string

ooh, I missed that. I am not familiar with the OsStr(ing)? interface

People tend to miss it. The Standard Library docs are great! Usually I'll take a look before using something to see what's available, especially if I tend to do common patterns like conversions!

mgattozzi · 2017-10-02T20:32:46Z

src/cargo/command_bridge.rs

+    pub fn arg<S: AsRef<OsStr>>(&mut self, arg: S) -> &mut Self {
+        let mut os_string = OsString::new();
+        os_string.push(arg);
+        self.args.push(os_string);


Again to_os_string would be better. Since you keep turning them into OsStrings why not just accept something like Into<OsString> and calling into?

You are probably right. I was mirroring the interface of std::process::Command without thinking of making my life easier.

I'll test that.

mgattozzi · 2017-10-02T20:32:58Z

src/cargo/command_bridge.rs

+    pub fn new(name: &str) -> Self {
+        CommandBridge {
+            name: String::from(name),
+            .. Default::default()


This is such a neat trick!

Ah ! Now that you say it, yes it is. Very useful to replace factories in tests: implement Default and only specify the field you want to test.

I'll be leveraging this in some of my own stuff now!

mgattozzi · 2017-10-02T20:33:55Z

src/cargo/command_bridge.rs

+        self
+    }
+
+    pub fn stderr<S: Into<Stdio>>(&mut self, stderr: S) -> &mut Self {


Is this supposed to be Into<Stderr>?

honestly I just mirrored the std::process::Command interface. I imagine Stdio is a reference to the libc streams from stdio.h. I didn't took the time to look for Into<Stdio> implementors as I need to stay compatible with std::process::Command anyway.

If you have any suggestion, I'll take it

I looked at the docs. I think it's Stdio as the type so this is fine, but this also means you can put an StdIn where there should be an StdErr.

Yes this is legal. In this case on Linux it's equivalent to passing it /dev/null as stdin is often closed for writing (not always though, in this case it's a blocking read for eternity)

mgattozzi · 2017-10-02T20:35:16Z

src/cargo/command_bridge.rs

+    }
+
+    pub fn to_command(self) -> Command {
+        let mut command = Command::new(self.name);


I don't think name is the best choice to describe this. I think command would be better but that looks kind of weird here.

good point. command_name it is

Awesome thanks! :D

mgattozzi · 2017-10-02T20:37:14Z

src/cargo/command_bridge.rs

+            cmd.stderr(stderr);
+            // assert
+            // FIXME: is there is really no way to compare an Stdio instance ??
+            //assert_eq!(stderr as *const (), cmd.stderr.unwrap() as *const ())


You can turn them into RawFd which is basically just a c_int and compare them.
https://doc.rust-lang.org/std/os/unix/io/type.RawFd.html

Since it implements this it also implies that it implements the into version of it.

Oh ! Good catch ! Thank you ! :)

You're welcome :D

that's in std::os::unix though, what about windows?

I dug into the libstd source and couldn't find anything windows specific. This won't work on appveyor.

mgattozzi · 2017-10-02T20:40:40Z

src/cargo/commands.rs

+
+pub fn retrieve_metadata(manifest_path: &Path) -> CommandBridge {
+    let mut command = CommandBridge::new("cargo");
+    command.arg("metadata")


I'd prefer it to be a builder pattern stylistically so you can do:

CommandBridge::new("cargo") .arg("metadata") .arg("--manifest-path") .arg(manifest_path) .arg("--no-deps") .arg("--format-version") .arg("1")

If you change the function arg to arg(self) -> Self this enables that pattern. Builder patterns are great and this is essentially what you're doing here! :D

Yes, it would be more canonic. I'll change

mgattozzi · 2017-10-02T20:41:22Z

src/cargo/commands.rs

+        command.arg("--verbose");
+    }
+
+    //match target.kind {


Did you take this out?

still in the WIP :)

Ah got it cool

mgattozzi · 2017-10-02T20:47:54Z

src/cargo/commands.rs

+        .stdout(Stdio::null());
+
+    if is_verbose {
+        command.arg("--verbose");


Hmmm this would cause command to be consumed if you did what I mentioned. You'd have to do a let command = command.arg("--verbose");` here

mgattozzi · 2017-10-02T20:49:04Z

src/cargo/commands.rs

+            assert!(res.is_err())
+        }
+    }
+}


Is this whole file new because it got moved?

nope, the file is new. I extracted the logic of command building here and I want to unit test it

Even better. I'm all for more tests. Looking forward to seeing the end result :D

euclio · 2017-10-03T17:01:25Z

src/cargo/command_bridge.rs

+/// This temporary struct is used to inspect the generated command before calling the process::Command
+/// builder, as we can't inspect its structure and make assertions on it
+#[derive(Debug, Default)]
+pub struct CommandBridge {


Just another use case for rust-lang/rust#44434 😞

:( If only. Might need an RFC?

Yes, this is much needed. And std::{fs, io} stubbing. So much of that

mgattozzi · 2017-10-04T19:22:06Z

Hmmm tests are failing but it doesn't seem to be your fault (I think). These changes look great! :D

steveklabnik · 2017-10-16T18:21:41Z

I like this idea in general, but yeah, the tests seem to be failing. do they pass locally? Given that both travis and appveyor fail with the same message, I suspect that it's something in here...

mackwic · 2017-10-22T11:53:06Z

Sorry, not much time to work on it recently. I am still up to finish this PR, but I would understand if you close it. I can always work on it offline and re-submit it later, rebased from master.

steveklabnik · 2017-11-22T13:47:15Z

It happens! I go for a long time without having time too.

I am going to give this a close though, if you or anyone else wants to update it, please submit a new PR! Thanks :)

mackwic added 2 commits October 2, 2017 15:31

WIP: extract logic of cargo commands building

357cbb6

- Add a CommandBridge which is a testable command builder - Move Target struct in its own module - Add cargo::commands module which contains the logic of building cargo commands

WIP: check the manifest_path in its own method

4ddd184

mgattozzi reviewed Oct 2, 2017

View reviewed changes

euclio reviewed Oct 3, 2017

View reviewed changes

mackwic added 2 commits October 3, 2017 23:47

Feedback part1: be a real builder + rename field

5424f92

rustfmt the thing

cbb7a55

steveklabnik closed this Nov 22, 2017

WIP: Refactor cargo commands #182

WIP: Refactor cargo commands #182

Conversation

mackwic commented Oct 2, 2017 • edited Loading

mackwic commented Oct 2, 2017

mgattozzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgattozzi commented Oct 4, 2017

steveklabnik commented Oct 16, 2017

mackwic commented Oct 22, 2017

steveklabnik commented Nov 22, 2017

mackwic commented Oct 2, 2017 •

edited

Loading