Implement `time` #141

envp · 2021-02-21T00:29:35Z

This PR will track building time

Partially fixes #53

GrayJack · 2021-02-21T19:24:16Z

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.

GrayJack

I left some comments for us to talk.

I feel that rusage.rs could be a module in the coreutils_core crate, but that can be done at any time, even in another PR.

GrayJack · 2021-02-21T19:32:43Z

time/Cargo.toml

+# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
+
+[dependencies]
+clap = { version = "^2.33.0", features = ["yaml", "wrap_help"] }


Looks like you're using yaml feature of clap. We dropped that due some problems with it. You can find the template used now here

GrayJack · 2021-02-21T19:35:31Z

Cargo.toml

@@ -22,4 +22,5 @@ members = [
    "tty",
    "wc",
    "yes",
+    "time",


Can you add this to all toml files related to the OS that this utility can be compiled? The reason behind this is because no all OS works the same, and sometimes (like fuchsia and haiku) doesn't even has a way to do something. The CI also uses the toml files os respective OS to build and test the crates

GrayJack · 2021-02-21T19:45:32Z

time/src/rusage.rs

+    let seconds = u64::try_from(tv.tv_sec).expect("Unable to convert tv_sec to u64");
+    let micros = u64::try_from(tv.tv_usec).expect("Unable to convert tv_usec to u64");


There is any case where this conversion will fail? Because panic exit with a different message than the tools messages.
Or is it the case where failing to convert in any OS with this tool should be considered a bug in the utility?

I don't expect the conversion to fail, since libc::getrusage won't return negative values for the timeval struct.

The latter is true. I would think it's a bug in time, since this is an implementation detail.

Because panic exit with a different message than the tools messages

I'm not sure I follow, this message corresponds to an internal error, if that helps clarify my usage.

don't expect the conversion to fail, since libc::getrusage won't return negative values for the timeval struct.

The latter is true. I would think it's a bug in time, since this is an implementation detail.

In this case panic is fine

I'm not sure I follow, this message corresponds to an internal error, if that helps clarify my usage.

I was just worried that panic exit may happen in a non-bug case where the error message should be similar to user error messages like the other tools

envp · 2021-03-06T20:59:41Z

@GrayJack

A few questions:

Fuchsia doesn't support the getrusage syscall. Do you think it's best to remove the target altogether?
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.
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?

- Initial structure taken from .template with tool specific info filled in

- Wraps getrusage(2)

No longer use number_of_values(true) as that implies an option Also use "ARGUMENT" to match from clap

- Refactor enum declaration to use `Self` - Add test cases for `TimeOpts` creation - Rename 'OutputFormat' to 'OutputFormatter'

Unspecified errors are arbitrarily mapped to the 1-125 range

Returns a zeroed RUsage struct

GrayJack · 2021-03-06T21:26:36Z

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

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
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 · 2021-03-06T21:38:50Z

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 · 2021-03-06T22:49:55Z

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.

GrayJack

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.

GrayJack · 2021-03-07T12:23:03Z

coreutils_core/src/os/resource.rs

+impl RUsage {
+    fn from(ru: rusage) -> Self {
+        RUsage {
+            timing: Timing { user_time: ru.ru_utime, sys_time: ru.ru_stime },
+            mem:    MemoryUsage {
+                max_rss: ru.ru_maxrss as u64,
+                num_minor_page_flt: ru.ru_minflt as u64,
+                num_major_page_flt: ru.ru_majflt as u64,
+                num_vol_ctx_switch: ru.ru_nvcsw as u64,
+                num_invol_ctx_switch: ru.ru_nivcsw as u64,
+                shared_mem_size: ru.ru_ixrss as u64,
+                unshared_data_size: ru.ru_idrss as u64,
+                unshared_stack_size: ru.ru_isrss as u64,
+                num_swaps: ru.ru_nswap as u64,
+            },
+            io:     IOUsage {
+                num_block_in:  ru.ru_inblock as u64,
+                num_block_out: ru.ru_oublock as u64,
+                num_sock_recv: ru.ru_msgrcv as u64,
+                num_sock_send: ru.ru_msgsnd as u64,
+                num_signals:   ru.ru_nsignals as u64,
+            },
+        }
+    }
+}


This would be better as a impl From<rusage> for RUsage

envp · 2021-03-07T17:23:26Z

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 · 2021-03-07T22:13:37Z

coreutils_core/src/os/resource.rs

@@ -6,6 +6,8 @@ use super::TimeVal;
 use libc::getrusage;
 use libc::{c_int, rusage, RUSAGE_CHILDREN, RUSAGE_SELF};

+use std::convert::From;


From is in the Rust prelude, so you don't need to pull it into the scope

GrayJack · 2021-03-07T22:32:44Z

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.

This test is truly arbitrary and more of a benchmark than a test

GrayJack · 2021-03-10T04:28:58Z

bors r+

bors · 2021-03-10T04:33:12Z

Build succeeded:

envp force-pushed the feature/time branch from bd9cef4 to c124bf2 Compare February 21, 2021 00:32

This comment has been minimized.

Sign in to view

envp force-pushed the feature/time branch 2 times, most recently from e283304 to b8ccc8a Compare February 21, 2021 16:45

GrayJack reviewed Feb 21, 2021

View reviewed changes

General: Add time binary to toml files of OSes

b27496b

envp force-pushed the feature/time branch 2 times, most recently from 85b4688 to 0fb3819 Compare March 6, 2021 17:35

envp marked this pull request as draft March 6, 2021 17:35

envp force-pushed the feature/time branch 3 times, most recently from 1b13de4 to 089004d Compare March 6, 2021 20:03

envp force-pushed the feature/time branch 4 times, most recently from 6002e6f to 61d4f86 Compare March 6, 2021 21:14

envp added 12 commits March 6, 2021 16:22

Time: Scaffold code from template

471414b

- Initial structure taken from .template with tool specific info filled in

Time: cli: Add -p flag, and accept a command to run

b9ad3d5

Time: cli: Rename struct s/Args/TimeOpts

a621450

Core: Create, export module resource

6229d6a

- Wraps getrusage(2)

Time: cli: Combine exec_args and executable into a single field

2ffb334

Time: subprocess: Create subprocess module

cd93133

Time: Parse CLI opts, run subprocess and extract resource usage

4e6468f

Time: cli: Fix bug in argument specification

8caf5c9

No longer use number_of_values(true) as that implies an option Also use "ARGUMENT" to match from clap

Time: cli: Refactor and add test cases

f0b97fd

- Refactor enum declaration to use `Self` - Add test cases for `TimeOpts` creation - Rename 'OutputFormat' to 'OutputFormatter'

Time: Break up CLI into flags, output modules

6f3dd31

Time: Implement POSIX.2 output formatting and macos (FreeBSD) default

003f012

Time: Format code

0bcb1f2

envp added 5 commits March 6, 2021 16:22

Time: output: Move platform specific details into their own modules

54223a0

Time: subprocess: Follow POSIX.2 spec for error codes

1f25d16

Unspecified errors are arbitrarily mapped to the 1-125 range

Time: cli: Drop non-standard --posix

d8eba37

Time: drop macos specific code

31264e0

Time: Custom get_rusage() for FuchsiaOS

c5b4b55

Returns a zeroed RUsage struct

envp force-pushed the feature/time branch from 61d4f86 to c5b4b55 Compare March 6, 2021 21:22

Time: Format source code

cfb73c7

envp force-pushed the feature/time branch from 7c54d4f to cfb73c7 Compare March 6, 2021 21:30

envp requested a review from GrayJack March 6, 2021 22:01

envp marked this pull request as ready for review March 6, 2021 22:01

envp mentioned this pull request Mar 7, 2021

Implement time #53

Open

4 tasks

GrayJack reviewed Mar 7, 2021

View reviewed changes

Core: resource: Implement Conversion trait for rusage -> RUsage

3776a7c

envp force-pushed the feature/time branch from da9b4fe to 3776a7c Compare March 7, 2021 17:45

GrayJack reviewed Mar 7, 2021

View reviewed changes

envp force-pushed the feature/time branch from 6e69844 to 1af17d3 Compare March 8, 2021 00:54

envp added 2 commits March 7, 2021 20:17

Time: subprocess: Drop benchmark like test

aca20a8

This test is truly arbitrary and more of a benchmark than a test

Core: resource: Drop import as std::convert::From is in the Prelude

46529e0

envp force-pushed the feature/time branch from 1af17d3 to 46529e0 Compare March 8, 2021 01:17

envp requested a review from GrayJack March 10, 2021 03:35

bors bot merged commit a6dc22f into GrayJack:dev Mar 10, 2021

envp deleted the feature/time branch April 12, 2021 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `time` #141

Implement `time` #141

envp commented Feb 21, 2021 •

edited

Loading

This comment has been minimized.

GrayJack commented Feb 21, 2021

GrayJack left a comment

GrayJack Feb 21, 2021

GrayJack Feb 21, 2021

GrayJack Feb 21, 2021

envp Feb 27, 2021 •

edited

Loading

GrayJack Feb 27, 2021

envp commented Mar 6, 2021 •

edited

Loading

GrayJack commented Mar 6, 2021 •

edited

Loading

envp commented Mar 6, 2021 •

edited

Loading

GrayJack commented Mar 6, 2021

GrayJack left a comment

GrayJack Mar 7, 2021

envp commented Mar 7, 2021

GrayJack Mar 7, 2021

GrayJack commented Mar 7, 2021 •

edited

Loading

GrayJack commented Mar 10, 2021

bors bot commented Mar 10, 2021

@@ @@ -22,4 +22,5 @@ members = [ @@
                   "tty",
                   "wc",
                   "yes",
+                  "time",

		let seconds = u64::try_from(tv.tv_sec).expect("Unable to convert tv_sec to u64");
		let micros = u64::try_from(tv.tv_usec).expect("Unable to convert tv_usec to u64");

Implement time #141

Implement time #141

Conversation

envp commented Feb 21, 2021 • edited Loading

This comment has been minimized.

GrayJack commented Feb 21, 2021

GrayJack left a comment

Choose a reason for hiding this comment

GrayJack Feb 21, 2021

Choose a reason for hiding this comment

GrayJack Feb 21, 2021

Choose a reason for hiding this comment

GrayJack Feb 21, 2021

Choose a reason for hiding this comment

envp Feb 27, 2021 • edited Loading

Choose a reason for hiding this comment

GrayJack Feb 27, 2021

Choose a reason for hiding this comment

envp commented Mar 6, 2021 • edited Loading

GrayJack commented Mar 6, 2021 • edited Loading

envp commented Mar 6, 2021 • edited Loading

GrayJack commented Mar 6, 2021

GrayJack left a comment

Choose a reason for hiding this comment

GrayJack Mar 7, 2021

Choose a reason for hiding this comment

envp commented Mar 7, 2021

GrayJack Mar 7, 2021

Choose a reason for hiding this comment

GrayJack commented Mar 7, 2021 • edited Loading

GrayJack commented Mar 10, 2021

bors bot commented Mar 10, 2021

Implement `time` #141

Implement `time` #141

envp commented Feb 21, 2021 •

edited

Loading

envp Feb 27, 2021 •

edited

Loading

envp commented Mar 6, 2021 •

edited

Loading

GrayJack commented Mar 6, 2021 •

edited

Loading

envp commented Mar 6, 2021 •

edited

Loading

GrayJack commented Mar 7, 2021 •

edited

Loading