use uutils uu_cp and uu_mv by fdncred · Pull Request #9463 · nushell/nushell

fdncred · 2023-06-17T18:29:25Z

Description

This PR is a Proof of Concept MVP for replacing our cp and mv commands with the uutils/coreutils version. In this PR, we add a new feature called nuuu to expose these commands. If you compile with cargo b --features=nuuu then the cp and mv you get are the coreutils version. You can see they're different by looking at their help. If you compile without the nuuu feature, then you'll get the current nushell's version of cp and mv.

Some additional notes and thoughts here

Copy (ucp)

        // MVP args
        // ucp [OPTIONS] SOURCE DEST
        // -r, --recursive     copy directories recursively [short aliases: R]
        // -v, --verbose       explicitly state what is being done (also adds --debug)
        // -f, --force         if an existing destination file cannot be opened, remove it and try again
        //                     (this option is ignored when the -n option is also used). Currently not
        //                     implemented for Windows.
        // -i, --interactive   ask before overwriting files
        // -g, --progress      Display a progress bar.

Move (umv)

        // MVP args
        // Usage: mv [OPTION]... SOURCE DEST
        // Rename SOURCE to DEST, or move SOURCE(s) to DIRECTORY.

        // Mandatory arguments to long options are mandatory for short options too.
        //   -f, --force                  do not prompt before overwriting
        //   -i, --interactive            prompt before overwrite
        // If you specify more than one of -i, -f, -n, only the final one takes effect.
        //   -s, --strip-trailing-slashes  remove any trailing slashes from each SOURCE argument
        //   -p, --progress               show progress bar
        //   -n, --no-clobber             do not overwrite an existing file
        //   -v, --verbose                explain what is being done

User-Facing Changes

Tests + Formatting

After Submitting

sophiajt · 2023-06-18T15:15:49Z

crates/nu-command/src/filesystem/uutil-notes.md

+    let s2 = "-h".to_string();
+    let args = vec![OsString::from(s1), OsString::from(s2)];
+    // Pass uucore::Args to app.uumain
+    uu_cp::uumain(&mut args.into_iter());


how does uumain read in stdin? Can we redirect it?

This is the first few lines from uumain

#[uucore::main] pub fn uumain(args: impl uucore::Args) -> UResult<()> { let matches = uu_app().try_get_matches_from(args);

So, it's reading the args passed in.

sophiajt · 2023-06-18T15:16:44Z

crates/nu-command/src/filesystem/uutil-notes.md

+    // Create uucore::Args somehow from nushell args
+    let s1 = "cp".to_string();
+    let s2 = "-h".to_string();
+    let args = vec![OsString::from(s1), OsString::from(s2)];


Makes me wonder if we instead want to create a From/Into for clap, so we can take the args and convert them using a converter that all the uu commands share.

Seems reasonable. We need to construct our arguments so that they're compatible uucore::Args. I think this is the trait definition for that. https://github.com/uutils/coreutils/blob/17830ad6f84583a3d552b360dece0c74d7efc2e3/src/uucore/src/lib/lib.rs#L155-L167

sophiajt · 2023-06-18T15:17:37Z

crates/nu-command/src/filesystem/uutil-notes.md

+    let args = vec![OsString::from(s1), OsString::from(s2)];
+    // Pass uucore::Args to app.uumain
+    uu_cp::uumain(&mut args.into_iter());
+    Ok(PipelineData::empty())


Same question for output. Here we're dropping whatever they give us back, but how do we convert the uumain's stdout to something nushell can use? or, more importantly, can we convert it to structured data?

My first thought was to start with commands like cp that don't really have structured output. The problem though is with error messages. We can catch parameter errors via our normal command argument parsing, but once we hand things off to blah::uumain() it does what it will do. This is the part we need to work with the uutils team on, having a way to hook the output.

You can see what uu_cp::uumain() is doing here https://github.com/uutils/coreutils/blob/17830ad6f84583a3d552b360dece0c74d7efc2e3/src/uu/cp/src/cp.rs#L651-L696

I think it's a good idea to somehow integrate with clap. It looks like 90% of what's going on in that cp.rs file I linked above is all clap/argument handling. So, we'd want to use that to our benefit somehow.

fdncred · 2023-06-30T16:17:35Z

There's a current known problem with mv documented in #9558. It has to do with how arguments are passed from nushell to uutils and globbing.

amtoine · 2023-07-01T11:15:25Z

my two cents on this 😌

i like the idea of relieving Nushell from the burden of all these complexe core utils
as a user, i do not mind what the implementation is coming from exactly => if we have the nice sugar of Nushell for the UX and the uutils as the backend, i'm all for it 👍

amtoine · 2023-07-01T11:16:16Z

tbh, if i had not explicitely compiled Nushell with --features nuuu for the experiment, i probably even wouldn't have seen the change 👌

amtoine

@fdncred
just a question about the Cargo.toml files, they have been regenerated automatically by cargo, right? 😮

fdncred · 2023-07-02T19:08:29Z

The changes to the cargo.toml files are two fold, (1) i tend to homogenize things, putting the nu- stuff together, adding new uu crates, and sorting crates and (2) the cargo/crates.io extension i use tends to reformat toml things.

But reformatting the toml is the least thing I'm concerned about in this PR.

fdncred · 2023-07-15T14:09:40Z

In an attempt to have a tighter integration with uutils/coreutils I've created this issue in their repo uutils/coreutils#5088.

tertsdiepraam · 2023-08-02T12:54:40Z

crates/nu-command/Cargo.toml

+# uu_cp = { path = "../../../coreutils/src/uu/cp", version = "0.0.19" }
+uu_mv = "0.0.19"
+# uucore = { path = "../../../coreutils/src/uucore", version = "0.0.19", features = ["fsext"] }
+uucore = { version = "0.0.19", features = ["fsext"] }


If fsext is necessary, that would be a bug in uutils. Does it still compile if you remove this?

I'll test this and let you know. I didn't add it because it was required. I just copy-n-pasted from somewhere. It could've been from JT's exploration of adding df a few months back.

tertsdiepraam · 2023-08-02T12:55:50Z

I've looked at this PR, but I posted my thoughts in the issue on uutils to avoid splitting the conversation: uutils/coreutils#5088 (comment)

dmatos2012 · 2023-08-05T10:02:28Z

So, I checked out this branch, and tried to run the {else} branch that required the private parts from uutils, and this is basically what its needed. The only part it would still be needed is

// Argument constants
mod options {
    pub const PATHS: &str = "paths";

but I thought this isnt strictly necessary since we could just copy paste into nushell given its just bunch of &str. Diff below (from uutils)

diff --git a/src/uu/cp/src/cp.rs b/src/uu/cp/src/cp.rs
index de9dd1c91..ac82f90f3 100644
--- a/src/uu/cp/src/cp.rs
+++ b/src/uu/cp/src/cp.rs
pub struct Options {
-    attributes_only: bool,
+    pub attributes_only: bool,
  // AND ALL THE OTHER FIELDS.OMMITED FOR BREVITY
}

@@ -765,7 +765,7 @@ impl Attributes {
        }
    }

-    fn default() -> Self {
+    pub fn default() -> Self {

impl Options {
    #[allow(clippy::cognitive_complexity)]
-    fn from_matches(matches: &ArgMatches) -> CopyResult<Self> {
+    pub fn from_matches(matches: &ArgMatches) -> CopyResult<Self> {

-fn parse_path_args(mut paths: Vec<Source>, options: &Options) -> CopyResult<(Vec<Source>, Target)> {
+pub fn parse_path_args(

-fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResult<()> {
+pub fn copy(sources: &[Source], target: &TargetSlice, options: &Options) -> CopyResult<()> {

diff --git a/src/uucore/src/lib/lib.rs b/src/uucore/src/lib/lib.rs
index 02724e1bf..593488276 100644
--- a/src/uucore/src/lib/lib.rs
+++ b/src/uucore/src/lib/lib.rs
-mod mods; // core cross-platform modules
+pub mod mods; // core cross-platform modules

I suppose the only big one would be the pub mod.rs in the lib.rs to expose the Options types. The two current tests from nushell still pass.

I guess this was more suitable on the uutils but its nicer to keep the conversation in the same place. If this is something you guys @fdncred @tertsdiepraam agree with as discussed here, then I could create the PR with this exact diff.

tertsdiepraam · 2023-08-05T10:17:29Z

@dmatos2012 My plan was a bit different: uutils/coreutils#5088 (comment)

The reason is that the double argument parsing would be a bit error prone and redundant. Also, I think nu does not need to match GNU's argument parsing exactly. If you agree with that plan you can pick it up if you want. I don't have a lot of time to work on it.

dmatos2012 · 2023-08-05T10:59:37Z

I apologize @tertsdiepraam if I misunderstood some of your comments, but basically what you mean is that what you would like is that everything is parsed from the nu side, and then only use the following exposed: struct Options and fn copy, and expose all the types from Options. That would be the same diff except for this part

impl Options {
    #[allow(clippy::cognitive_complexity)]
-    fn from_matches(matches: &ArgMatches) -> CopyResult<Self> {
+    pub fn from_matches(matches: &ArgMatches) -> CopyResult<Self> {

as we should handle the argument parsing in nu

Thus, once we have parsed what is source, target and options flags, then we can call the fn copy(sources: &[Source], target: &TargetSlice, options: &Options) from uutils. Hope its clear. And yes, I dont mind working on it, but want to make sure I properly understand in which way it will be achieved to not waste my time as well :)

The mentioned {'else} branch mentioned that I checked out is the commented part, and does not use the uumain() that would introduce some extra issues.
Thanks!

tertsdiepraam · 2023-08-05T11:09:48Z

Yes, that's indeed what I meant!

fdncred · 2023-08-05T12:54:22Z

Thanks @dmatos2012 for working on this. I'm interested to see how this will go. Hopefully cp will be no big deal but I'm a little anxious about nushell parsing parameters the way uutils wants to use them. However, I do think this is the right way to go, otherwise we wouldn't have spans we could use to report errors.

fdncred · 2023-08-24T12:00:36Z

closing in favor of #10097

@fdncred

)  # Description Hi. Basically, this is a continuation of the work that @fdncred started. Given some nice discussions on #9463 , and [merged uutils PR](uutils/coreutils#5152) from @tertsdiepraam we have decided to give the `cp` command the `crawl` stage as it was named. > [!NOTE] Given that the `uutils` crate has not made the release for the merged PR, just make sure you checkout latest and put it in the required place to make this PR work. The aim of this PR is for is to see how to move forward using `uutils` crate. In order to getting this started, I have made the current `nushell cp tests` pass along with some extra ones I copied over from the `uutils` repo. With all of that being said, things that would be nice to decide, and keep working on: Crawl: - Handling of certain `named` flags, with their long and short forms(e.g. --update, --reflink, --preserve, etc), and using default values. Maybe `-u` can already have a `default_missing_value`. - Should we maybe just support one single option `switch` flags (see `--backup` in code) as a contrast to the other named args. - Complete test coverage from `uutils`. They had > 100 tests, and I could only port like 12 as they are a bit time consuming given they cannot be straight up copy pasted. Maybe we do not need all >100, but maybe the more relevant to what we want. - Refactor this code Walk: - Non fatal errors on `copy` from `utils`. Currently it just sends it to stdout but errors have no span - Better integration An added possibility is the addition of `SyntaxShape::OneOf()` for `Named` arguments which was briefly mentioned in the discord server, but that is still to be decided. This could greatly improve some of the integration. This would enable something like `cp --preserve [all timestamp]` or `cp --preserve all` to both work. I did not want to keep holding on this, and wait till I was happy with the code because I think its nice if everyone can start up and suggest refactors, but the main important part now was getting it out the door, as if I take my sweet time this will take way longer 😛  # User-Facing Changes  # Tests + Formatting Make sure you've run and fixed any issues with these commands: - [X] cargo fmt --all -- --check` to check standard code formatting (`cargo fmt --all` applies these changes) - [X] cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to check that you're using the standard code style - [X] cargo test --workspace` to check that all tests pass - [X] cargo run -- -c "use std testing; testing run-tests --path crates/nu-std"` to run the tests for the standard library > **Note** > from `nushell` you can also use the `toolkit` as follows > ```bash > use toolkit.nu # or use an `env_change` hook to activate it automatically > toolkit check pr > ``` --> # After Submitting  --------- Co-authored-by: Darren Schroeder <343840+fdncred@users.noreply.github.com>

fdncred added 2 commits June 17, 2023 13:27

use uutils uu_cp

ec490d5

add some notes

d72f835

sophiajt reviewed Jun 18, 2023

View reviewed changes

fdncred added 7 commits June 21, 2023 12:32

MVP - working now

1676ee2

typo

77b7812

reformat comment

7df18f6

cleanup some clippy problems

701c296

typo

ac23370

Merge branch 'main' into uu_cp

bd2127e

add uu_mv

0b58da5

fdncred changed the title ~~use uutils uu_cp~~ use uutils uu_cp and uu_mv Jun 28, 2023

fdncred mentioned this pull request Jun 29, 2023

Provide a string that does not expand globs #9558

Closed

make uutils a feature named nuuu

6e20999

fdncred added 4 commits July 1, 2023 07:39

Merge branch 'main' into uu_cp

6a2ae78

Merge remote-tracking branch 'refs/remotes/origin/uu_cp' into uu_cp

57542c6

make arguments a bit nicer with into()

0baf7fb

experiments with cp

d0cad90

amtoine reviewed Jul 2, 2023

View reviewed changes

fdncred added 5 commits July 3, 2023 14:29

Merge branch 'main' into uu_cp

cd15b59

Merge branch 'main' into uu_cp

ea3937b

Merge branch 'main' into pr/fdncred/9463

bd88cb4

add the ability to handle our own errors

3293962

clean up and add a couple tests from uu

b2a73d7

fdncred mentioned this pull request Jul 15, 2023

Add nushell dependency analysis script uutils/coreutils#5051

Merged

fdncred mentioned this pull request Jul 15, 2023

allow a deeper integration with nushell uutils/coreutils#5088

Open

fdncred added 2 commits July 18, 2023 06:35

Merge branch 'main' into uu_cp

77c70c5

update main, add comments

298de6b

tertsdiepraam reviewed Aug 2, 2023

View reviewed changes

fdncred added 3 commits August 2, 2023 09:07

Merge branch 'main' into uu_cp

26cf764

Merge remote-tracking branch 'refs/remotes/origin/uu_cp' into uu_cp

afd4994

surgery

e5f614a

dmatos2012 mentioned this pull request Aug 22, 2023

use uutils/coreutils cp command in place of nushell's cp command #10097

Merged

4 tasks

fdncred closed this Aug 24, 2023

fdncred mentioned this pull request Jan 16, 2024

add uutils/coreutils commands into nushell #11549

Open

16 tasks

fdncred added the A:coreutils-uutils Changes relating to coreutils/uutils label Feb 3, 2024

fdncred deleted the uu_cp branch February 23, 2024 12:11

Conversation

fdncred commented Jun 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Copy (ucp)

Move (umv)

User-Facing Changes

Tests + Formatting

After Submitting

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdncred commented Jun 30, 2023

Uh oh!

amtoine commented Jul 1, 2023

Uh oh!

amtoine commented Jul 1, 2023

Uh oh!

amtoine left a comment

Choose a reason for hiding this comment

Uh oh!

fdncred commented Jul 2, 2023

Uh oh!

fdncred commented Jul 15, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tertsdiepraam commented Aug 2, 2023

Uh oh!

dmatos2012 commented Aug 5, 2023 • edited by amtoine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tertsdiepraam commented Aug 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmatos2012 commented Aug 5, 2023 • edited by amtoine Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tertsdiepraam commented Aug 5, 2023

Uh oh!

fdncred commented Aug 5, 2023

Uh oh!

fdncred commented Aug 24, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

fdncred commented Jun 17, 2023 •

edited

Loading

dmatos2012 commented Aug 5, 2023 •

edited by amtoine

Loading

tertsdiepraam commented Aug 5, 2023 •

edited

Loading

dmatos2012 commented Aug 5, 2023 •

edited by amtoine

Loading