Skip to content

Re-enable System.IO.Pipes.Tests on iOS-based platforms #73258

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.

Sign up for GitHub

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

Jump to bottom
Closed
directhex wants to merge 4 commits into from
Closed

Re-enable System.IO.Pipes.Tests on iOS-based platforms #73258

directhex wants to merge 4 commits into from

Conversation

@directhex
Copy link
Contributor

@directhex directhex commented Aug 2, 2022

This seems fixed by a pipe name shortening at some point in the past

Closes: #51335

@directhex
Re-enable System.IO.Pipes.Tests on iOS-based platforms
272f9c9 
This seems fixed by a pipe name shortening at some point in the past
@ghost ghost assigned directhex Aug 2, 2022
@ghost
Copy link

ghost commented Aug 2, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

This seems fixed by a pipe name shortening at some point in the past

Closes: #51335

Author: directhex
Assignees: -
Labels:

area-Infrastructure-libraries
Milestone: -

@ghost
Copy link

ghost commented Aug 2, 2022

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

This seems fixed by a pipe name shortening at some point in the past

Closes: #51335

Author: directhex
Assignees: directhex
Labels:

area-Infrastructure-mono
Milestone: -

@directhex directhex added os-ios Apple iOS os-tvos Apple tvOS os-maccatalyst MacCatalyst OS labels Aug 2, 2022
@ghost
Copy link

ghost commented Aug 2, 2022

Tagging subscribers to 'os-maccatalyst': @steveisok, @akoeplinger
See info in area-owners.md if you want to be subscribed.

Issue Details

This seems fixed by a pipe name shortening at some point in the past

Closes: #51335

Author: directhex
Assignees: directhex
Labels:

area-Infrastructure-mono, os-ios, os-tvos, os-maccatalyst
Milestone: -

@steveisok
Copy link
Member

steveisok commented Aug 2, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 2, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@directhex
Copy link
Contributor Author

directhex commented Aug 2, 2022

I think we can smash merge on here, the iOS & Catalyst jobs have already passed

@steveisok steveisok self-requested a review August 2, 2022 21:34
@steveisok
Copy link
Member

steveisok commented Aug 2, 2022

Appears to be still failing

https://dev.azure.com/dnceng/public/_build/results?buildId=1918613&view=ms.vss-test-web.build-test-results-tab&runId=49709938&resultId=108995&paneView=debug

@directhex
Work around long socket path on tvOS.
c8c1eb3 
tvOS has a MAX_PATH of 104 characters on domain sockets, and a 90 character
value for GetTempPath. That gives us only 14 characters to play with, and
temp filenames are 12 characters long. We cannot afford the `CoreFxPipe_`
prefix on tvOS, with our tiny character path budget.

Check whether this fixes some or all of dotnet#67853 and dotnet#51390 too.
@directhex
Copy link
Contributor Author

directhex commented Aug 3, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 3, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@directhex
Copy link
Contributor Author

directhex commented Aug 3, 2022

Extracting my notes from a commit message:

tvOS has a MAX_PATH of 104 characters on domain sockets, and a 90 character value for GetTempPath. That gives us only 14 characters to play with, and temp filenames are 12 characters long. We cannot afford the CoreFxPipe_ prefix on tvOS, with our tiny character path budget.

Check whether this fixes some or all of #67853 and #51390 too.

@steveisok
Copy link
Member

steveisok commented Aug 3, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 3, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@directhex
Copy link
Contributor Author

directhex commented Aug 3, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 3, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@runfoapp runfoapp bot mentioned this pull request Aug 3, 2022
Closed
@directhex
Copy link
Contributor Author

directhex commented Aug 3, 2022

I'm going to close this.

We need to PNSE pipes on iOS and tvOS. The 104 characters thing is a ruse - a) we get Permission Denied errors trying to bind to the socket if it fits in the path, and b) the path on iOS/tvOS Simulator is MUCH longer, but the limit isn't. We can't make domain sockets on there, because we need 184 characters minimum for the path & all we have is 104. It's impossible to support.

@directhex directhex closed this Aug 3, 2022
@directhex
Copy link
Contributor Author

directhex commented Aug 3, 2022

Actually, using this PR for testing, since I can't get anything running locally

@directhex directhex reopened this Aug 3, 2022
@directhex directhex added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Aug 3, 2022
@directhex directhex marked this pull request as draft August 3, 2022 20:01
@directhex
Copy link
Contributor Author

directhex commented Aug 3, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 3, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@directhex directhex force-pushed the reenable-pipes-mac branch from 8fdbefd to 4ab77e0 Compare August 3, 2022 21:46
@directhex
Copy link
Contributor Author

directhex commented Aug 3, 2022

/azp run runtime-extra-platforms

@azure-pipelines
Copy link

azure-pipelines bot commented Aug 3, 2022

Azure Pipelines successfully started running 1 pipeline(s).

@directhex directhex closed this Aug 4, 2022
@directhex directhex mentioned this pull request Aug 4, 2022
Merged
@runfoapp runfoapp bot mentioned this pull request Aug 4, 2022
Closed
@karelz karelz mentioned this pull request Aug 7, 2022
Open
@ghost ghost locked as resolved and limited conversation to collaborators Sep 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@steveisok steveisok steveisok approved these changes

@mdh1418 mdh1418 mdh1418 approved these changes

@akoeplinger akoeplinger Awaiting requested review from akoeplinger

@mkhamoyan mkhamoyan Awaiting requested review from mkhamoyan

Assignees

@directhex directhex

Labels

area-Infrastructure-mono NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) os-ios Apple iOS os-maccatalyst MacCatalyst OS os-tvos Apple tvOS

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

System.IO.Pipes.Tests Fails on iOS/tvOS

4 participants

@directhex @steveisok @mdh1418 @ViktorHofer