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

Windows compatiblity #1516

Merged
merged 6 commits into from
Feb 4, 2020
Merged

Windows compatiblity #1516

merged 6 commits into from
Feb 4, 2020

Conversation

coot
Copy link
Contributor

@coot coot commented Jan 28, 2020

This PR includes three things:

  • simplifies ErrorPolicies by removing callback which classifies return values of error policies
  • adds clientSubscriptionWorker which subscribes to a unix socket or named pipe on windows. It is a simplified version of ipSubscriptionWorker.
  • adds ./scripts/test.sh - a script to run tests on widnows - all are green 🎉 😎

It depends on #1499 - which needs to be merge first, it addresses #1501.

@coot coot requested a review from karknu January 28, 2020 14:18
@coot coot force-pushed the coot/windows-api branch from 5948716 to 91bf5f0 Compare January 31, 2020 12:25
@coot coot added the Windows label Jan 31, 2020
@coot coot changed the base branch from master to coot/snockets January 31, 2020 13:01
@coot
Copy link
Contributor Author

coot commented Jan 31, 2020

I changed target to coot/snockets branch (#1499) - just to make it easier for the review. After #1499 is merged I'll re-target it to master back again.

Copy link
Contributor

@karknu karknu left a comment

Choose a reason for hiding this comment

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

LGTM.
I have the same reservation on the use of client in this pr as on #1499.
How will this PR's subscription work interact with #1455? Is there a point to adding it?

{ nsSubscriptionTracer
, nsMuxTracer
, nsHandshakeTracer
, nsErrorPolicyTracer
Copy link
Contributor

Choose a reason for hiding this comment

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

In cardano-sl's style the prefix would be nst not ns. But I'm not sure we have any rules for this in ourboros-network.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know, but we already have nst prefix in NetworkServerTracers :/

@coot
Copy link
Contributor Author

coot commented Feb 3, 2020

How will this PR's subscription work interact with #1455? Is there a point to adding it?

Yes, once snockets are merged they can be used in #1455 in the same way as they are used in Ouroboros.Network.Server. But we can add the snocket support later on, byron release does not require from us to be worried about window's users.

@coot coot force-pushed the coot/snockets branch 3 times, most recently from c0c6e09 to 11ef1c7 Compare February 3, 2020 13:47
@coot coot force-pushed the coot/windows-api branch 2 times, most recently from bfc1efe to 565bb29 Compare February 3, 2020 14:05
@coot
Copy link
Contributor Author

coot commented Feb 3, 2020

I have the same reservation on the use of client in this pr as on #1499.

Fixed :)

coot added 6 commits February 4, 2020 16:52
Run pipe tests on windows using `pipeChannelFromNamedPipe` This patch
makes test-network buildable again and enables pipes tests on Windows.
This allows to connect to the diffusion layer using unix socket or named
pipe.  Currently we don't support local clients connected using a tcp
socket, but this could be easily added in the future.
This only allows to connect to a node using a unix socket or a named
pipe.
A subscription worker which works over ClientSnocket.  It is integrated
into data diffusion `Ouroboros.Network.Diffusion`.

* `ouroboros-network`   - compiler and runs all its tests on Windows.
* `ouroboros-consensus` - windows support is tracked in #1082
This is useful for running all tests on windows.  Currently it does not
include `ouroboros-consensus` tests, since `ouroboros-consensus` does
not compile on windows.
@coot coot force-pushed the coot/windows-api branch from 565bb29 to 2f81dec Compare February 4, 2020 16:17
@coot coot merged commit 3c06a86 into coot/snockets Feb 4, 2020
@iohk-bors iohk-bors bot deleted the coot/windows-api branch February 4, 2020 16:17
@coot
Copy link
Contributor Author

coot commented Feb 4, 2020

I merged this into #1499 - so I have less PRs to re-base.

@coot coot restored the coot/windows-api branch February 4, 2020 16:18
@coot coot deleted the coot/windows-api branch February 4, 2020 16:18
@coot coot restored the coot/windows-api branch February 4, 2020 16:55
@coot coot deleted the coot/windows-api branch February 4, 2020 16:56
iohk-bors bot added a commit that referenced this pull request Feb 28, 2020
1552: windows vectored io for sockets r=coot a=coot

Currently this PR is opened to merge into `coot/windows-api`, but after #1516 is merged it will be re-targeted to `master` - it's just to make the diff adequate.

This PR provides vectored IO for sockets, vectored IO for file handles will be done at a later stage.
It also includes documentation overview, which will be useful for other teams (`cardano-wallet` or `cardano-launcher`). 

Co-authored-by: Marcin Szamotulski <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants