search

GrayJack / coreutils

Core utils re-implementation for UNIX/UNIX-like systems written in Rust
Mozilla Public License 2.0
108 stars 40 forks source link

Implement `time` #141

Closed envp closed 3 years ago

envp commented 3 years ago

This PR will track building time

Partially fixes #53

envp commented 3 years ago

I tried running cargo fmt -p time -- --check locally, but I'm not able to reproduce the CI's results. What config are you using?

GrayJack commented 3 years ago

Sorry, but could you change the commit messages to use the project standard commit message style, I decided on this standard because the project is a workspace crate of several other crates, so it's hard to know what commit is related to what crate.

Formatting is using nightly because several features in the rustfmt are still as unstable feature.

envp commented 3 years ago

@GrayJack

A few questions:

  1. Fuchsia doesn't support the getrusage syscall. Do you think it's best to remove the target altogether?
  2. What do you think about getting time to be POSIX compliant first, and then adding OS-specific extensions? That would make both review and development easier as we can address the exceptions on a case by case basis.
  3. I'm not able to replicate the formatting requirements locally (or inside an ubuntu docker) with nightly toolchain. Both of those seem to pass for me. What's the best way to go about matching the formatting standards?
GrayJack commented 3 years ago 
1. Fuchsia doesn't support the `getrusage` syscall. Do you think it's best to remove the target altogether?

Yes, if is something required to support POSIX standard. We have this many toml files for 2 reasons

  1. Sometimes the API used on different systems are different and we want to at least build for some systems before moving to support other systems
  2. Many tools can be implemented for non UNIX systems, but has some UNIXisms, like Fuchsia, Redox, Haiku, etc, but since not all tools can be implemented on them, having separated toml files for them allow us to check only the ones that are possible to build.
2. What do you think about getting `time` to be POSIX compliant first, and then adding OS-specific extensions? That would make both review and development easier as we can address the exceptions on a case by case basis.

That is totally fine, in most tools we only require for POSIX compliance to consider the tool complete, and extensions, being OS specific or a available on all OSes, are greatly accepted additions

3. I'm not able to replicate the formatting requirements locally (or inside an ubuntu docker) with nightly toolchain. Both of those seem to pass for me. What's the best way to go about matching the formatting standards?

That's odd, It is working fine for me just doing cargo fmt. In the worst case scenario, if you gave me permission to push changes to your PR, I can format before merging.

envp commented 3 years ago 
1. Fuchsia doesn't support the `getrusage` syscall. Do you think it's best to remove the target altogether?

Yes, if is something required to support POSIX standard. We have this many toml files for 2 reasons

1. Sometimes the API used on different systems are different and we want to at least build for some systems before moving to support other systems

2. Many tools can be implemented for non UNIX systems, but has some UNIXisms, like Fuchsia, Redox, Haiku, etc, but since not all tools can be implemented on them, having separated toml files for them allow us to check only the ones that are possible to build.

Gotcha, it's been commented out of Fuchsia.toml for now. Is that all that is required?

2. What do you think about getting `time` to be POSIX compliant first, and then adding OS-specific extensions? That would make both review and development easier as we can address the exceptions on a case by case basis.

That is totally fine, in most tools we only require for POSIX compliance to consider the tool complete, and extensions, being OS specific or a available on all OSes, are greatly accepted additions

Great, thanks! I'll make an issue with the TODO items from this PR, and set up machinery for platform extensions in a follow-up PR.

3. I'm not able to replicate the formatting requirements locally (or inside an ubuntu docker) with nightly toolchain. Both of those seem to pass for me. What's the best way to go about matching the formatting standards?

That's odd, It is working fine for me just doing cargo fmt. In the worst case scenario, if you gave me permission to push changes to your PR, I can format before merging.

Are you using an ubuntu/debian machine?

Also, I think the build is failing because it seems there is a crate called time on crates.io already (there's a complaint of potential segfault)

GrayJack commented 3 years ago

Is that all that is required?

Yes 😄

Great, thanks! I'll make an issue with the TODO items from this PR, and set up machinery for platform extensions in a follow-up PR.

That's great to hear, we're always happy to receive contributions 😄

Are you using an ubuntu/debian machine?

I use Manjaro, it is Arch-based

Also, I think the build is failing because it seems there is a crate called time on crates.io already (there's a complaint of potential segfault)

Yes, it's the crate we use to handle time in some other tools. It's a possible segfault by using a non-thread-safe function call on UNIX platforms, but since for now all tools are single threaded, it's fine (for now). But cargo-deny check don't block merges.

envp commented 3 years ago

Besides the from function that should be a From impl, the only other thing that caught my attention are the public fields of the structs in coreutils_core::os::resource considering that coreutils_core is suppose to be of a medium to high level of API abstraction for the raw API used to implement the utilities, so the access to the field is suppose to be done using functions that return references to the fields, but I think that is kinda of a big change that can be addressed in another PR.

Besides that, nothing else seems out of place.

I see, I was considering implementing functions for "derived" properties (per GNU time's % modifiers, like "%D"), wouldn't it be prudent to keep the getter/setters to a minimum and use struct fields (only using functions when they summarize some kind of computation)? (especially since the rusage interface is unlikely to change).

Ack on implementing the From<...> trait didn't realize that was a thing :)

GrayJack commented 3 years ago

wouldn't it be prudent to keep the getter/setters to a minimum and use struct fields (only using functions when they summarize some kind of computation)? (especially since the rusage interface is unlikely to change).

That depends, I personally think about the contract I want with the library usage. For example, in the Utmpx, I made with methods to get a immutable reference to all the possible fields (unless if return type is Copy), because it's a struct made to the user only get the system information, but not modify it, so I made an API that even if the user creates a mutable instance of Utmpx, it cannot modify whatever it holds.

But since in this case all possible fields are Copy, maybe it is a ok thing. But still, a user that creates a mutable instance of RUsage can by mistake modify it (maybe it's just me that are paranoid with it because I lost count the amount of times I did exactly that before Rust and when I was still a inexperienced rustacean). I'll say let's roll like it is now and moving forward well see if it will be a need to make everything private and create methods to access the values.

GrayJack commented 3 years ago

bors r+

bors[bot] commented 3 years ago

Build succeeded: