Draft pull request: Add Vapor and Linux support #642

ronaldmannak · 2022-10-31T00:27:49Z

Quick and dirty fix to get web3swift to work on Linux and Vapor. Comments how to improve the code are welcome.

Known issues:

Didn't have time to figure out how to get /dev/random to work and added a cryptographically insecure temporary solution. This needs to be fixed. The temporary code that needs to be replaced in URandom.swift
Not all unit tests pass. I haven't looked into it in detail yet, and would appreciate some help here. See build log here: https://github.com/MoonfishApp/web3swift/actions/runs/3357648189/jobs/5563603718#step:5:503

.github/workflows/ubuntu.yml

JeneaVranceanu · 2022-11-02T19:53:47Z

It looks like tests failed for a commit where CI for Ubuntu wasn't ready yet.

@yaroslavyaroslav Could you please approve the workflow for Ubuntu for the latest commit? According to what I saw all tests should pass.

yaroslavyaroslav · 2022-11-04T05:21:26Z

@JeneaVranceanu i’m unable to do so, like literally no such button. Seems like your lint PR broke this one and it required to be rebased from develop once more.

It was really necessary although, since there was a mess (and there still is) within code, so code was haunted for such linting rather sooner or later.

janndriessen · 2022-11-06T10:08:46Z

@JeneaVranceanu i’m unable to do so, like literally no such button. Seems like your lint PR broke this one and it required to be rebased from develop once more.

It was really necessary although, since there was a mess (and there still is) within code, so code was haunted for such linting rather sooner or later.

yea I had to rebase mine as well. that could be one of the issues.

yaroslavyaroslav · 2022-11-14T06:41:19Z

One things is matters for me is to avoid #if os(Linux) as much as it possible. So I'd wish to take some time to overthought it, how it could be designed properly. For now I see only one place where such case is really required: The one that overrides getData() async. So I need to look very closely into each case where it is used and ensure that there's none options to avoid them. Means this one could take a while.

ronaldmannak · 2022-11-14T15:08:55Z

@yaroslavyaroslav I'd love to avoid that as well, but I'm not sure what you mean that all except one are 'not really required'. How are you proposing to solve to the other cases?

yaroslavyaroslav

I definitely agree with making conditional imports in several files where some URLSession related methods implemented. There's two for now.

On other hand I don't agree with usage of conditional imports that is happens to handle URLSession call within any internal method: this one could be extracted into separate function that in turn could be placed file where such conditional import provided already.

I don't p agree with any #if conditions within any method implementation as well. Those cases could be handled by either separate type extensions, that is used @available(Linux) clause, or separate type stored in separate file which availability manages in Package.swift.

Also I consider it important to state that mixing platform related code is leading to a huge codebase mess almost instantly, so I'd encourage any person to avoid it at all costs.

Sources/Core/EthereumNetwork/Request/APIRequest+Methods.swift

Sources/Core/EthereumNetwork/Utility/Async+BackwardCapability.swift

yaroslavyaroslav · 2022-11-15T18:14:12Z

Sources/Core/Structure/SECP256k1.swift

+        #if os(Linux)
+//        return Data(URandom().bytes(count: length))
+        return try? Data.random(length: length)
+        #else


Seems like it's good time to change this sepc256k implementation to those that bitcoin maintains, that did @JeneaVranceanu proposed in the past. I'd bet it's cross platform.

It should be cross-platform as it's written in C but it's not a solution here as the code you are commenting on is our custom extension function and it's just the question of how to get secure random bytes on Linux.

I'd not complicate this PR with the secp256k1 update.

yaroslavyaroslav · 2022-11-15T18:17:35Z

Sources/Core/Structure/Web3ProviderProtocol.swift

+#if canImport(FoundationNetworking)
+import FoundationNetworking
+#endif


In this proposal I see that URL doesn't included within FoundationNetwork, is it necessary here?

Let me double check if this one is necessary

yaroslavyaroslav · 2022-11-15T18:20:54Z

Sources/Core/Utility/Data+Extension.swift

+        #if os(Linux)
+//        return Data(URandom().bytes(count: length)) 
+        return try? Data.random(length: length)
+        #else


If this is necessary, why btw? But if it is, it's better to implement it in a way as backward capability implemented above.

Means implement exact same methods as existed one, but marked it with @available(obsolete) on all Apple platforms.

The issue is SecRandomCopyBytes isn't available on Linux. I was trying to avoid spinning up a new process and use dev/random, but now believe it might be inevitable.

yaroslavyaroslav · 2022-11-15T18:22:25Z

Sources/Core/Utility/URandom.swift

+//  Created by Ronald Mannak on 10/29/22.
+//
+
+import Foundation


Why are we importing libc here?

yaroslavyaroslav · 2022-11-15T18:24:06Z

Sources/web3swift/Browser/Bridge.swift

+#if !os(Linux)
 import WebKit


Guess this file should be excluded from the sources on the Linux platform in a manner that it is now for macOS one. Means it should be ignored in Package.swift.

yaroslavyaroslav · 2022-11-15T18:26:14Z

Sources/web3swift/Web3/Web3+HttpProvider.swift

+#if canImport(FoundationNetworking)
+import FoundationNetworking
+#endif


And this one should be refactored to not use URLSession in it (it was a rough one implementation within 3.0.0 release, so it's could be changed).

yaroslavyaroslav · 2022-11-15T18:27:53Z

Sources/web3swift/Utils/EIP/EIP67Code.swift

+#if !os(Linux)
 import CoreImage
+#endif


We've got to investigate what the hell happening here, and why ERC spec file required CoreImage as an import?

I believe it's for the creation of the QR code, which is convenient to have on iOS and macOS I assume.

I see, so we really could consider just to crop this file for a Linux platform until we figured out how we could implement it properly.

yaroslavyaroslav · 2022-11-15T18:59:35Z

Worth to say that I'm considering that adding new platform support is a big step further, in the meaning, that it possibly wouldn't gets easy, and could require some preparation to provide it into the prod.

I'd propose to create separate long living branch linux where all the work could be done appropriately with no rush, and that could be reached by any folks who need linux so hard that are considering to use pre released version of the lib.

Also that'll allow to update that branch with latest lib releases if any and any fixes, we can even introduce some pre-release tagging there.

I mean this one is a really good start for linux branch, but as for me it's a far away from production ready so far.

yaroslavyaroslav · 2022-11-15T19:00:49Z

But anyway, maybe I'm overreacting here, and other folks has another look onto this @JeneaVranceanu @janndriessen @mloit

yaroslavyaroslav · 2022-11-17T14:45:55Z

I've changed the base of that PR to new created external branch for Linux

yaroslavyaroslav · 2022-11-24T10:11:19Z

@ronaldmannak And now you have push rights to that branch.

This exact PR also could be merged in it.

JeneaVranceanu · 2023-04-03T08:36:36Z

@ronaldmannak Hello!
I'll look into this PR once again today. It would be great to merge it into development.
Wanted to ask if you have any updates that you'd like to push to this branch?

JeneaVranceanu · 2023-04-12T17:41:40Z

@ronaldmannak I'm closing this PR in favour of #810 as it has all your commits. I wasn't able to update this PR so I had to focus on a new one.
If you have any updates please base them on MoonfishApp/develop or linux-support branches of this repository.

@yaroslavyaroslav All comments mentioned here will be addressed in the #810

ronaldmannak added 30 commits October 20, 2022 21:16

exclude ios vc by default

280316c

Merge branch 'skywinder:develop' into develop

1850b46

Import FoundationNetworking for Linux

3461cb8

URLsession to FoundationNetworking

773ba92

FoundationNetworking preceeding Foundation

f14a899

Add Linux alternative to SecRandomCopyBytes

33479ee

Fix return random

c8fc0a7

Fix count

9898f0d

Import Vapor

5f90f67

Add [UInt8].random

8460d92

Fix [UInt8] to Data

b7b020d

Excluded WebKit extension for Linux

d4955d4

Disabled CoreImage for Linux

855ad8e

Comment out random

936ff2a

Rename random to randomData

268564f

fix randomData

8d7c7df

Add Ubuntu CI workflow

8486802

Rename swift action to ubuntu

44039d2

Add dev/random

9e341a3

Use URandom

93a89c1

Swift to temp random

ec86b96

Add try?

60b181b

Remove force unwraps in unit test

5976670

Force actions to run

84528e2

Update Ubuntu actions

cdf4ad4

Run tests in single step

616168f

Install ganache

bea91b5

Install Node

ab991d5

ping localhost

bb0a410

install nc

71117e3

ronaldmannak added 6 commits November 1, 2022 12:23

remote tests only

3cdb528

fix yaml error

1f51faf

Revert to default swift test

96191e9

fix localhost typo

b7b77a4

Intentional wrong port. Should fail

f3c7125

revert to correct port

71ec91d

JeneaVranceanu reviewed Nov 2, 2022

View reviewed changes

.github/workflows/ubuntu.yml Outdated Show resolved Hide resolved

ronaldmannak added 2 commits November 3, 2022 12:37

Update Ubunti CI script

ce46ea7

The inevitable “I love YAML” commit

7052ac7

yaroslavyaroslav marked this pull request as ready for review November 4, 2022 05:17

yaroslavyaroslav added the enhancement New feature or request label Nov 11, 2022

JeneaVranceanu added the linux Issue or PR mainly about issue/feature/question on Linux OS label Nov 13, 2022

yaroslavyaroslav requested changes Nov 15, 2022

View reviewed changes

janndriessen marked this pull request as draft November 17, 2022 14:09

yaroslavyaroslav changed the base branch from develop to linux-support November 17, 2022 14:45

yaroslavyaroslav approved these changes Nov 24, 2022

View reviewed changes

JeneaVranceanu mentioned this pull request Apr 5, 2023

[WIP] feat: Linux support #810

Open

JeneaVranceanu closed this Apr 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Draft pull request: Add Vapor and Linux support #642

Draft pull request: Add Vapor and Linux support #642

ronaldmannak commented Oct 31, 2022

JeneaVranceanu commented Nov 2, 2022

yaroslavyaroslav commented Nov 4, 2022 •

edited

Loading

janndriessen commented Nov 6, 2022

yaroslavyaroslav commented Nov 14, 2022 •

edited

Loading

ronaldmannak commented Nov 14, 2022

yaroslavyaroslav left a comment

yaroslavyaroslav Nov 15, 2022

JeneaVranceanu Nov 15, 2022

yaroslavyaroslav Nov 15, 2022

ronaldmannak Nov 15, 2022

yaroslavyaroslav Nov 15, 2022

ronaldmannak Nov 15, 2022

yaroslavyaroslav Nov 15, 2022

yaroslavyaroslav Nov 15, 2022

yaroslavyaroslav Nov 15, 2022 •

edited

Loading

yaroslavyaroslav Nov 15, 2022

ronaldmannak Nov 15, 2022

yaroslavyaroslav Nov 16, 2022

yaroslavyaroslav commented Nov 15, 2022

yaroslavyaroslav commented Nov 15, 2022 •

edited

Loading

yaroslavyaroslav commented Nov 17, 2022

yaroslavyaroslav commented Nov 24, 2022 •

edited

Loading

JeneaVranceanu commented Apr 3, 2023

JeneaVranceanu commented Apr 12, 2023

Draft pull request: Add Vapor and Linux support #642

Draft pull request: Add Vapor and Linux support #642

Conversation

ronaldmannak commented Oct 31, 2022

JeneaVranceanu commented Nov 2, 2022

yaroslavyaroslav commented Nov 4, 2022 • edited Loading

janndriessen commented Nov 6, 2022

yaroslavyaroslav commented Nov 14, 2022 • edited Loading

ronaldmannak commented Nov 14, 2022

yaroslavyaroslav left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslavyaroslav Nov 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaroslavyaroslav commented Nov 15, 2022

yaroslavyaroslav commented Nov 15, 2022 • edited Loading

yaroslavyaroslav commented Nov 17, 2022

yaroslavyaroslav commented Nov 24, 2022 • edited Loading

JeneaVranceanu commented Apr 3, 2023

JeneaVranceanu commented Apr 12, 2023

yaroslavyaroslav commented Nov 4, 2022 •

edited

Loading

yaroslavyaroslav commented Nov 14, 2022 •

edited

Loading

yaroslavyaroslav Nov 15, 2022 •

edited

Loading

yaroslavyaroslav commented Nov 15, 2022 •

edited

Loading

yaroslavyaroslav commented Nov 24, 2022 •

edited

Loading