`cp`: make more types public and add more documentation (for nushell) #5152

tertsdiepraam · 2023-08-11T17:53:27Z

Part of #5088.

I've removed the Source, SourceSlice, Target and TargetSlice type aliases, because I think those obfuscate the public API. I want to make it as easy as possible for the nushell people to understand how to use our types and functions, without having to dive into the source code. For the same reason, I've documented every field of the Options struct with the arguments that change the behavior. Various other docstrings are added or improved too.

While the Attributes struct is pub, nu won't be able to construct it yet. We'll have to discuss whether they want to do their own parsing there or that we should do it (i.e. should we make all the fields pub or should we provide some functions to set the values).

Attributes can now be constructed either:

directly with a struct literal,
using the Attributes::parse_iter method which takes an iterator of strings,
or with the constants Attributes::{ALL, NONE, LINKS}

This PR also cleans up attribute parsing in general. It should therefore receive additional attention in code review. Those changes are in a separate commit.

@dmatos2012 I figured you weren't actively working on this anymore, so I picked it up. If you have a branch with changes ready, let's discuss how we can get the best of both versions.

@fdncred Does this look like something you can work with? I'd be happy to help out with the PR on the nu side.

Some final notes:

cp is particularly well-suited for this purpose with a clear single function, a good Options struct and Error type.
It's nice that nu does not need to do with UResult yet. That might be something to tackle later.
There are more public functions and types than necessary. To get better docs, we might need to restrict some of the things we currently expose.
If nu starts using this, that puts a restriction on uutils to not change the API arbitrarily. That's something we haven't had to deal with up until now. At the moment, I don't really think we can guarantee real stability, but on the other hand, the current API makes sense, so I don't see any reason to change it in the near future.

github-actions · 2023-08-11T18:50:14Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail-2/inotify-dir-recreate

fdncred · 2023-08-11T21:00:30Z

Thanks @tertsdiepraam for putting up this PR to support us. I'm excited to see where this leads. I'll try to look at it soon, maybe this weekend.

dmatos2012 · 2023-08-12T09:55:08Z

Hi @tertsdiepraam , first of all the hard work. I have indeed been working on this on the background, but I go slow since I dont have much time available, plus being still not a seasoned rust developer. But I try to work on it a bit certain days and make progress.

please see my branch with some extra implementation taken from the uu_cp branch @fdncred created, and trying to use the approach we suggested in #5088.

So couple of things

As you can see in the PR linked, I had to manually make lots of things public, which are not meant to be, but otherwise it seems way more difficult. Particular pain problem was implementing Attributes in nushell, since the uutils implementation makes use of lots of private functions, see here, with attribute.try_set_from_string(), attribute.max() , Attribute::default(), etc, and all of Attributes field to be public as well. I think implementing this ourselves would seem a bit more complicated, as those functions do a lot already. If we could sort out this part I think it would greatly help.

Slightly less relevant but important:

I have been also trying to port some of the tests from here to help with the implementation, but I think there are like >100 tests, so porting one by one manually takes some time. But I have ported like 8 or so and most of them pass, but then some of previous nushell ones fail. Right now, from nushell test suite + the ones I have added, I have 16 passing, 12 failed.
Implementation
Right now, the code is absolutely not ready for a PR, because I have just been wanting to implement the argument parsing, and its quite repetitive, but its pretty good to showcase where things can be changed and improved to allow easier codebase to maintain for the new uu_cp in nushell.

There might be some things here that are not strictly related to your PR, but it might help us identifying where we should help each other more, and how to go about it in the future!

tertsdiepraam · 2023-08-12T10:06:15Z

That PR looks great! I think we can combine that with this PR.

Two thoughts:

I don't think nushell has to care about perfect compatibility, so we can just make a simple non-compatible parser.
In the spirit of "crawl, walk, run", I think we can omit the Attributes for now and get a simple implementation in first.

Do you think you can simplify your branch so that we can turn it into a PR for nu? Just take all the arguments that are easy to implement and leave the rest as TODO.

fdncred · 2023-08-12T13:06:26Z

@dmatos2012 Wow, great work! Thanks so much for putting in the time and effort! I'm perfectly happy to close my prototype PR in favor of yours if you'd want to submit it to nushell after some minor changes.

I'm pretty much in the same camp as @tertsdiepraam. Let's do the crawl, walk, run strategy for now and simplify, marking things as TODO that aren't currently supported like Attributes.

I lean towards not being 100% GNU parameter compliant, but it would be nice if we can offer the same things but with slightly different flags/parameters at some point. For now, I think it would be great to get a PR up in nushell with all the cp tests this code supports.

tertsdiepraam · 2023-08-15T10:41:32Z

@dmatos2012 How shall we proceed? I can probably put up a PR based on your branch in the next couple of days or do you want to do that?

@fdncred If we want to get this in the next nushell release, what us our deadline? If the next release is soon, we might have to aim for the next one, since we'll have to do a new release of uutils for this to work too (since the changes to uu_cp need to be on crates.io).

Edit: Another issue is MSRV. We currently have 1.64 and nushell seems to have 1.60 as MSRV. What is nushell's MSRV policy?

dmatos2012 · 2023-08-15T10:57:30Z

Hey @tertsdiepraam , I was thinking on still doing it, I have made some progress by adding more tests, see latest commit, but I want to add all the other tests to cover all the flags as @fdncred mentioned.

I do not intend to add more flags that are there. I think by end of this week it should be possible to have it ready, but yeah can only work on it certain nights, of course only after my day job.

Its only some extra tests, and checking the use of flags is ok for the Nu people as well. One key thing that I am currently missing is how to get the Attributes to the Options ? I know we said we wont support it yet, but its still needed in the Option{} struct, so if I am not able to construct it, then I cant pass it on the fn copy.

Let me know your thoughts :)

tertsdiepraam · 2023-08-15T11:00:09Z

Excellent! I'll leave it to you then. In the meantime, I'll update this PR with a solution for Attributes. I think the solution is to extract all the attribute parsing into a function that we expose for nu.

fdncred · 2023-08-15T12:33:31Z

@tertsdiepraam Here's some info.

Our next release is August 22nd. If we don't make this release, the next one is fine. No pressure. People have families, jobs, circumstances, etc.
MSRV - We have 1.60 in our Cargo.toml but we don't really keep that up to date very well. What we try to do is stay 2 rust versions behind. This way, it gives package managers time to update their rust versions and we can have the widest availability possible for nushell. So, right now, we're on 1.69.0. In about 10 days, when rust moves to 1.72.0, we'll move to 1.70.0.
I'm still good with @dmatos2012 doing the PR. Looks good so far. Once the Attributes change gets up, it sounds like we'll have the changes we need from uu_cp.
I won't want to squeeze this change in at the last moment. Since we're about a week out now, I'm guessing that this feature will slip to the next release. I'd like to have a week or so before the release for real-world usage and testing. I hope y'all are ok with that too.
I'm so excited ( I'll quit saying this at some point 🤣 )

tertsdiepraam · 2023-08-15T12:39:01Z

Alright, let's aim for the release after the next one then. I'm glad that MSRV won't be an issue. Thanks for the info!

tertsdiepraam · 2023-08-15T12:48:20Z

Re: Attributes

Here's an interesting consideration: do you want the attributes to be given as a nu list?

I.e. should cp be used like this:

cp --preserve [ownership links] a b

or like this:

cp --preserve=ownership,links a b

Or should both be supported? And if the first is chosen, is --preserve ownership allowed as a shorthand?

fdncred · 2023-08-15T12:52:48Z

My first choice would be to use a nushell list, keeping things nushell-y. I'd also be ok with the shorthand.

Clean up the parsing of the attributes to preserve. There are several improvements here: Preserve now uses `max` from Ord, the `max` method is now called `union` and does not mutate, the parse loop is extracted into a new function `parse_iter`, the `all` and `none` methods are replaced with const values. Finally all fields of Attributes are now public.

cakebaker

Looks good, though I can't judge whether these changes meet the requirements of nushell.

fdncred · 2023-08-18T13:20:57Z

@dmatos2012 not trying to rush you, just checking in on your progress. wondering if you've been able to test with these latest changes to attributes?

dmatos2012 · 2023-08-18T22:08:30Z

@fdncred as far as I can tell, the changes @tertsdiepraam made are perfect for us. Now with Attributes public with the public parsing, along with the Options it seems that we dont need anything else from uutils at the moment. This is of course, following the crawl approach.

I have implemented the parsing, but still writing tests to confirm! I have tried manually some of hte attributes and everything looks ok so far!

Once I submit my PR to nushell (somewhere this weekend maybe?), we can keep adding stuff and going from there. Apologies for the time im taking, but well some other things going on as well and limited time.

Thanks!

fdncred · 2023-08-18T22:22:40Z

Once I submit my PR to nushell (somewhere this weekend maybe?), we can keep adding stuff and going from there. Apologies for the time im taking, but well some other things going on as well and limited time.

Sounds good. Thanks for the update. No apologies necessary.

@tertsdiepraam What's the plan to land this PR and do a release? It appears that things are working good. Although I haven't seen the PR. Do you want to wait until there's a nushell PR so we can get some more testing in to make sure this PR is solid or are you ready now? Also, I'm not sure of your release schedule/cadence.

tertsdiepraam · 2023-08-18T22:37:30Z

@fdncred My initial plan was:

Get a draft PR on nushell working with passing tests
Land this PR
Do a uutils release
Land the nushell PR

However, I think we can just land this PR because it's fairly straightforward. If there's more to do we can open more PRs :)

Our release cadence is roughly 6 weeks, but it's not fixed. The next one would be due in roughly 2 weeks. I wouldn't mind releasing earlier (but the other maintainers would have to agree).

fdncred · 2023-08-18T22:47:47Z

Thanks for the info and for this PR! We have a release on August 22 and then one 4 weeks after that. So, your cadence will probably fit within ours. We appreciate your support and help.

dmatos2012 · 2023-08-22T22:36:44Z

Hi @fdncred and @tertsdiepraam I have made draft PR on nushell. Check it out, and let me know your thoughts. Thanks for the patience, but yeah decided to just do this rather than wait to make it "nicer" !

fdncred · 2023-08-22T22:55:45Z

Thanks @dmatos2012. Already skimmed it and made a brief comment.

@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>

tertsdiepraam mentioned this pull request Aug 11, 2023

allow a deeper integration with nushell #5088

Open

cp: make more types public and add more documentation

ffa08f4

tertsdiepraam force-pushed the cp-prepare-for-nu branch from f686ceb to ffa08f4 Compare August 11, 2023 17:56

tertsdiepraam requested review from cakebaker and sylvestre August 16, 2023 22:46

cakebaker approved these changes Aug 18, 2023

View reviewed changes

tertsdiepraam merged commit 9e3d93e into uutils:main Aug 18, 2023

dmatos2012 mentioned this pull request Aug 22, 2023

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

Merged

4 tasks

tertsdiepraam mentioned this pull request Sep 20, 2023

mv: expose main functionality for nushell #5293

Closed

tertsdiepraam mentioned this pull request Sep 20, 2023

rm: expose main functionality for nushell #5294

Closed

Uh oh!

cp: make more types public and add more documentation (for nushell) #5152

cp: make more types public and add more documentation (for nushell) #5152

Uh oh!

Conversation

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

Uh oh!

Uh oh!

github-actions bot commented Aug 11, 2023

Uh oh!

fdncred commented Aug 11, 2023

Uh oh!

dmatos2012 commented Aug 12, 2023

Uh oh!

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

Uh oh!

Uh oh!

fdncred commented Aug 12, 2023

Uh oh!

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

Uh oh!

Uh oh!

dmatos2012 commented Aug 15, 2023

Uh oh!

tertsdiepraam commented Aug 15, 2023

Uh oh!

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

Uh oh!

Uh oh!

tertsdiepraam commented Aug 15, 2023

Uh oh!

tertsdiepraam commented Aug 15, 2023

Uh oh!

fdncred commented Aug 15, 2023

Uh oh!

cakebaker left a comment

Choose a reason for hiding this comment

Uh oh!

fdncred commented Aug 18, 2023

Uh oh!

dmatos2012 commented Aug 18, 2023

Uh oh!

fdncred commented Aug 18, 2023

Uh oh!

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

Uh oh!

Uh oh!

fdncred commented Aug 18, 2023

Uh oh!

dmatos2012 commented Aug 22, 2023

Uh oh!

fdncred commented Aug 22, 2023

Uh oh!

Uh oh!

`cp`: make more types public and add more documentation (for nushell) #5152

`cp`: make more types public and add more documentation (for nushell) #5152

tertsdiepraam commented Aug 11, 2023 •

edited

Loading

tertsdiepraam commented Aug 12, 2023 •

edited

Loading

tertsdiepraam commented Aug 15, 2023 •

edited

Loading

fdncred commented Aug 15, 2023 •

edited

Loading

tertsdiepraam commented Aug 18, 2023 •

edited

Loading