-
Notifications
You must be signed in to change notification settings - Fork 400
Macos ci #1339
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
Macos ci #1339
Conversation
The CI run failed because for some reason it didn't manage to delete the temp directory it creates in test_unit_load.py using shutil.rmtree in prepare_fixture_dir
Thanks for working on this! I think I've seen the temp dir removal issue myself as well. Would be great if we could figure out a workaround. If we can't, we should allow it to fail only on macOS.
e94114a to
34a0d71
Compare
ad44019 to
2e6e2c9
Compare
17a1477 to
eaa8ce1
Compare
eaa8ce1 to
cd1cc08
Compare
f6c04d4 to
4331cfa
Compare
4331cfa to
ede4793
Compare
ede4793 to
687c188
Compare
511147b to
cf77da1
Compare
804b3c9 to
f70330d
Compare
test/t/unit/test_unit_load.py
Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
f70330d to
2b01104
Compare
Also use venv instead of global install brew/python doesn't like installing python packages globally: ``` error: externally-managed-environmen×ばつ This environment is externally managed ```
vipw seems to have -d as an option on macos now. Partially reverts ccf7bf6 "test(dmesg,vipw): expect no options on macOS"
It seems there is some behaviour differences regarding the order completions are returned in between linux and macos, specifically regarding sorting of upper case and lowercase letters. Sorting them ourselves in the tests makes it consistent.
On macOS we need sudo to delete them, maybe because they were created by a different prcoess?
2b01104 to
14c8dce
Compare
There was a problem hiding this comment.
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.
test/t/unit/test_unit_dequote.py
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
test/t/unit/test_unit_load.py
Outdated
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
This reverts commit 8074741.
8b727c3 to
5d79137
Compare
a927bc8 to
9414d6d
Compare
Instead of running bash commands, copy directories and create symlinks using pure python. This should hopefully make macos GHA not fail to delete these directories.
9414d6d to
e70aa46
Compare
Uh oh!
There was an error while loading. Please reload this page.
See #726
Cherry-picked from here: