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

touch: implement - #3158

Merged
merged 37 commits into from
Mar 6, 2022
Merged

Conversation

dgunay
Copy link
Contributor

@dgunay dgunay commented Feb 19, 2022

Closes #3157

Issues:

  • Still doesn't seem to pass the GNU empty-file test nevermind, it passes, I just forgot to rebuild after fixing it.
  • Solution isn't portable (just opens /dev/fd/1)
  • On Windows, involves a bunch of low-level unsafe code not really an issue I guess since it is standard across uutils to use winapi
  • running into frequent CI roadblocks

@dgunay dgunay changed the title Bug/stdin touch change mtime touch: implement - Feb 19, 2022
@sylvestre
Copy link
Contributor

This is failing on windows:


---- test_touch::test_touch_changes_time_of_file_in_stdout stdout ----
current_directory_resolved: 
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmpDT63Tf\test_touch_changes_time_of_file_in_stdout
run: D:\a\coreutils\coreutils\target\x86_64-pc-windows-msvc\debug\coreutils.exe touch -
thread 'test_touch::test_touch_changes_time_of_file_in_stdout' panicked at 'Command was expected to succeed.
stdout = 
 stderr = touch: cannot touch '/dev/fd/1': No such file or directory
', tests\common\util.rs:174:9


@dgunay
Copy link
Contributor Author

dgunay commented Feb 19, 2022

This is failing on windows:


---- test_touch::test_touch_changes_time_of_file_in_stdout stdout ----
current_directory_resolved: 
touch: C:\Users\RUNNER~1\AppData\Local\Temp\.tmpDT63Tf\test_touch_changes_time_of_file_in_stdout
run: D:\a\coreutils\coreutils\target\x86_64-pc-windows-msvc\debug\coreutils.exe touch -
thread 'test_touch::test_touch_changes_time_of_file_in_stdout' panicked at 'Command was expected to succeed.
stdout = 
 stderr = touch: cannot touch '/dev/fd/1': No such file or directory
', tests\common\util.rs:174:9


Do you know of there's a good cross-platform way to get a Path to stdout? The GNU touch just uses the file descriptor number (1) - wasn't sure how to do that in Rust idiomatically

@sylvestre
Copy link
Contributor

sylvestre commented Feb 19, 2022 via email

@dgunay
Copy link
Contributor Author

dgunay commented Feb 19, 2022

Seems like this will be tricky, the only option I've found so far to support this is to do std::io::StdOut, convert it to a raw file handle, and then call this unsafe Windows API: https://microsoft.github.io/windows-docs-rs/doc/windows/Win32/Storage/FileSystem/fn.GetFinalPathNameByHandleA.html

Will set up a windows VM soon and try it out.

@dgunay dgunay marked this pull request as draft February 19, 2022 10:33
@dgunay
Copy link
Contributor Author

dgunay commented Feb 20, 2022

Almost got it, there's some gross <?> in the output probably because I am not navigating the string conversions correctly.

@dgunay dgunay marked this pull request as ready for review February 20, 2022 20:54
@dgunay
Copy link
Contributor Author

dgunay commented Feb 20, 2022

Ok, I think this should be okay for now. This is my first time doing unsafe Rust and using Win32 APIs, so there may be improvements to be made.

@dgunay
Copy link
Contributor Author

dgunay commented Feb 20, 2022

Not really sure what is causing this CI failure: https://github.com/uutils/coreutils/runs/5267054489?check_suite_focus=true#step:12:318

@tertsdiepraam
Copy link
Member

I've never seen that before, but it might indeed be unrelated. We'll see if it oersists on this run

@dgunay
Copy link
Contributor Author

dgunay commented Feb 21, 2022

I did do some UTF-16 string stuff here, maybe it's not impossible I've inadvertently caused some spooky action at a distance.

@dgunay
Copy link
Contributor Author

dgunay commented Feb 21, 2022

This function has a comment about the same function I'm using :

// Due to canonicalize()'s use of GetFinalPathNameByHandleW() on Windows, the resolved path
// starts with '\\?\' to extend the limit of a given path to 32,767 wide characters.
//
// To address this issue, we remove this prepended string if available.
//
// Source:
// http://stackoverflow.com/questions/31439011/getfinalpathnamebyhandle-without-prepended

maybe I've done something not thread-safe. Investigating further

@dgunay
Copy link
Contributor Author

dgunay commented Feb 21, 2022

Wasn't able to reproduce using this setup on Windows 11:

$ gcc -v
Using built-in specs.
COLLECT_GCC=C:\Users\devin\Documents\compilers\mingw32\bin\gcc.exe
COLLECT_LTO_WRAPPER=c:/users/devin/documents/compilers/mingw32/bin/../libexec/gcc/i686-w64-mingw32/10.3.1/lto-wrapper.exe
Target: i686-w64-mingw32
Configured with: ../configure --prefix=/R/winlibs32_stage/inst_gcc-10-20211112/share/gcc --build=i686-w64-mingw32 --host=i686-w64-mingw32 --with-pkgversion='MinGW-W64 i686-posix-dwarf, built by Brecht Sanders' --with-tune=generic --enable-checking=release --enable-threads=posix --with-dwarf2 --disable-sjlj-exceptions --disable-libunwind-exceptions --disable-serial-configure --disable-bootstrap --enable-host-shared --enable-plugin --disable-default-ssp --disable-rpath --enable-libstdcxx-pch --enable-libstdcxx-time=yes --disable-libstdcxx-debug --disable-version-specific-runtime-libs --with-stabs --disable-symvers --enable-languages=c,c++,fortran,lto,objc,obj-c++ --disable-gold --disable-nls --disable-stage1-checking --disable-win32-registry --disable-multilib --enable-ld --enable-libquadmath --enable-libada --enable-libssp --enable-libstdcxx --enable-lto --enable-fully-dynamic-string --enable-libgomp --enable-graphite --enable-mingw-wildcard --with-mpc=/d/Prog/winlibs32_stage/custombuilt --with-mpfr=/d/Prog/winlibs32_stage/custombuilt --with-gmp=/d/Prog/winlibs32_stage/custombuilt --with-isl=/d/Prog/winlibs32_stage/custombuilt --enable-install-libiberty --enable-__cxa_atexit --without-included-gettext --with-diagnostics-color=auto --enable-clocale=generic --with-libiconv --with-system-zlib --with-build-sysroot=/R/winlibs32_stage/gcc-10-20211112/build_mingw/mingw-w64 --enable-large-address-aware CFLAGS=-I/d/Prog/winlibs32_stage/custombuilt/include/libdl-win32
Thread model: posix
Supported LTO compression algorithms: zlib zstd
gcc version 10.3.1 20211112 (MinGW-W64 i686-posix-dwarf, built by Brecht Sanders)
$ rustup show
Default host: x86_64-pc-windows-gnu
rustup home:  C:\Users\devin\.rustup

installed toolchains
--------------------

stable-i686-pc-windows-gnu (default)
stable-x86_64-pc-windows-gnu
nightly-x86_64-pc-windows-gnu

active toolchain
----------------

stable-i686-pc-windows-gnu (default)
rustc 1.58.1 (db9d1b20b 2022-01-20)

Very weird. I'll see if I can get closer to the CI environment when I have time.

@dgunay
Copy link
Contributor Author

dgunay commented Feb 23, 2022

oh wait! I repro'd it locally (turns out rustup toolchain default didn't recompile the tests with the failing toolchain ), investigating

@tertsdiepraam
Copy link
Member

I think they're due to another PR (https://github.com/uutils/coreutils/pull/3182/checks), so you don't have to fix them :)

@sylvestre
Copy link
Contributor

Wahou
Warning: Changes from main: PASS +3 / FAIL -3 / ERROR +0 / SKIP +0
bravo

@dgunay
Copy link
Contributor Author

dgunay commented Feb 24, 2022

@sylvestre @tertsdiepraam - should I rebase onto main or merge main back into my branch?

@tertsdiepraam
Copy link
Member

Preferably rebase, but I don't think it's necessary. GitHub executes the actions on a temporary commit that merges your branch into main. So I've restarted the CI, which should be enough.

src/uu/touch/src/touch.rs Show resolved Hide resolved
src/uu/touch/src/touch.rs Show resolved Hide resolved
@dgunay dgunay force-pushed the bug/stdin-touch-change-mtime branch from 27d8872 to ef689c9 Compare March 2, 2022 02:41
@dgunay
Copy link
Contributor Author

dgunay commented Mar 2, 2022

Never done a rebase like that so I think I might have messed up the history a little.

@sylvestre
Copy link
Contributor

cool:

Warning: Congrats! The gnu test tests/touch/empty-file is now passing!
Warning: Congrats! The gnu test tests/touch/read-only is now passing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

touch doesn't support -
3 participants