Skip to content
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

Add macOS to the pipeline #1395

Closed
wants to merge 3 commits into from
Closed

Conversation

Kludex
Copy link
Member

@Kludex Kludex commented Mar 5, 2022

As mentioned on the PR above:

The path we use to bind to the socket is 127 bytes length. AF_UNIX has only 108 bytes allocated for it, so we keep receiving OSError: AF_UNIX path too long.

Solutions that I've tried:

  • Use basetemp, but it doesn't work as expected on Windows.
  • Use tmp_path_factory, but still doesn't work on some systems. It passes on the GitHub CI, because the path is short, but locally the name is still long.
  • Enable test only for Linux systems (I've used the recommended way of checking the system, given the docs.

I've used the last solution, but if you guys have an alternative, feel free to suggest.

EDIT: The goal of the PR evolved due to discussion. Now it's meant to add the macOS machine to the pipeline.

@Kludex Kludex force-pushed the fix/mac-path-size branch from 69226f7 to 2dc87ef Compare March 5, 2022 20:15
@Kludex Kludex requested a review from euri10 March 5, 2022 20:36
@Kludex Kludex requested a review from a team March 19, 2022 17:42
Copy link
Member

@euri10 euri10 left a comment

Choose a reason for hiding this comment

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

it seems you forgot to add mac-os to the CI ?

@Kludex
Copy link
Member Author

Kludex commented Mar 19, 2022

I didn't add it on purpose, as the test suite doesn't pass locally for macOS yet. There are some other tests that needs to be fixed.

I need to check that again, but on GitHub macOS it passes. Maybe related to the M1 chip... Not sure.

In any case, this PR only solves that specific test for macOS.

@euri10
Copy link
Member

euri10 commented Mar 19, 2022

I'd rather see a full PR for adding mac-os to the CI even if several things have to change just for the history, seems cleaner to me, what do you think ?

@Kludex
Copy link
Member Author

Kludex commented Mar 19, 2022

That makes sense 👍

@Kludex Kludex changed the title Shorten generated temporary file paths on tests Add macOS to the pipeline Mar 19, 2022
@euri10 euri10 mentioned this pull request Mar 20, 2022
@euri10
Copy link
Member

euri10 commented Mar 20, 2022

replaced with #1412

@euri10 euri10 closed this Mar 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants