run: Process::new() should take &[Str] instead of &[~str] for args

[lilyball]

Fixes #7928.

As a side-effect, this no longer clones all the arg strings on unix.

[lilyball]

@graydon: Upon thinking more about the ugly code in with_argv I'm no longer convinced that my replacement is safe. Specifically, as_c_str may actually copy the string if it's not null-terminated, and provide a pointer to that copied string. I suspect I may have to go ahead and use my new into_owned() so I can then use as_bytes_with_null() instead, although this will mean copying strings even if they were null-terminated to begin with.

Alternatively I could reimplement as_c_str() but have it return (Option<~str>, *libc::c_char) instead, which would let me hold onto the temporary if one was constructed.

Thoughts?

[lilyball]

@graydon: I deleted your r+ comment due to the above issue.

[lilyball]

@graydon: r? I fixed with_argv to only copy arguments when necessary.

[graydon]

Needs a rebase. Otherwise r=me, yeah. This is something I thought wouldn't be a problem at first since you were asking about a code change when the incoming values were all ~str and then I overlooked that later you were using it as a way to abstract it to take Str. Sorry.

[lilyball]

r? @graydon

[graydon]

The bad news is that we just (today) agreed to take the patch that removes the trailing nulls from slices and sets their length back to always cover only valid chars. This means that (a) you'll always need to copy and (b) if you use that probe-one-past-the-end code you copied and pasted in here, it'll start causing memory faults.

Sorry about this mess. Confusion around the trailing nulls is exactly the thing we are trying to get rid of, by removing the feature.

[lilyball]

Holding this patch until #8296 lands, at which point I will rewrite this on top of it.

[lilyball]

Rewritten on top of #8296. r? @graydon

[lilyball]

@graydon There was one error in librust, which I didn't catch because I only ran the libstd tests instead of doing a full build. My bad. r?

[lilyball]

Rebased yet again.

[catamorphism]

@kballard Needs rebase (and the test failure may be legitimate, I'm not sure)

[lilyball]

Rebased. Don't understand the test failures though. Nothing I did should affect Windows linkage, or should add calls to Windows functions that weren't previously used.

[lilyball]

I have no idea what's causing the Windows failures:

error: linking with `g++` failed with code 1
note: g++ arguments: -Lc:\bot\slave\auto-win-32-nopt-t\build\obj\i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin -m32 -o i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra-a7c050cfd46b2c9a-0.8-pre.dll i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o -Lc:\bot\slave\auto-win-32-nopt-t\build\obj\i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin -lstd-6c65cf4b443341b1-0.8-pre -Lc:\bot\slave\auto-win-32-nopt-t\build\obj\.rust -Lc:\bot\slave\auto-win-32-nopt-t\build\obj -shared -lmorestack -lrustrt
note: i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8d83c): undefined reference to `GetCurrentProcess'
i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8d963): undefined reference to `DuplicateHandle'
i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8db74): undefined reference to `DuplicateHandle'
i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8dd85): undefined reference to `DuplicateHandle'
i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8eb67): undefined reference to `CloseHandle'
i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8eb79): undefined reference to `CloseHandle'
i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8eb8b): undefined reference to `CloseHandle'
i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8ec50): undefined reference to `CloseHandle'
i686-pc-mingw32\stage1\bin\rustc\i686-pc-mingw32\bin\extra.o:fake:(.text+0x8f031): undefined reference to `CreateProcessA'
collect2: ld returned 1 exit status

[lilyball]

The referenced calls are all used in spawn_process_os(). The only change I made to that function was changing the type of the args arg from &[~str] to &[S] where S: Str.

[catamorphism]

@kballard Needs a rebase in any case.

[lilyball]

@catamorphism I can rebase it yet again, but that won't solve the windows problem. My best guess right now is that there's something going on here where the fact that it's generic means that libstd isn't actually linking against the FFI functions (because it contains the AST for the function instead of the actual compiled executable code), and then when libextra tries to call it, it hits the undefined symbols.

[lilyball]

It appears that process spawning has changed significantly. I'm going to re-write this PR on top of the new approach, but I don't know if the Windows issue will continue to happen. I wish I had some way of testing without going through the full r+ process.

[lilyball]

Looks like the new approach no longer defines the Windows FFI functions as part of the process stuff, instead relying on libuv to deal with it, so I hope that means it won't happen again.

[lilyball]

I have a rewritten version now that passes the stage2 libstd tests, but I'm running make check before pushing it.

[lilyball]

Don't review yet. Pushing here because I'm having a compilation issue.

[lilyball]

Ok this new commit passes make check. Sadly I had to leave run::process_status() as taking &[~str], because of the ICE in #8835.

[lilyball]

r?

[alexcrichton]

Sorry about std::run being in such flux and for causing all the bounces. The change to libuv and process bindings has now been reverted, so this shouldn't bounce any more. It needs a rebase though :(

[lilyball]

@alexcrichton Why was it reverted? Was that the source of the crashes on the builders?

Anyway, going back to the previous implementation of this is just going to bring back the Windows test failures. My guess right now is there's a problem with declaring extern fns inside a generic fn. I could try refactoring those declarations to be outside the generic fn, but I have no way of test Windows short of going through the whole r+ -> full make check process again.

[alexcrichton]

It was both the source of a massive regression on windows and I also suspect that it was causing all the segfaults.

For you extern fn inside a generic fn problem, I'd be interested in seeing if #8843 fixes that issue.

[lilyball]

Rebased. This presumably is going to require #8843 to work on Windows though.

[huonw]

@kballard #8843 has now landed.

[lilyball]

@huonw Great. This can be reviewed again now.

[lilyball]

Damn. #8843 didn't fix it.

[catamorphism]

Closing due to lack of activity. Please reopen a new pull request if you get the chance to work on this more -- thanks!

[lilyball]

It seems #9055 may be the cause of the Windows errors.