install: fix issue #3814 #3950

bruecke · 2022-09-17T19:28:11Z

This pull request fixes the the issue mentioned in #3814, so installing multiple files using the "create leading dirs" option combined with the target parameter is now working and the GNU test is passing.

Furthermore this PR fixes more issues:

The verbose option, when using -D, was printing all directories to stdout, instead of only the missing ones.
Also installing only a single file using -Dt is now working as expected.
Installing a single file into a destination file, but the source being a directory, did not fail with the correct error message.

Copy GNU test case
Add testcase to verify the correct behaviour

tests/by-util/test_install.rs

sylvestre · 2022-09-25T16:37:00Z

I guess you saw:


---- test_install::test_install_creating_leading_dirs_with_single_source_and_target_dir stdout ----
current_directory_resolved: 
touch: /tmp/.tmpOS9IXF/source_file_1
run: /home/runner/work/coreutils/coreutils/target/debug/coreutils install -D source_file_1 /tmp/.tmpOS9IXF/missing_target_dir/
thread 'test_install::test_install_creating_leading_dirs_with_single_source_and_target_dir' panicked at ''install: cannot install 'source_file_1' to '/tmp/.tmpOS9IXF/missing_target_dir/': Is a directory
' does not contain 'missing_target_dir/' is not a directory'', tests/common/util.rs:420:9

sylvestre

ping?

bruecke · 2022-10-04T20:04:03Z

I guess you saw:


---- test_install::test_install_creating_leading_dirs_with_single_source_and_target_dir stdout ----
current_directory_resolved: 
touch: /tmp/.tmpOS9IXF/source_file_1
run: /home/runner/work/coreutils/coreutils/target/debug/coreutils install -D source_file_1 /tmp/.tmpOS9IXF/missing_target_dir/
thread 'test_install::test_install_creating_leading_dirs_with_single_source_and_target_dir' panicked at ''install: cannot install 'source_file_1' to '/tmp/.tmpOS9IXF/missing_target_dir/': Is a directory
' does not contain 'missing_target_dir/' is not a directory'', tests/common/util.rs:420:9

Sorry for the late reply. Was a little busy.

Yes I know of this. The problem has been there before: When a non-existing directory path is given as target ending on a '/', the install command fails, but with a wrong error message. At least it is not the same as the original GNU install does.
This test only uncovers that problem.

I also already have a fix for unix systems. I will push it now.
Do we need to support "install" also on windows?

tertsdiepraam · 2022-10-04T22:51:49Z

Install is currently unix-only. You can check that in the docs (https://uutils.github.io/coreutils-docs/user/utils/install.html, icons in the top right). Or in the global Cargo.toml, though it is a bit hard to read.

sylvestre · 2022-10-05T11:34:17Z

But I wonder if there is a good reason not to run on Windows?

bruecke · 2022-10-15T13:05:30Z

Install is currently unix-only. You can check that in the docs (https://uutils.github.io/coreutils-docs/user/utils/install.html, icons in the top right). Or in the global Cargo.toml, though it is a bit hard to read.

I've that seen in the Cargo.toml, but I also think that I've found some special test cases for Windows - something about very long filenames or so. Therefore I was unsure.
Nonetheless with the latest commit it should also work on any non-Unix platform. It is probably slower than the Unix-solution and I was not able to test it yet.

Can this pull request be merged then?

github-actions · 2022-10-15T14:06:54Z

GNU testsuite comparison:

Congrats! The gnu test tests/install/create-leading is no longer failing!
Congrats! The gnu test tests/rm/rm2 is no longer failing!
GNU test failed: tests/misc/tee. tests/misc/tee is passing on 'main'. Maybe you have to rebase?

github-actions · 2022-10-15T14:33:28Z

GNU testsuite comparison:

Congrats! The gnu test tests/install/create-leading is no longer failing!
Congrats! The gnu test tests/rm/rm2 is no longer failing!

bruecke · 2022-10-15T18:32:41Z

While trying to increase the code coverage, I tried some more test cases and compared them with the orig. GNU install.
Turns out it was not behaving the same. I will push another commit next.

Here's the test code to try it our yourself:

export C=coreutils/target/debug/

rm -rf dir1 no-dir2 dir3 file

mkdir -p dir1 dir3
touch file

${C}install file dir1 no-dir2 # fails w/ "is not a directory"
${C}install file dir1 dir3    # fails w/ "omitting directory", but copies file first
${C}install dir1 dir3         # fails w/ "omitting directory"
${C}install dir1              # fails w/ "missing destination file operand", then prints usage
${C}install                   # fails w/ "missing file operand", then prints usage
${C}install -D -t no-dir2     # fails w/ "missing file operand", then prints usage
                              # and does not create target dir

github-actions · 2022-10-20T16:17:27Z

GNU testsuite comparison:

Congrats! The gnu test tests/install/create-leading is no longer failing!

Code was missing the logic to create the target dir when multiple files should be copied and target dir is given by -t option. This simplifies the copy logic also when only one file should be copied to the target dir.

Also adds a unit test to verify the same behaviour as the gnu tools.

Tests various combinations of "-D" with and w/o "-t" when installing either a single file or multiple files into a non existing directory. install -D file1 file2 (-t) not_existing_dir install -D file1 (-t) not_existing_dir/ Also fixes file formatting, spelling and adds some more test asserts.

The install command failed with a different error message than the original GNU install tool. Checking for a trailing slash fixes this. Only works on unix though.

This increases the CI test coverage and also checks for more corner cases to ensure uu_install is compliant with GNU's original. export C=coreutils/target/debug/ rm -rf dir1 no-dir2 dir3 file mkdir -p dir1 dir3 touch file ${C}install dir1/file1 dir1/.. no-dir2 ${C}install dir1/file1 dir1/.. dir3 ${C}install dir1/.. dir3

Also check that install returns the correct error messages, when only a target directory is given via -t and that is is not created (-D option).

This ensures correct (GNU install like) behavior. Tests from the last commit will pass now.

github-actions · 2022-10-22T14:53:08Z

GNU testsuite comparison:

Congrats! The gnu test tests/install/create-leading is no longer failing!

github-actions · 2022-10-22T14:53:20Z

GNU testsuite comparison:

Congrats! The gnu test tests/install/create-leading is no longer failing!

tertsdiepraam

Just one question, otherwise ready to be merged I think! Sorry for taking so long :)

tertsdiepraam · 2022-11-10T23:28:06Z

src/uu/install/src/install.rs

+#[cfg(not(unix))]
+fn is_potential_directory_path(path: &Path) -> bool {
+    let path_str = path.to_string_lossy();
+    path_str.ends_with(MAIN_SEPARATOR) || path_str.ends_with('/') || path.is_dir()


I'm wondering if the second case here is really expected behaviour. Does windows accept trailing / in paths?

Just one question, otherwise ready to be merged I think! Sorry for taking so long :)

No worries! We all have personal lives sometimes :)

I'm wondering if the second case here is really expected behaviour. Does windows accept trailing / in paths?

AFAIK it does. I'm not sure, whether that is a valid delimiter in the standard cmd.exe or powershell already, or if this is only allowed when you use a MinGW or Git-bash shell. Nonetheless you can find the same check already in here uucore::features::fs::canonicalize().

let has_to_be_directory = (miss_mode == MissingHandling::Normal || miss_mode == MissingHandling::Existing) && { let path_str = original.to_string_lossy(); path_str.ends_with(MAIN_SEPARATOR) || path_str.ends_with('/') };

Alright! Sounds good. We can merge this if you fix the conflicts.

The branch is now up to date. Still merging is not possible.
Am I missing something @sylvestre and @tertsdiepraam ?

Maybe the main of your repo is not up to date with the upstream one? I usually do something like this to ensure my main is set to the upstream one:

git switch main git fetch upstream git reset --hard upstream/main

Note: this will delete any changes on your local main

Nevermind, merging is blocked because of the requested changes by @sylvestre, it doesn't have anything to do with git.

github-actions · 2022-12-05T23:28:58Z

GNU testsuite comparison:

Congrats! The gnu test tests/install/create-leading is no longer failing!
GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

sylvestre · 2022-12-06T08:11:30Z

yeah, thanks, approved!

sylvestre reviewed Sep 18, 2022

View reviewed changes

tests/by-util/test_install.rs Show resolved Hide resolved

sylvestre requested changes Oct 3, 2022

View reviewed changes

bruecke requested a review from sylvestre October 15, 2022 13:00

bruecke added 10 commits October 22, 2022 10:23

install: fix installing one file when using -Dt options

bcd6145

install: fix installing multiple files with -Dt

bd9f2d7

Code was missing the logic to create the target dir when multiple files should be copied and target dir is given by -t option. This simplifies the copy logic also when only one file should be copied to the target dir.

install: fix verbose output when using -D

dd3bb8d

Also adds a unit test to verify the same behaviour as the gnu tools.

install: fix error for nonex. dir path ending on /

15544ff

The install command failed with a different error message than the original GNU install tool. Checking for a trailing slash fixes this. Only works on unix though.

install: add windows support when checking for '/'

0e4f7b3

install.rs: fix spelling

126eae6

install: improve test_install_missing_arguments

a275420

Also check that install returns the correct error messages, when only a target directory is given via -t and that is is not created (-D option).

install: rework the checks for missing file args

ea8dd62

This ensures correct (GNU install like) behavior. Tests from the last commit will pass now.

sylvestre force-pushed the install_fix_issue_#3814 branch from 9ede8f8 to ea8dd62 Compare October 22, 2022 08:23

tertsdiepraam reviewed Nov 10, 2022

View reviewed changes

Merge branch 'main' into install_fix_issue_#3814

bdb159e

sylvestre merged commit 42b9b7b into uutils:main Dec 6, 2022

ImperatorStorm mentioned this pull request Feb 7, 2023

install should not fail when target directory doesn't exist #3814

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

install: fix issue #3814 #3950

install: fix issue #3814 #3950

bruecke commented Sep 17, 2022 •

edited

Loading

sylvestre commented Sep 25, 2022

sylvestre left a comment

bruecke commented Oct 4, 2022

tertsdiepraam commented Oct 4, 2022

sylvestre commented Oct 5, 2022

bruecke commented Oct 15, 2022 •

edited

Loading

github-actions bot commented Oct 15, 2022

github-actions bot commented Oct 15, 2022

bruecke commented Oct 15, 2022 •

edited

Loading

github-actions bot commented Oct 20, 2022

github-actions bot commented Oct 22, 2022

github-actions bot commented Oct 22, 2022

tertsdiepraam left a comment

tertsdiepraam Nov 10, 2022

sylvestre Nov 23, 2022

bruecke Nov 28, 2022

bruecke Nov 28, 2022

tertsdiepraam Nov 28, 2022

bruecke Dec 5, 2022

tertsdiepraam Dec 6, 2022 •

edited

Loading

tertsdiepraam Dec 6, 2022

github-actions bot commented Dec 5, 2022

sylvestre commented Dec 6, 2022

install: fix issue #3814 #3950

install: fix issue #3814 #3950

Conversation

bruecke commented Sep 17, 2022 • edited Loading

sylvestre commented Sep 25, 2022

sylvestre left a comment

Choose a reason for hiding this comment

bruecke commented Oct 4, 2022

tertsdiepraam commented Oct 4, 2022

sylvestre commented Oct 5, 2022

bruecke commented Oct 15, 2022 • edited Loading

github-actions bot commented Oct 15, 2022

github-actions bot commented Oct 15, 2022

bruecke commented Oct 15, 2022 • edited Loading

github-actions bot commented Oct 20, 2022

github-actions bot commented Oct 22, 2022

github-actions bot commented Oct 22, 2022

tertsdiepraam left a comment

Choose a reason for hiding this comment

tertsdiepraam Nov 10, 2022

Choose a reason for hiding this comment

sylvestre Nov 23, 2022

Choose a reason for hiding this comment

bruecke Nov 28, 2022

Choose a reason for hiding this comment

bruecke Nov 28, 2022

Choose a reason for hiding this comment

tertsdiepraam Nov 28, 2022

Choose a reason for hiding this comment

bruecke Dec 5, 2022

Choose a reason for hiding this comment

tertsdiepraam Dec 6, 2022 • edited Loading

Choose a reason for hiding this comment

tertsdiepraam Dec 6, 2022

Choose a reason for hiding this comment

github-actions bot commented Dec 5, 2022

sylvestre commented Dec 6, 2022

bruecke commented Sep 17, 2022 •

edited

Loading

bruecke commented Oct 15, 2022 •

edited

Loading

bruecke commented Oct 15, 2022 •

edited

Loading

tertsdiepraam Dec 6, 2022 •

edited

Loading