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

fix: workaround for macOS AF_UNIX path error #8208

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

king-11
Copy link
Contributor

@king-11 king-11 commented Apr 4, 2025

symlink the socket to a tempfile which has a shorter path

Important

25.02 FREEZE JANUARY 31ST: Non-bugfix PRs not ready by this date will wait for 25.05.

Checklist

Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:

  • The changelog has been updated in the relevant commit(s) according to the guidelines.
  • Tests have been added or modified to reflect the changes.
  • Documentation has been reviewed and updated as needed.
  • Related issues have been listed and linked, including any that this PR closes.

@king-11 king-11 requested a review from cdecker as a code owner April 4, 2025 18:08
@king-11 king-11 force-pushed the king-11/macos_unix_path branch from 660e8ad to d57cb29 Compare April 4, 2025 18:09
@king-11
Copy link
Contributor Author

king-11 commented Apr 4, 2025

I think its possible to do the same thing on linux too shall we merge both the workarounds?

@vincenzopalazzo
Copy link
Collaborator

Does this happen when running the integration tests? Can we also post the stack trace that you are receiving in the commit body?

P.S: probably you want to remove also the merge commit and use the git rebase upstrea/master for a clean git history

@king-11
Copy link
Contributor Author

king-11 commented Apr 5, 2025

P.S: probably you want to remove also the merge commit and use the git rebase upstrea/master for a clean git history

yeah the this update branch button ruined the history will rebase

image

Also shared the trace in discord

    @pytest.mark.openchannel('v1')
    @pytest.mark.openchannel('v2')
    def test_pay(node_factory):
>       l1, l2 = node_factory.line_graph(2)
...
FAILED tests/test_pay.py::test_pay - OSError: AF_UNIX path too lon

@king-11 king-11 force-pushed the king-11/macos_unix_path branch from 58bbd29 to d57cb29 Compare April 5, 2025 14:22
@vincenzopalazzo
Copy link
Collaborator

cool, would be good to have the stackstrace inside the commit body, just git commit --amend

    @pytest.mark.openchannel('v1')
    @pytest.mark.openchannel('v2')
    def test_pay(node_factory):
>       l1, l2 = node_factory.line_graph(2)
...
FAILED tests/test_pay.py::test_pay - OSError: AF_UNIX path too lon

This will help us to remember in 20 years why this commit was introduced

@king-11 king-11 force-pushed the king-11/macos_unix_path branch from d57cb29 to 7be490b Compare April 5, 2025 14:35
@king-11
Copy link
Contributor Author

king-11 commented Apr 5, 2025

done

    @pytest.mark.openchannel('v1')
    @pytest.mark.openchannel('v2')
    def test_pay(node_factory):
>       l1, l2 = node_factory.line_graph(2)
...
FAILED tests/test_pay.py::test_pay - OSError: AF_UNIX path too long

Changelog-None: symlink the socket to a tempfile which has a shorter path

Signed-off-by: Lakshya Singh <[email protected]>
@king-11 king-11 force-pushed the king-11/macos_unix_path branch from 7be490b to a5fd34d Compare April 5, 2025 14:36
@king-11
Copy link
Contributor Author

king-11 commented Apr 5, 2025

@vincenzopalazzo any thoughts about merging both unix and mac workarounds? I think temp directory should work in both the cases.

@vincenzopalazzo
Copy link
Collaborator

I think its possible to do the same thing on linux too shall we merge both the workarounds?

Not sure if we are having any problem on linux? If not, it is better to wait to find a problem before merging workarounds that required two pages of comments to remember why we are doing it, or I am missing something?

@king-11
Copy link
Contributor Author

king-11 commented Apr 7, 2025

Sounds good @vincenzopalazzo then we this one is good for merge.

Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Ack a5fd34d

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.

3 participants