Skip to content
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

Initial implementation for uutils uname #11684

Merged
merged 9 commits into from Mar 25, 2024
Merged

Conversation

dmatos2012
Copy link
Contributor

@dmatos2012 dmatos2012 commented Jan 30, 2024

Hi,
This PR aims at implementing the first iteration for uname using uutils. Couple of things:

  • Currently my PR to make the required changes is pending in uutils repo.
  • I guess the number of flags has to be investigated. Still the tests cover all of them.

Description

User-Facing Changes

Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

  • cargo fmt --all -- --check to check standard code formatting (cargo fmt --all applies these changes)
  • cargo clippy --workspace -- -D warnings -D clippy::unwrap_used to check that you're using the standard code style
  • cargo test --workspace to check that all tests pass (on Windows make sure to enable developer mode)
  • 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

use toolkit.nu  # or use an `env_change` hook to activate it automatically
toolkit check pr

-->

After Submitting

@fdncred
Copy link
Collaborator

fdncred commented Jan 30, 2024

Thanks @dmatos2012 for another great PR!

I'm wondering if uuname can just return the data instead of printing it out? It would be great to have something closer to $nu.os-info.

 $nu.os-info
╭────────────────┬─────────╮
 name            windows 
 arch            x86_64  
 family          windows 
 kernel_version  22631   
╰────────────────┴─────────╯

Maybe something like this

╭───────────────────┬─────────────────────────╮
 kernel-name        Windows_NT              
 node-name          DESKTOP-1QK8J8G         
 kernel-release     10.0                    
 kernel-version     22631                   
 machine            x86_64                  
 processor          unknown                 
 hardware-platform  unknown                 
 operating-system   MS/Windows (Windows 11) 
╰───────────────────┴─────────────────────────╯

@dmatos2012
Copy link
Contributor Author

dmatos2012 commented Jan 30, 2024

Thanks @dmatos2012 for another great PR!

I'm wondering if uuname can just return the data instead of printing it out? It would be great to have something closer to $nu.os-info.

 $nu.os-info
╭────────────────┬─────────╮
 name            windows 
 arch            x86_64  
 family          windows 
 kernel_version  22631   
╰────────────────┴─────────╯

Thought about something similar as well to the output of ls , but wasnt exactly sure if thats something we wanted.

However, with your suggestion, it means there would be only one way of using it: namely uname, which will print out all the flags as record as you pointed out. i really like it, since it avoids the mess of all the -n, -p,-i. Will give it a try :)

@fdncred
Copy link
Collaborator

fdncred commented Jan 30, 2024

ya, i think it would be more nushell-y just to print out a record if we can get the information.

@fdncred
Copy link
Collaborator

fdncred commented Feb 1, 2024

Not quite right but looking better.

❯ uname 
╭──────────────────┬─────────────────────────╮
│ kernel-name      │ Windows_NT              │
│ nodename         │ MYHOSTNAME              │
│ kernel-release   │ 10.0                    │
│ kernel-version   │ 22631 x86_64 MS/Windows │
│ machine          │ (Windows                │
│ operating-system │ 11)                     │
╰──────────────────┴─────────────────────────╯

@dmatos2012
Copy link
Contributor Author

dmatos2012 commented Feb 2, 2024

Much better now :)
image

Please check this and the uutils PR and let me know what you think. I realized that trying to parse the long string was no good because probably every system will output in a different way so we needed a more structured output from uutils.

As far as testing goes, given that we have no flags now, I think we will have to rely on their test harness at least for now.

@fdncred
Copy link
Collaborator

fdncred commented Feb 2, 2024

Looks good on my windows box. I'll try to test it on mac later.

❯ uname
╭──────────────────┬─────────────────────────╮
│ kernel-name      │ Windows_NT              │
│ nodename         │ MYHOSTNAME              │
│ kernel-release   │ 10.0                    │
│ kernel-version   │ 22631                   │
│ machine          │ x86_64                  │
│ operating-system │ MS/Windows (Windows 11) │
╰──────────────────┴─────────────────────────╯

@fdncred
Copy link
Collaborator

fdncred commented Feb 3, 2024

This is what my mac says. Long kernel-version, not saying that's bad, was just surprised.

❯ uname
╭──────────────────┬───────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ kernel-name      │ Darwin                                                                                                │
│ nodename         │ M1X.local                                                                                             │
│ kernel-release   │ 23.3.0                                                                                                │
│ kernel-version   │ Darwin Kernel Version 23.3.0: Wed Dec 20 21:30:44 PST 2023; root:xnu-10002.81.5~7/RELEASE_ARM64_T6000 │
│ machine          │ arm64                                                                                                 │
│ operating-system │ Darwin                                                                                                │
╰──────────────────┴───────────────────────────────────────────────────────────────────────────────────────────────────────╯

@fdncred
Copy link
Collaborator

fdncred commented Feb 8, 2024

I guess we're just waiting for the core-utils release. Is that correct?

@dmatos2012
Copy link
Contributor Author

I guess we're just waiting for the core-utils release. Is that correct?

Yes, technically we are waiting for it to be merged first, and then the release, but aside from that everything should be ready.

@fdncred
Copy link
Collaborator

fdncred commented Feb 20, 2024

yay! one step closer.

@dmatos2012
Copy link
Contributor Author

yeah I think I somehow messed up the branch but should be fixed now @fdncred. So maybe time to take it for a ride and wait for the release of uutils. Looking forward for this getting released :)

@fdncred
Copy link
Collaborator

fdncred commented Feb 20, 2024

It's kind of amazing how troublesome a little utility like this can be. I really appreciate all the effort doing the uutils PR and making changes here, so it works nushell-y. All good stuff. Thanks very much!

@fdncred fdncred added the coreutils-uutils Changes relating to coreutils/uutils label Mar 2, 2024
@fdncred
Copy link
Collaborator

fdncred commented Mar 12, 2024

I guess we're still waiting for a coreutils release? @tertsdiepraam is there a coreutils release in our future? 🤣

@tertsdiepraam
Copy link
Contributor

🔮👀

My cristal ball says.... yes, should be fairly soon

@AndreiSva
Copy link

Will this replace $env.os-info?

@fdncred
Copy link
Collaborator

fdncred commented Mar 13, 2024

Will this replace $env.os-info?

no. $nu.os-info is a compile time variable that we plan on keeping. it's helpful because it's super fast.

@fdncred
Copy link
Collaborator

fdncred commented Mar 23, 2024

yay, looks like coreutils 0.25 has been released

@dmatos2012
Copy link
Contributor Author

I think this should be ready to go now @fdncred ? The diff probably looks weird as I noticed all of the Cargo.toml were changed lol, but think should be good now :)

@fdncred
Copy link
Collaborator

fdncred commented Mar 25, 2024

I'd like to keep all uu_* crates on the same version so I changed them all to 0.0.25. I think bad things may happen if we mix and match versions.

@fdncred
Copy link
Collaborator

fdncred commented Mar 25, 2024

No clue what this means
image

@sholderbach
Copy link
Member

The Cargo.lock just needs to be commited as well after the version bump. This check is aimed to catch bad tests or out of date Cargo.lock

@fdncred fdncred merged commit 838fc7e into nushell:main Mar 25, 2024
16 checks passed
@fdncred
Copy link
Collaborator

fdncred commented Mar 25, 2024

Thanks again @dmatos2012!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
coreutils-uutils Changes relating to coreutils/uutils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants