feat Touch

[prsabahrami]

Closes #144.
However, there is one thing. When using uu_touch for this, we are not passing the ShellState to the uumain function. Now, I have gone over the args, added state.cwd to the filenames (if not absolute) and then passed to uumain. The other option would be to copy and use the function inside uu_touch. I like the second option better giving us better control over this. What currently is implemented works but seems a bit hacky but let me know what you think!

[codecov]

Codecov Report

Attention: Patch coverage is 62.96296% with 50 lines in your changes missing coverage. Please review.

Project coverage is 62.01%. Comparing base (950da22) to head (6936a2b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/shell/src/commands/touch.rs	62.40%	50 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   61.50%   62.01%   +0.50%     
==========================================
  Files          29       30       +1     
  Lines        2824     2959     +135     
==========================================
+ Hits         1737     1835      +98     
- Misses       1087     1124      +37     

Flag	Coverage Δ
	62.01% <62.96%> (+0.50%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Hofer-Julian]

The other option would be to copy and use the function inside uu_touch

That also sounds better to me

[wolfv]

I think one of the problems is that the uu library uses println! in most places. This works fine as a standalone CLI utility, of course, but is problematic in our use-case because we would better do something like context.stdout.write(...).

This is why it's currently not possible to do ls > foo.txt.

I am not sure how we want to fix this generally. For uname I basically copied in the required functions and did the arg parsing on our end. I think for touch this makes sense as well if the cwd is part of the story.

[prsabahrami]

Yes! That is what I was thinking as well to copy all of the required functions and take care of it on our end.
Perfect, I'll update the implementation to this.
I'll have to update date accordingly as well later.

[Hofer-Julian]

This works fine as a standalone CLI utility

My understanding is that the nushell people libar-ify each uutil command upstream before they integrate it. We could consider doing something similar.

[wolfv]

I am not sure what libar-ify means but the nushell people do seem to wrap everything on their own CLI parser etc: https://github.com/nushell/nushell/tree/main/crates/nu-command/src/filesystem

[Hofer-Julian]

I am not sure what libar-ify means

PRs like this one: uutils/coreutils#5946

Tbf I thought they went further than this, so my comment didn't make much sense in this discussion.

[certik]

@Hofer-Julian I like your idea to offload as many things as we can upstream to coreutils, so that the whole ecosystem benefits, and we only maintain things that are needed specifically for shell.

[wolfv]

Yes, I think we are already benefitting from that! :) I read libar-ify and thought they do something special with binary modification with the ar tool.

[prsabahrami]

The functions from uu_touch have been moved to shell side. Though I modified parse_date and parse_timestamp and used dtparse crate to handle timestamp parsing instead of checking the formats one by one.
Also, the function pathbuf_from_stdout is currently put in this file but I have opened an issue uutils/coreutils#6740 on coreutils to add this function to their fsext.rs as a public function because it is actually quite a useful function and seemingly they planned to do this as well. That way it we can remove it and use directly from uucore.

// TODO: this may be a good candidate to put in fsext.rs
/// Returns a PathBuf to stdout.
///
/// On Windows, uses GetFinalPathNameByHandleW to attempt to get the path
/// from the stdout handle.
fn pathbuf_from_stdout() -> UResult<PathBuf> {

I'll try to submit a PR for this tomorrow to coreutils.

[certik]

Awesome, thanks!

I went to test it out on Windows, but unfortunately it didn't build:

~/repos/shell(touch)$ cargo r
    Updating crates.io index
  Downloaded arrayvec v0.7.6
  Downloaded thiserror v1.0.64
  Downloaded thiserror-impl v1.0.64
  Downloaded lazy_static v1.5.0
  Downloaded filetime v0.2.25
  Downloaded uu_touch v0.0.27
  Downloaded uu_date v0.0.27
  Downloaded rust_decimal v1.36.0
  Downloaded aho-corasick v1.1.3
  Downloaded regex v1.10.6
  Downloaded dtparse v2.0.1
  Downloaded regex-syntax v0.8.4
  Downloaded regex-automata v0.4.7
  Downloaded 13 crates (1.7 MB) in 14.73s
   Compiling thiserror v1.0.64
   Compiling num-traits v0.2.19
   Compiling lock_api v0.4.12
   Compiling windows-sys v0.52.0
   Compiling parking_lot_core v0.9.10
   Compiling windows-sys v0.48.0
   Compiling thiserror-impl v1.0.64
   Compiling aho-corasick v1.1.3
   Compiling scopeguard v1.2.0
   Compiling regex-syntax v0.8.4
   Compiling minimal-lexical v0.2.1
   Compiling anyhow v1.0.89
   Compiling rust_decimal v1.36.0
   Compiling nom v7.1.3
   Compiling regex-automata v0.4.7
   Compiling terminal_size v0.3.0
   Compiling dirs-sys v0.4.1
   Compiling anstyle-query v1.1.1
   Compiling anstyle-wincon v3.0.4
   Compiling mio v1.0.2
   Compiling socket2 v0.5.7
   Compiling pest v2.7.12
   Compiling miette v7.2.0
   Compiling anstream v0.6.15
   Compiling parking_lot v0.12.3
   Compiling home v0.5.9
   Compiling clap_builder v4.5.17
   Compiling chrono v0.4.38
   Compiling tokio v1.40.0
   Compiling nu-ansi-term v0.49.0
   Compiling arrayvec v0.7.6
   Compiling lazy_static v1.5.0
   Compiling lscolors v0.16.0
   Compiling pest v2.7.12 (https://github.com/pest-parser/pest.git?branch=master#65e5b2b7)
   Compiling dirs v5.0.1
   Compiling pest_meta v2.7.12
   Compiling regex v1.10.6
   Compiling fd-lock v4.0.2
   Compiling filetime v0.2.25
   Compiling unicode-segmentation v1.12.0
   Compiling parse_datetime v0.6.0
   Compiling which v6.0.3
   Compiling dtparse v2.0.1
   Compiling pest_generator v2.7.12
   Compiling rustyline v14.0.0
   Compiling clap v4.5.17
   Compiling pest_derive v2.7.12
   Compiling uucore v0.0.27
   Compiling pest_ascii_tree v0.1.0 (https://github.com/prsabahrami/pest_ascii_tree.git?branch=master#65ef51a8)
   Compiling uu_date v0.0.27
   Compiling uu_ls v0.0.27
   Compiling uu_uname v0.0.27
   Compiling uu_touch v0.0.27
   Compiling tokio-util v0.7.12
   Compiling deno_task_shell v0.17.0 (C:\Users\ondrejcertik\repos\shell\crates\deno_task_shell)
   Compiling shell v0.1.0 (C:\Users\ondrejcertik\repos\shell\crates\shell)
error[E0433]: failed to resolve: use of undeclared crate or module `windows_sys`
   --> crates\shell\src\commands\touch.rs:254:13
    |
254 |         use windows_sys::Win32::Foundation::{
    |             ^^^^^^^^^^^ use of undeclared crate or module `windows_sys`

error[E0433]: failed to resolve: use of undeclared crate or module `windows_sys`
   --> crates\shell\src\commands\touch.rs:258:13
    |
258 |         use windows_sys::Win32::Storage::FileSystem::{
    |             ^^^^^^^^^^^ use of undeclared crate or module `windows_sys`

error[E0408]: variable `ERROR_NOT_ENOUGH_MEMORY` is not bound in all patterns
   --> crates\shell\src\commands\touch.rs:283:13
    |
283 | ...   ERROR_PATH_NOT_FOUND | ERROR_NOT_ENOUGH_MEMORY | ERROR_INVALID_PARAMETER =...
    |       ^^^^^^^^^^^^^^^^^^^^   -----------------------   ^^^^^^^^^^^^^^^^^^^^^^^ pattern doesn't bind `ERROR_NOT_ENOUGH_MEMORY`
    |       |                      |
    |       |                      variable not in all patterns
    |       pattern doesn't bind `ERROR_NOT_ENOUGH_MEMORY`
    |
help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `path::to::ModOrType::ERROR_NOT_ENOUGH_MEMORY`
   --> crates\shell\src\commands\touch.rs:283:36
    |
283 | ...   ERROR_PATH_NOT_FOUND | ERROR_NOT_ENOUGH_MEMORY | ERROR_INVALID_P...
    |                              ^^^^^^^^^^^^^^^^^^^^^^^

error[E0408]: variable `ERROR_INVALID_PARAMETER` is not bound in all patterns
   --> crates\shell\src\commands\touch.rs:283:13
    |
283 | ...   ERROR_PATH_NOT_FOUND | ERROR_NOT_ENOUGH_MEMORY | ERROR_INVALID_PARAMETER =...
    |       ^^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^^^^^^^^^^^   ----------------------- variable not in all patterns
    |       |                      |
    |       |                      pattern doesn't bind `ERROR_INVALID_PARAMETER`
    |       pattern doesn't bind `ERROR_INVALID_PARAMETER`
    |
help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `path::to::ModOrType::ERROR_INVALID_PARAMETER`
   --> crates\shell\src\commands\touch.rs:283:62
    |
283 | ...R_NOT_ENOUGH_MEMORY | ERROR_INVALID_PARAMETER => {
    |                          ^^^^^^^^^^^^^^^^^^^^^^^

error[E0408]: variable `ERROR_PATH_NOT_FOUND` is not bound in all patterns
   --> crates\shell\src\commands\touch.rs:283:36
    |
283 | ...   ERROR_PATH_NOT_FOUND | ERROR_NOT_ENOUGH_MEMORY | ERROR_INVALID_PARAMETER =...
    |       --------------------   ^^^^^^^^^^^^^^^^^^^^^^^   ^^^^^^^^^^^^^^^^^^^^^^^ pattern doesn't bind `ERROR_PATH_NOT_FOUND`
    |       |                      |
    |       |                      pattern doesn't bind `ERROR_PATH_NOT_FOUND`
    |       variable not in all patterns
    |
help: if you meant to match on a variant or a `const` item, consider making the path in the pattern qualified: `path::to::ModOrType::ERROR_PATH_NOT_FOUND`
   --> crates\shell\src\commands\touch.rs:283:13
    |
283 | ...   ERROR_PATH_NOT_FOUND | ERROR_NOT_ENOUGH_MEMORY | ERROR_INVALID_P...
    |       ^^^^^^^^^^^^^^^^^^^^

error[E0433]: failed to resolve: use of undeclared type `USimpleError`
   --> crates\shell\src\commands\touch.rs:284:28
    |
284 |                 return Err(USimpleError::new(
    |                            ^^^^^^^^^^^^ use of undeclared type `USimpleError`

error[E0433]: failed to resolve: use of undeclared type `USimpleError`
   --> crates\shell\src\commands\touch.rs:290:28
    |
290 |                 return Err(USimpleError::new(
    |                            ^^^^^^^^^^^^ use of undeclared type `USimpleError`

error[E0433]: failed to resolve: use of undeclared type `USimpleError`
   --> crates\shell\src\commands\touch.rs:304:26
    |
304 |             .map_err(|e| USimpleError::new(1, e.to_string()))?
    |                          ^^^^^^^^^^^^ use of undeclared type `USimpleError`

Some errors have detailed explanations: E0408, E0433.
For more information about an error, try `rustc --explain E0408`.
error: could not compile `shell` (lib) due to 8 previous errors
warning: build failed, waiting for other jobs to finish...
~/repos/shell(touch)$

So we need to do two things:

• Add Windows testing into our CI
• Fix the PR to work on Windows

[prsabahrami]

Ahh yes! I missed that. I have added windows CI in #160.

[certik]

I think there is no $TEMP_DIR on Windows. Rather, why didn't the ~ work? That might be another bug.

[prsabahrami]

I think there is no $TEMP_DIR on Windows.

The test builder itself creates a temporary directory and adds it as a env var before running the test.

I am trying to recreate the error locally with cross but everything passes on Windows.
I am actually not sure why ~ failed on Ubuntu as well. Both cases pass locally. I changed to TEMP_DIR to fix the issue on Windows first then I'll investigate the reason why ~ failed.

[prsabahrami]

I think I got it. The behaviour of std::fs::File::create differs based on the platform.

[prsabahrami]

So, based on my understanding, there is an issue here with the way testing is being done.
We are comparing stderr with an expected stderr but the issue is that stderr is actually a miette fancy print. Which in this case can be something like below because of the size of the terminal or etc.

× cannot touch C:/non_existent_dir/non_existent.txt: No such file or
 │ directory

The existence of | here in the stderr output causes issues. We have been lucky that this hasn't happened in any other test until now.
@wolfv @certik What do you think we should do here? We can either take a hacky approach and check wether the words of the expected string are in the output in the same order or completely switch the testing approach and return let stderr_output to be a miette error instead of a simple string. That way we can simply check the actual error. However, I suggest this change to made in a separate PR.
Let me know what do you think!

[certik]

What is the problem with | in the output?

If the issue is absolute paths, we can filter them, we do that in LFortran so that we can test stacktraces.

[prsabahrami]

@certik Basically, the way testing is done right now is by simply checking if the stderr is exactly the same as the expected stderr or if it contains the expected string. For example in the case of the failing test and the reason I haven't really been able to reproduce this locally:
The stderr output of the failing test on Github Action platform is:

× cannot touch C:/non_existent_dir/non_existent.txt: No such file or
│ directory

Whereas locally this is just

× cannot touch C:/non_existent_dir/non_existent.txt: No such file or directory

So when we check wether the stderr contains the string "No such file or directory" it raises an error because actually there is a | right in the middle. But locally, since I guess miette prints all in one line..., there is no issue.

Note: this is a bug with the way testing is being done. Not with touch. Touch's behaviour is as expected.

[certik]

I see. Who is inserting the | in there? It shouldn't be there ideally.

[prsabahrami]

I believe that is just how miette fancy feature works. It prints each line starting with a | if I'm not wrong.

[Hofer-Julian]

We can either take a hacky approach and check wether the words of the expected string are in the output in the same order or completely switch the testing approach and return let stderr_output to be a miette error instead of a simple string.

Yeah, let's adapt the testing approach. Doing it in a separate PR sounds alright.

[prsabahrami]

This was much easier to handle than expected.
miette looks for the env var NO_GRAPHICS and setting it to 1 causes the prints not to have any ansi escape codes.

[Hofer-Julian]

After the merge conflicts are resolved, we can merge this from my side

[prsabahrami]

conflicts are resolved now.