-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
use uutils/coreutils cp command in place of nushell's cp command #10097
Conversation
Forgot: For the live of me I could not figure out how to add a |
Thanks for working on this @dmatos2012!! We appreciate your efforts. Can you try to get the CI green? Just comment out the env test if that's needed. I think it'll be easier to continue with it being green. At first glance, I just go, "wow, look at all the parameter checking code". 98% of all this code is just parameter checking. In the coming days, I'm sure the core-team will look into this closer and suggestion changes. |
I'm also getting these errors. Not sure if it's because of a preliminary BackupMode attempt or if it's because we need @tertsdiepraam to make something else public?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent! I'll review this more from a design and integration perspective.
I think choosing to make --backup
not take any arguments is a pretty good idea. In general, I'd say that the goal here is a simplified, more nushell-style interface to cp
. We don't necessesarily have to take our inspiration from GNU either. For example, FreeBSD cp
has only switches and no arguments with values and that seems to work all right for them.1
They also have just -p
, for example, which just preserves all attributes.So, I think we could even decide to make --preserve
and --update
also just switches for instance.
Another good idea in here is that you make conflicting arguments just error. In uutils, I begrudingly make everything override because that's GNU's behaviour, but here we're free to just make things error :smile
Footnotes
progress_bar: progress, | ||
}; | ||
|
||
// For enabling easy `crawl` of cp command, we need to strip current directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does cp
not support absolute paths? That would be a pretty serious bug!
crates/nu-command/Cargo.toml
Outdated
@@ -90,6 +90,8 @@ toml = "0.7" | |||
unicode-segmentation = "1.10" | |||
ureq = { version = "2.7", default-features = false, features = ["charset", "gzip", "json", "native-tls"] } | |||
url = "2.2" | |||
uu_cp = { path = "../../../coreutils/src/uu/cp"} | |||
uucore = { path = "../../../coreutils/src/uucore"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this used for? I hoped we could do this without directly depending on anything but uu_cp
(even though uucore
would still be a transitive dependency).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
} | ||
}; | ||
} | ||
// // uucore::error::set_exit_code(EXIT_ERR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remember to clean this up :)
// ShellError | ||
// _ => uucore::macros::show_error!("{}", error), | ||
_ => { | ||
return Err(ShellError::GenericError( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could look into integrating UResult/UError
with the nu system for future utils to make this easier.
}; | ||
|
||
/// Returns tuple of (Source paths, Target) | ||
pub fn parse_path_args( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say we're being too compatible here. I think the way to go is to make it super simple: parse the paths, pop the last one and check that both are non-empty. We can then ignore --target-dir
It's both, sort of. I think @dmatos2012 used the wrong path to import it, because it should be public, since |
Just chiming in again on these comments.
Agreed.
Agreed.
Agreed. Another thought I keep having is that nushell does not have to support every parameter uu_cp supports. In fact, the nushell maintainers, have done their best to not have dozens of parameters on internal commands. I assume that some of the core-team will not approve this PR with, if I counted correctly, 29 parameters. I'm not saying that to be critical, but I'm just predicting that we'll want less, not more. The trick will be to decide which parameters we'll want to keep. I'm also guessing that this less-is-more approach will apply to any future commands as well. |
@fdncred looks like we're very much on the same page. I love how simple nushells commands tend to be. Maybe to help @dmatos2012 out, could you make a list of arguments that you and the core team think are important? |
Hey, so I think given the feedback you folks gave, I think I will start by making the change for . In my opinion, but it could be because I am not a hardcore |
Thanks @dmatos2012, and some files may be missing or not configured correctly which caused the CI to puke. The @nushell/core-team talked about listing the parameters that are important to us on the team meeting yesterday. I plan on making a list for the team to look at. One last thing we talked about yesterday, we definitely want to try and reduce dependencies if possible. I think we can remove |
Excellent!
It would be quite hard for us to remove I agree though that it's quite big, so I'll open an issue to at least put some more dependencies behind feature flags, because we definitely also do not need every dependency for every util. (uutils/coreutils#5202) Another problem I see dependency-wise is Edit: The issues are up. I added the links to the text above. Edit 2: I'd also love to call via Discord sometime to catch these kinds of issues a bit quicker. Maybe I could join one of the core team calls or we could set up a separate call. Other uutils maintainers and dmatos might also want to join. |
Id be up for joining! And unless I am not getting it right, isnt it expected that the CI will fail (for now)? Its currently pointing to my local path of |
I just meant remove it from the nushell Cargo.toml. It seemed to be able to do that when I was testing a yesterday.
Our team meetings are public every Wednesday at 3pm Central time. Anyone is welcome. If we want to have a special section in the meeting about uutils, I just need some bullet topics that we want to discuss. We're very informal.
@dmatos2012 maybe you're right? but couldn't we point it to the particular git branch in uutils/coreutils and make it work? |
Ahh, smart! Yeah I will try that tomorrow, seems doable but now its time to sleep in Europe lol. |
Cool! I might join on Wednesday. I'll let you know and we can discuss topics beforehand. I'll also tell the other uutils maintainers.
Very nice! |
Any progress on this @dmatos2012? Are there blockers that you need help with? Are you waiting for anyone to do something or you're just not finding the time yet? |
I've created a voting spreadsheet for the @nushell/core-team members to vote on parameters to keep. @dmatos2012 and @tertsdiepraam feel free to add a column to collect your votes as well. I don't really understand all the parameters but I put some votes in to show what I'm expecting. https://docs.google.com/spreadsheets/d/1aFXAg2IkHFCz289QYBroSn9XK_80lV3y7KEZxzeHw2c/edit?usp=sharing |
Looks good! I'll add a column! I'd also suggest a column with the BSD set of options for reference. The link only seems to get me view access at the moment. I've sent you a request for edit access. |
@tertsdiepraam feel free to augment. I updated the links, sorry, forgot to hit a button. :) |
voted in the sheet 😉 |
Hi @fdncred , I now made the CI happy, since I had made the mistake of changing some of the As far as I know, given that now CI is happy, now I am waiting for the wednesday meeting to define what flags we would be interested on, and then change the code to reflect that :) |
@tertsdiepraam Looks like there are 3 things that are blocking us from removing // use uucore::backup_control::{self, BackupMode};
// use uucore::update_control::{self, UpdateMode};
use uu_cp::{BackupMode, UpdateMode};
|
Interesting. I don't think For the other two, my preference is option 1, which I could make a PR for very quickly. Option 1 gives a full API to access Shall I make a PR for option 1? |
Reflect those on tests & code
Update uutils with progress bar fix
Rename current cp to CpOld
apply suggested changes
Rename commands again
650d74a
to
46f0f39
Compare
So I used some time before work 😛 , and proceeded to do as @fdncred indicated in the last comment. I also agree its a very good idea as it gives us some real life usage. Most likely, it wont be the only uncovered bug, so we could keep adding tests in the meantime. I additionally split the tests now into |
Thanks @dmatos2012. I'm trying to get a thumbs up/down on this right now. Hopefully we can land this today if it's approved by the nushell core-team. |
The nushell core-team agreed on this strategy. Let's go!!!!! |
Let's goo!!! Very excited for next commands to port and keep improving on the current ones. Thanks for all the guidance and patience here, it's so lovely to contribute here @tertsdiepraam @fdncred |
Whooo!!! 🚀🚀 I'm already excited for the next util, do we already know which one that's gonna be? 😄 Great work here @dmatos2012 and thanks for the coordination and guidance @fdncred! |
@tertsdiepraam I think the next 2 should be |
@tertsdiepraam @dmatos2012 |
@dmatos2012 @tertsdiepraam Thank you for working on this! Really excited about the new It's time to make |
Description
Hi. Basically, this is a continuation of the work that @fdncred started. Given some nice discussions on #9463 , and merged uutils PR from @tertsdiepraam we have decided to give the
cp
command thecrawl
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 currentnushell cp tests
pass along with some extra ones I copied over from theuutils
repo.With all of that being said, things that would be nice to decide, and keep working on:
Crawl:
named
flags, with their long and short forms(e.g. --update, --reflink, --preserve, etc), and using default values. Maybe-u
can already have adefault_missing_value
.switch
flags (see--backup
in code) as a contrast to the other named args.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.Walk:
copy
fromutils
. Currently it just sends it to stdout but errors have no spanAn added possibility is the addition of
SyntaxShape::OneOf()
forNamed
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 likecp --preserve [all timestamp]
orcp --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:
to check standard code formatting (
cargo fmt --all` applies these changes)-->
After Submitting