Use short options for ls command

[tsoutsman]

Closes #783

I don't think it's possible to create a regression test as that would require complex CI environments with different ls versions.

[tmccombs]

Please add an entry to the CHANGELOG. Other than that I think this looks good.

[tsoutsman]

@tmccombs done

[tsoutsman]

I'm actually stupid. 5th time's the charm.

[tsoutsman]

I just tried to run ls on an old MacBook I had lying around. It seems like it doesn't support the --color option which outputs ls: illegal option -- - the same error found in the logs. None of the changes I made should have affected the colour options, but I made a quick and dirty fix to check if that's the issue. The colour should be enabled later on with the -G option anyway.

[sharkdp]

I just tried to run ls on an old MacBook I had lying around. It seems like it doesn't support the --color option

That's why we previously only passed the color arg if we have gls (GNU ls) available on macOS. I think it was working correctly.

[tsoutsman]

Ight, sorry about that whole debacle, it should finally be good now.

[peternewman]

I don't think it's possible to create a regression test as that would require complex CI environments with different ls versions.

Thanks @tsoutsman . As mentioned in the issue, this happened on GitHub Actions, so it should be pretty easy to test. Given it looks like there are test runs I'm a bit surprised they haven't caught this before.

I think this was where I had it broken and worked around it:
ssilverman/rdm-schema@3d61366

[tsoutsman]

Thanks @tsoutsman . As mentioned in the issue, this happened on GitHub Actions, so it should be pretty easy to test. Given it > looks like there are test runs I'm a bit surprised they haven't caught this before.

Given how I handled changing 4 lines of code, I think I should avoid touching the CI with a ten-foot pole.

[peternewman]

Interestingly there is a test:

fd/tests/tests.rs

Lines 1764 to 1770 in 224b7f2

	#[test]
	fn test_list_details() {
	let te = TestEnv::new(DEFAULT_DIRS, DEFAULT_FILES);
	// Make sure we can execute 'fd --list-details' without any errors.
	te.assert_success_and_get_output(".", &["--list-details"]);
	}

I'm definitely no rust expert, but even when I add a 20.04 run (#820) (which ubuntu-latest was at the time I hit the issue), there's no mention of any of the tests in that file in the test log that I can see:
https://github.com/peternewman/fd/runs/3279423614?check_suite_focus=true

Which might be why it wasn't caught...

Indeed the docs seem to say it won't run as an integration test, but not how to run it instead:
https://doc.rust-lang.org/book/ch11-03-test-organization.html#integration-tests-for-binary-crates

[sharkdp]

I'm definitely no rust expert, but even when I add a 20.04 run (#820) (which ubuntu-latest was at the time I hit the issue), there's no mention of any of the tests in that file in the test log that I can see:
https://github.com/peternewman/fd/runs/3279423614?check_suite_focus=true

Which might be why it wasn't caught...

These tests do not run on ARM platforms (see CARGO_TEST_OPTIONS in CI scripts). They run on all other platforms though (e.g. https://github.com/peternewman/fd/runs/3279423791?check_suite_focus=true)

[sharkdp]

@tsoutsman Thank you very much.

Given how I handled changing 4 lines of code, I think I should avoid touching the CI with a ten-foot pole.

No worries! 😄 If you (still) think that the code is confusing, please let me know. There's certainly a way to improve it.

I think this PR is good to go. @peternewman ?

[peternewman]

These tests do not run on ARM platforms (see CARGO_TEST_OPTIONS in CI scripts). They run on all other platforms though (e.g. https://github.com/peternewman/fd/runs/3279423791?check_suite_focus=true)

Yes of course, you can't run the binary when cross-compiling...

[peternewman]

I think this PR is good to go. @peternewman ?

Yes, agreed.

Looking back at my issue, it was within a Docker container I had the issue, which is probably far more of a faff to spin up CI wise...