Replace deprecated shell commands

[a1346054]

• What does this PR aim to accomplish?:

Increased compatibility with systems and removal of deprecation warnings.

• How does this PR accomplish the above?:

Removes deprecated and nonstandard shell utilities egrep and which with standardized versions grep -E and command -v

• What documentation changes (if any) are needed to support this PR?:

None.

---

By submitting this pull request, I confirm the following:

1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
2. I have commented my proposed changes within the code and I have tested my changes.
3. I am willing to help maintain this change if there are issues with it later.
4. It is compatible with the EUPL 1.2 license
5. I have squashed any insignificant commits. (git rebase)

---

• I have read the above and my PR is ready for review. Check this box to confirm

[MichaIng]

The which sh mocks in tests need to be adjusted accordingly: https://github.com/pi-hole/pi-hole/blob/0f74165/test/test_any_automated_install.py#L795

[a1346054]

I'm looking into it but can't seem to get the syntax right for the tests to pass.

[rdwebdesign]

I'm not sure if the error is related to the tests syntax.
Reading the (really messy) test output, I found assertion errors on lines 677, 756, 783 and 809, like this:

2022-09-18T20:28:02.7160172Z test_any_automated_install.py:677: AssertionError

In every case, the returned value was different than the expected one. Example (line 677):

• expected output = Detected AArch64 (64 Bit ARM) processor
• returned value = Detected ARMv5 (or newer) processor

But I have no idea why this is happening.

[dschaper]

    mock_command('uname', {'-m': ('armv8a', '0')}, host)
    # mock `command -v sh` to return `/bin/sh`
    mock_command('command', {'-v sh': ('/bin/sh', '0')}, host)

[dschaper]

But I have no idea why this is happening.

Because the code to mock what platform is being reported has changed but the test haven't. So every test now checks the actual ldd ${which sh} response for the platform the container is running on. Changing the test to use the mocked ldd ${command -v sh} response will let us control what platform is being mocked in the tests.

[rdwebdesign]

I thought both codes would report the same output.

[MichaIng]

I thought both codes would report the same output.

They usually do, but in the test environment the output needs to be faked/mocked for system/architecture dependant tests. And command -v sh isn't mocked yet, hence does not produce the output required/expected for the next steps.

EDIT: I guess both return /usr/bin/sh, so an alternative might be to remove the which mock and adjust the next to mock ldd /usr/bin/sh instead of ldd /bin/sh.

[yubiuser]

@a1346054

You basically need to change 8 lines of code (+8 lines of comments):

pi-hole/test/test_any_automated_install.py

Lines 622 to 623 in 997a771

	# mock `which sh` to return `/bin/sh`
	mock_command("which", {"sh": ("/bin/sh", "0")}, host)

pi-hole/test/test_any_automated_install.py

Lines 650 to 651 in 997a771

	# mock `which sh` to return `/bin/sh`
	mock_command("which", {"sh": ("/bin/sh", "0")}, host)

pi-hole/test/test_any_automated_install.py

Lines 678 to 679 in 997a771

	# mock `which sh` to return `/bin/sh`
	mock_command("which", {"sh": ("/bin/sh", "0")}, host)

pi-hole/test/test_any_automated_install.py

Lines 707 to 708 in 997a771

	# mock `which sh` to return `/bin/sh`
	mock_command("which", {"sh": ("/bin/sh", "0")}, host)

pi-hole/test/test_any_automated_install.py

Lines 737 to 738 in 997a771

	# mock `which sh` to return `/bin/sh`
	mock_command("which", {"sh": ("/bin/sh", "0")}, host)

pi-hole/test/test_any_automated_install.py

Lines 766 to 767 in 997a771

	# mock `which sh` to return `/bin/sh`
	mock_command("which", {"sh": ("/bin/sh", "0")}, host)

pi-hole/test/test_any_automated_install.py

Lines 792 to 793 in 997a771

	# mock `which sh` to return `/bin/sh`
	mock_command("which", {"sh": ("/bin/sh", "0")}, host)

pi-hole/test/test_any_automated_install.py

Lines 816 to 817 in 997a771

	# mock `which sh` to return `/bin/sh`
	mock_command("which", {"sh": ("/bin/sh", "0")}, host)

Replace the which line with command line from #4907 (comment)

---

P.S. You will also need to rebase your code on the latest development.

[MichaIng]

Strange, tests fail with the same error. Mocking multiple arguments via -v sh works, or need individual arguments be separated? Or does it have issues with shell internals (which is a dedicated executable, command not)?

[a1346054]

Or does it have issues with shell internals (which is a dedicated executable, command not)?

That doesn't seem to be the problem according to the failure logs in the tests.

It expects return of a different architecture than the one that is actually being returned. I need to look into where it actually performs that check.

[MichaIng]

https://github.com/pi-hole/pi-hole/blob/e5695f8/automated%20install/basic-install.sh#L2270-L2303

Since the ldd command and mock has not changed, it must be the command -v sh which does not return the expected path, hence is still not mocked correctly.

Since the two tests on Debian succeed, I guess command -v sh returns /bin/sh there OOTB, hence the container does not have the /bin => /usr/bin symlink yet, while the ones for the other distros have it.

[yubiuser]

Interestingly, it's passing on Debian OS....

[MichaIng]

Here it's the other way round, Debian fails, all others succeed: https://github.com/a1346054/pi-hole/commit/c650e42 => https://github.com/MichaIng/pi-hole/actions/runs/3229542602/jobs/5286970802
So in those containers, /bin => /usr/bin is a symlink, so that with default PATH /usr/bin/sh is returned by command -v sh, while on Debian the symlink does not exist, so that command -v sh returns /bin/sh by default. Official Debian (debootstrap) has these symlinks by default, so it's the container developers which for some reason chose to not have it created 🤔.

However, this proves that the mock does not work, as mentioned above, probably because command is a shell internal which probably cannot be mocked by tox that way.

[MichaIng]

Jep, it is like that, if I interpret the test helper correctly: https://github.com/pi-hole/pi-hole/blob/0df38cd/test/conftest.py#L55
The mock is done by creating a script with the same name in /usr/local/bin, which is used with highest priority in default PATH, hence overrides any /bin and /usr/bin command. But shell internals have highest priorities, hence cannot be overridden by any executable in PATH.

Easiest solution is probably to mock both, ldd /bin/sh and ldd /usr/bin/sh (and remove the command/which mock) and in parallel check why the underlying Debian containers do not have the common /bin => /usr/bin symlink in place. It is default on any modern distro I'm aware of (including Debian!), so any container should follow this convention.

EDIT: The container is a mutation of the official Debian container, which indeed ships without the symlinks. I opened an issue to suggest just this: debuerreotype/docker-debian-artifacts#180

[MichaIng]

PR up to fix tests in this PR: https://github.com/a1346054/pi-hole/pull/1
Tests all green with this: https://github.com/MichaIng/pi-hole/actions/runs/3234059145
Using the glob */bin/sh as single argument should be possible as well, but better to be explicit here.

[yubiuser]

Pretty clever!

[yubiuser]

You need to un-draft to trigger the tests