Skip to content
Draft
8 changes: 8 additions & 0 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -107,3 +107,11 @@ jobs:
GH_TOKEN: ${{github.token}}
RELEASE_PLEASE_TAG_NAME: ${{steps.release.outputs.tag_name}}
if: steps.release.outputs.release_created

distcheck-macos:
runs-on: macos-latest
steps:
- uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
with:
persist-credentials: false
- run: env PYTESTFLAGS="--verbose -p no:cacheprovider --color=yes" test/macos-script.sh
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to confirm, is --color=yes needed here? We don't seem to add it in Linux envs, but do get color nevertheless.

24 changes: 24 additions & 0 deletions test/macos-script.sh
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No that strong opinions, but perhaps we could take a look if it would become more messier than it's good for if we'd combine this with test/docker/entrypoint.sh with appropriate conditionals to avoid duplicating a bunch of things.

Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
#!/bin/sh -eux

# Note that this script is intended to be run only in throwaway environments;
# it may install undesirable things to system locations (if it succeeds in
# that).

brew install \
automake \
bash

python3 -m venv venv
#shellcheck disable=SC1091
source venv/bin/activate
python3 -m pip install -r test/requirements.txt

export bashcomp_bash=bash
env

autoreconf -i
./configure
make -j

make distcheck \
PYTESTFLAGS="${PYTESTFLAGS---verbose -p no:cacheprovider --numprocesses=auto --dist=loadfile}"
6 changes: 4 additions & 2 deletions test/t/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -776,7 +776,9 @@ def startswith(self, prefix: str) -> bool:
return self.output.startswith(prefix)

def _items(self) -> List[str]:
return [x.strip() for x in self.output.strip().splitlines()]
return sorted(
[x.strip() for x in self.output.strip().splitlines() if x]
)

def __eq__(self, expected: object) -> bool:
"""
Expand All @@ -791,7 +793,7 @@ def __eq__(self, expected: object) -> bool:
return False
else:
expiter = expected
return self._items() == expiter
return self._items() == sorted(expiter)

def __contains__(self, item: str) -> bool:
return item in self._items()
Expand Down
2 changes: 1 addition & 1 deletion test/t/test_tar.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ def test_24(self, completion):

# Test compression detection of gnu style options
@pytest.mark.complete("tar --extract --xz --file ", cwd="tar")
def test_25(self, completion):
def test_25(self, completion, gnu_tar):
assert completion == "archive.tar.xz dir/ dir2/".split()

# TODO: "tar tf escape.tar a/b"
7 changes: 1 addition & 6 deletions test/t/test_vipw.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,7 @@
import sys

import pytest


class TestVipw:
@pytest.mark.complete("vipw -", require_cmd=True)
def test_1(self, completion):
if sys.platform == "darwin":
assert not completion # takes no options
else:
assert completion
assert completion
5 changes: 4 additions & 1 deletion test/t/unit/test_unit_dequote.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import re

import pytest

from conftest import assert_bash_exec, bash_env_saved
Expand Down Expand Up @@ -38,7 +40,8 @@ def test_5_brace(self, bash, functions):

def test_6_glob(self, bash, functions):
output = assert_bash_exec(bash, "__tester 'a?b'", want_output=True)
assert output.strip() == "<a b><a$b><a&b><a'b>"
items = sorted(re.findall(r"<[^>]*>", output))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not immediately clear to me why this is needed on macOS and not on others, could we elaborate in a comment?

...after having a look further in the PR, it seems various sorts are added elsewhere as well. That smells a bit as if the collation options would not be properly in effect. Or are they, but just sort differently in macOS nevertheless?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not sure what's up with that. I just checked and LC_COLLATE seems to be set to C. I'll need to look into that

assert "".join(items) == "<a b><a$b><a&b><a'b>"

def test_7_quote_1(self, bash, functions):
output = assert_bash_exec(
Expand Down
7 changes: 6 additions & 1 deletion test/t/unit/test_unit_load.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import os
import sys

import pytest

Expand Down Expand Up @@ -33,7 +34,11 @@ def fixture_dir(self, request, bash):
assert_bash_exec(
bash, "ln -sf ../prefix1/sbin/cmd2 %s/bin/cmd2" % tmpdir
)
return str(tmpdir)
yield str(tmpdir)
if sys.platform == "darwin":
assert_bash_exec(bash, "sudo rm -rf %s/*" % tmpdir)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love running sudo in the test suite, but I couldn't find a another way to delete these files

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we need to do this manual removal in the first place. In prepare_fixture_dir we add a finalizer that is supposed to clean up the temp dir altogether. Does that not work here? Does it work in other places where we use it to set up a fixture dir?

But if it is needed, some followup questions:

Can we assume sudo is present? What if it asks for credentials?

Any idea if this is needed on all macOS setups, or just the GH one?

If it's a GH only thing, and perhaps otherwise as well, I suppose we could just not fail on error here if the remove fails and let the error message come through so people can clean up manually.

Let's add a comment with details why it doesn't work without it here.

Maybe consider doing the sudo rm in the prepare_fixture_dir finalizer on darwin, if it is needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why we need to do this manual removal in the first place. In prepare_fixture_dir we add a finalizer that is supposed to clean up the temp dir altogether. Does that not work here? Does it work in other places where we use it to set up a fixture dir?

From what I gathered, the issue is that this is the only place where we create more files and folders inside the fixture_dir. Those extra files are created by other processes and not python, so I think that's what causes the issue. See this note in the removefile() syscall:

Write protected files owned by another process cannot be removed by
removefile(), regardless of the permissions on the directory containing
the file.

I might have just not found the correct magic spell to make these files deletable without sudo

But if it is needed, some followup questions:

Can we assume sudo is present? What if it asks for credentials?

Yeah that's a problem. It seems the default behavior is asking for a password

Any idea if this is needed on all macOS setups, or just the GH one?

I don't know, the man pages seem to imply this will happen for all macOS's, but maybe that's not the real issue. I don't have a physical one to test on.

If it's a GH only thing, and perhaps otherwise as well, I suppose we could just not fail on error here if the remove fails and let the error message come through so people can clean up manually.

I'm not sure what a good way to let the error come through without failing the tests is

Let's add a comment with details why it doesn't work without it here.

Maybe consider doing the sudo rm in the prepare_fixture_dir finalizer on darwin, if it is needed?

prepare_fixture_dir doesn't currently have access to bash, so it can't run sudo. It also isn't needed for all of the uses of it. I can make it accept a bash instance if you think that's better though.

else:
assert_bash_exec(bash, "rm -rf %s/*" % tmpdir)

def test_userdir_1(self, bash, fixture_dir):
with bash_env_saved(bash) as bash_env:
Expand Down