`touch`: move from `time` to `chrono` by tertsdiepraam · Pull Request #4600 · uutils/coreutils

tertsdiepraam · 2023-03-24T11:06:00Z

chrono has better support for daylight savings time, so I was able to support that and enable the test for it. It's also just easier to work with, so we can remove a lot of verbosity with this too.

For reviewing, the DST and leap second handling is probably most important. The formats should also be checked, even though they are (hopefully) well-tested.

sylvestre · 2023-03-24T19:26:22Z

Some rustfmt & clippy warnings

tertsdiepraam · 2023-03-24T19:29:07Z

Yeah I'll look into those after the PR it depends on is merged.

sylvestre · 2023-04-14T19:29:46Z

Windows is broken:


---- test_touch::test_touch_mtime_dst_fails stdout ----
run: D:\a\coreutils\coreutils\target\debug\coreutils.exe touch -m -t 202003080200 test_touch_set_mtime_dst_fails
thread 'test_touch::test_touch_mtime_dst_fails' panicked at 'Command was expected to fail.
stdout = 
 stderr = ', tests\by-util\test_touch.rs:627:6
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/28a29282f6dde2e4aba6e1e4cfea5c9430a00217/library\std\src\panicking.rs:577
   1: core::panicking::panic_fmt
             at /rustc/28a29282f6dde2e4aba6e1e4cfea5c9430a00217/library\core\src\panicking.rs:67
   2: tests::common::util::CmdResult::failure
             at .\tests\common\util.rs:402
   3: tests::common::util::UCommand::fails
             at .\tests\common\util.rs:1568
   4: tests::test_touch::test_touch_mtime_dst_fails
             at .\tests\by-util\test_touch.rs:618
   5: tests::test_touch::test_touch_mtime_dst_fails::closure$0
             at .\tests\by-util\test_touch.rs:608
   6: core::ops::function::FnOnce::call_once<tests::test_touch::test_touch_mtime_dst_fails::closure_env$0,tuple$<> >
             at /rustc/28a29282f6dde2e4aba6e1e4cfea5c9430a00217\library\core\src\ops\function.rs:250
   7: core::ops::function::FnOnce::call_once
             at /rustc/28a29282f6dde2e4aba6e1e4cfea5c9430a00217/library\core\src\ops\function.rs:250
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

tertsdiepraam · 2023-04-18T10:38:53Z

Yes, I'll look into that :)

tertsdiepraam · 2023-05-28T23:01:42Z

I bet Windows doesn't work because it does not parse the libc-style timezone syntax. I think it's best if I just disable the test for Windows. At least we get the correct behaviour merged then. Do you agree, @sylvestre?

sylvestre · 2023-07-12T09:55:09Z

@tertsdiepraam sorry, i missed your question. I am fine with disabling the test on Windows

tertsdiepraam · 2023-07-27T10:51:25Z

I disabled the test on windows and got this all up to date with main I think :)

cakebaker · 2023-07-27T14:24:24Z

src/uu/touch/src/touch.rs

+mod format {
+    pub(crate) const POSIX_LOCALE: &str = "%a %b %e %H:%M:%S %Y";
+    pub(crate) const ISO_8601: &str = "%Y-%m-%d";
+    // "%Y%m%d%H%M.%S" 15 chars


Somehow I don't see what the purpose is of this and the other similar comments.

It's used in this function:

coreutils/src/uu/touch/src/touch.rs

Lines 401 to 415 in 1037304

let (format, ts) = match s.chars().count() {

15 => (YYYYMMDDHHMM_DOT_SS, s.to_owned()),

12 => (YYYYMMDDHHMM, s.to_owned()),

// If we don't add "20", we have insufficient information to parse

13 => (YYYYMMDDHHMM_DOT_SS, format!("20{}", s)),

10 => (YYYYMMDDHHMM, format!("20{}", s)),

11 => (YYYYMMDDHHMM_DOT_SS, format!("{}{}", current_year(), s)),

8 => (YYYYMMDDHHMM, format!("{}{}", current_year(), s)),

_ => {

return Err(USimpleError::new(

1,

format!("invalid date format {}", s.quote()),

))

}

};

It's mostly just a leftover from before, when it was harder to determine the length of the format due to the format strings from the time crate. I can remove it if you want.

I would remove them.

cakebaker · 2023-07-27T14:30:11Z

src/uu/touch/src/touch.rs

+///
+/// The DateTime is converted into a unix timestamp from which the FileTime is
+/// constructed.
+fn dt_to_filetime<T: TimeZone>(dt: &DateTime<T>) -> FileTime {


I would use datetime instead of dt because chrono also uses datetime in its function names.

Suggested change

fn dt_to_filetime<T: TimeZone>(dt: &DateTime<T>) -> FileTime {

fn datetime_to_filetime<T: TimeZone>(dt: &DateTime<T>) -> FileTime {

Yeah that name was leftover from local_dt_to_filetime. I'll fix it

cakebaker · 2023-07-27T14:47:36Z

src/uu/touch/src/touch.rs

-
-    Ok(ft)
+
+    // We have to check that ft is valid time. Due to daylight saving


I guess ft is a left over from the previous code. Not sure whether this first sentence is even needed.

Yeah I removed the sentence

cakebaker · 2023-07-27T14:52:30Z

src/uu/touch/src/touch.rs

+    // We have to check that ft is valid time. Due to daylight saving
+    // time switch, local time can jump from 1:59 AM to 3:00 AM,
+    // in which case any time between 2:00 AM and 2:59 AM is not valid.
+    // If we are within this jump, chrono assumes takes the offset from before


I'm not sure whether there is something missing between "assumes" and "takes" or one of those words can be removed.

Indeed, I removed "assumes"

This allows us to work with daylight savings time which is necessary to enable one of the tests. The leap second calculation and parsing are also ported over. A bump in the chrono version is necessary to use NaiveTime::MIN.

sylvestre force-pushed the touch-move-from-time-to-chrono branch from 4ff8b60 to 3081b15 Compare March 24, 2023 19:14

tertsdiepraam force-pushed the touch-move-from-time-to-chrono branch from 3081b15 to f98247c Compare March 26, 2023 10:11

tertsdiepraam marked this pull request as ready for review March 26, 2023 10:11

tertsdiepraam mentioned this pull request Mar 29, 2023

date -f dates.txt is failing #4657

Closed

sylvestre force-pushed the touch-move-from-time-to-chrono branch from f98247c to 21b1dab Compare March 30, 2023 09:28

tertsdiepraam force-pushed the touch-move-from-time-to-chrono branch from 21b1dab to 795030a Compare April 7, 2023 11:20

tertsdiepraam marked this pull request as draft April 18, 2023 10:38

cakebaker linked an issue Jun 3, 2023 that may be closed by this pull request

touch should use the chrono crate and not time #4935

Closed

tertsdiepraam force-pushed the touch-move-from-time-to-chrono branch 4 times, most recently from ca4292a to 1037304 Compare July 27, 2023 10:50

tertsdiepraam marked this pull request as ready for review July 27, 2023 10:50

cakebaker reviewed Jul 27, 2023

View reviewed changes

touch: move from time to chrono

c299771

This allows us to work with daylight savings time which is necessary to enable one of the tests. The leap second calculation and parsing are also ported over. A bump in the chrono version is necessary to use NaiveTime::MIN.

tertsdiepraam force-pushed the touch-move-from-time-to-chrono branch from 1037304 to c299771 Compare July 27, 2023 15:06

cakebaker merged commit b24f91c into uutils:main Jul 27, 2023

	let (format, ts) = match s.chars().count() {
	15 => (YYYYMMDDHHMM_DOT_SS, s.to_owned()),
	12 => (YYYYMMDDHHMM, s.to_owned()),
	// If we don't add "20", we have insufficient information to parse
	13 => (YYYYMMDDHHMM_DOT_SS, format!("20{}", s)),
	10 => (YYYYMMDDHHMM, format!("20{}", s)),
	11 => (YYYYMMDDHHMM_DOT_SS, format!("{}{}", current_year(), s)),
	8 => (YYYYMMDDHHMM, format!("{}{}", current_year(), s)),
	_ => {
	return Err(USimpleError::new(
	1,
	format!("invalid date format {}", s.quote()),
	))
	}
	};

	fn dt_to_filetime<T: TimeZone>(dt: &DateTime<T>) -> FileTime {
	fn datetime_to_filetime<T: TimeZone>(dt: &DateTime<T>) -> FileTime {


		Ok(ft)

		// We have to check that ft is valid time. Due to daylight saving

Uh oh!

Conversation

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

Uh oh!

Uh oh!

sylvestre commented Mar 24, 2023

Uh oh!

tertsdiepraam commented Mar 24, 2023

Uh oh!

sylvestre commented Apr 14, 2023

Uh oh!

tertsdiepraam commented Apr 18, 2023

Uh oh!

tertsdiepraam commented May 28, 2023

Uh oh!

sylvestre commented Jul 12, 2023

Uh oh!

tertsdiepraam commented Jul 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tertsdiepraam commented Mar 24, 2023 •

edited

Loading