rune-rs / rune

An embeddable dynamic programming language for Rust.
https://rune-rs.github.io
Apache License 2.0
1.7k stars 85 forks source link

modules: extend process module #763

Open VorpalBlade opened 1 month ago

VorpalBlade commented 1 month ago

This greatly increases the power of the process module:

Remains to be done:

This fixes #751

VorpalBlade commented 1 month ago

@udoprog Perhaps you can give early feedback on the direction I'm taking the process module?

I need to add support for feeding data to stdin for a child still, presumably this will be done via ChildStdin.

Similarly ChildStdout/Stderr should be expanded so that you can read from them. This will then allow talking to an "interactive" process if you want to.

I'm also not sure how you handle tests for things like this.

udoprog commented 1 month ago

Implementation looks good, documentation-wise I'd recommend copying and adapting the std::process documentation.

Can you make sure to add a Copyright notice to modules.rs since we've copied a fair bit of documentation? It seems to have been misplaced:

// Documentation copied from the Rust Project under the MIT license.
//
// Copyright 2014-2024 The Rust Project Developers
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// https://www.apache.org/licenses/LICENSE-2.0> or the MIT license <LICENSE-MIT
// or https://opensource.org/licenses/MIT>, at your option. Files in the project
// may not be copied, modified, or distributed except according to those terms.
VorpalBlade commented 1 month ago

Can you make sure to add a Copyright notice to modules.rs since we've copied a fair bit of documentation?

I actually copied from tokio, but it seems a fair bit of it is in turn copied from std. So I suggest adding both.

udoprog commented 1 month ago

Yeah, that's fine. Just add all the copyright notices one after another.

I'm also not sure how you handle tests for things like this.

Also realized I didn't mention this (sorry I'm a bit sick right now), but you can add code and mark it as no_run like you do with doc-tests in Rust.

udoprog commented 1 month ago

As for actual tests we run, I think you'll have to build proper integration tests (in e.g. crates/rune/tests) using tempfile for setting up temporary directories.

Might also be time to add a Path type if that's something you're interested in.

VorpalBlade commented 1 month ago

As for actual tests we run, I think you'll have to build proper integration tests (in e.g. crates/rune/tests) using tempfile for setting up temporary directories.

Well, I have my own (specialized) file system Rune API for my own program. It is more advanced than the rune-modules one (but also not general purpose). For obvious sandboxing reasons filesystem access (or process access) doesn't belong in std, but should be an optional module. The reason I bring it up, is that it does have a TempDir abstraction: https://github.com/VorpalBlade/paketkoll/blob/37a97123099c5793462391356e8844ce3a080db1/crates/konfigkoll_script/src/plugins/filesystem.rs

But it might not come to that, if we just work with pipes instead. However, being able to redirect stdout or stdin to/from files is an interesting functionality and cross-module integration, but I feel there is scope creep here now (I didn't plan to also make a new file system module!).

Might also be time to add a Path type if that's something you're interested in.

Hm, I generally work with camino Utf8Path (I have decided that supporting non-Unicode paths is out of scope for my project(s)) so I likely wouldn't have much use for the general purpose one that would be suitable for the file system module.

sorry I'm a bit sick right now

I wish you quick recovery (or whatever the idiomatic phrase in English is for this)

udoprog commented 1 month ago

I wish you quick recovery (or whatever the idiomatic phrase in English is for this)

Tack tack

VorpalBlade commented 1 month ago

I wish you quick recovery (or whatever the idiomatic phrase in English is for this)

Tack tack

Åh, hej. Svensk du med förstår jag.