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

Change build script of uplink-sys crate to support Windows #61

Open
ifraixedes opened this issue Apr 11, 2023 · 8 comments
Open

Change build script of uplink-sys crate to support Windows #61

ifraixedes opened this issue Apr 11, 2023 · 8 comments
Labels
crate:uplink-sys Specific issues / PRs for uplink-sys crate scope:build Issues / PRs realted with the build process type:enhancement New feature or request

Comments

@ifraixedes
Copy link
Collaborator

The uplink-sys crate build script only works on Unix family operative systems.

Change the script to support Windows.

This is important because the uplink crate requires building the uplink-c crate, hence the uplink crate doesn't work in Windows either.

@ifraixedes ifraixedes added type:enhancement New feature or request scope:build Issues / PRs realted with the build process crate:uplink-sys Specific issues / PRs for uplink-sys crate labels Apr 11, 2023
@Wicpar
Copy link

Wicpar commented Apr 28, 2023

I attempted to do so in my fork, uplink-sys compiles but there are underlying issues with the bindgen or build...

error[E0063]: missing field `_address` in initializer of `UplinkCopyObjectOptions`
  --> C:\Users\_\IdeaProjects\uplink-rust\uplink\src\project\options.rs:44:9
   |
44 |         ulksys::UplinkCopyObjectOptions {}
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `_address`

error[E0063]: missing field `_address` in initializer of `UplinkMoveObjectOptions`
   --> C:\Users\_\IdeaProjects\uplink-rust\uplink\src\project\options.rs:335:9
    |
335 |         ulksys::UplinkMoveObjectOptions {}
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `_address`

error[E0063]: missing field `_address` in initializer of `UplinkUploadObjectMetadataOptions`
   --> C:\Users\_\IdeaProjects\uplink-rust\uplink\src\project\options.rs:371:9
    |
371 |         ulksys::UplinkUploadObjectMetadataOptions {}
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ missing `_address`

error: aborting due to 3 previous errors

For more information about this error, try `rustc --explain E0063`.
error: could not compile `uplink` (lib) due to 3 previous errors

I am uncertain how to fix them.

Edit: _address is a u8, no idea where they come from.

You can see the changes in my fork https://github.com/Wicpar/uplink-rust

@Wicpar
Copy link

Wicpar commented Apr 28, 2023

Related issue: rust-lang/rust-bindgen#1683

@Wicpar
Copy link

Wicpar commented Apr 29, 2023

I created a branch with precompiled binaries for windows. https://github.com/Wicpar/uplink-rust/tree/precompiled
The build script is also heavily altered. Adding targets for the precompiled libs should be as easy as adding a folder with the proper files and the proper triple. No weird macro cfg necessary.

The bindings were altered by hand to match the linux signatures, no idea if it will break things.

At least it compiles. I'll see if it works.

@ifraixedes
Copy link
Collaborator Author

@Wicpar thank you for all your effort in digging into it.

I cannot promise when I will be able to take a look at this, but I'll have this at the top of my list for the allotted time that we have to work on third-party projects.

@ifraixedes
Copy link
Collaborator Author

@Wicpar I finally got the chance to look at your changes.

The build script is also heavily altered. Adding targets for the precompiled libs should be as easy as adding a folder with the proper files and the proper triple. No weird macro cfg necessary.

We have a discussion about if the uplink-sys crate should download or even contain precompiled libs for some platforms on issue #19

I'm in favor of not requiring to compile the uplink-c and have the precompiled binaries in the crate. When I opened the issue I thought about downloading them, but I don't think that's a good idea because it requires access to the Internet and in some environments may not be allowed.

Before adding any precompiled binary we should resolve #19, I'm not the only decision maker here, I came to work on this later than others.

The bindings were altered by hand to match the linux signatures, no idea if it will break things.

It's unfortunate that issue rust-lang/rust-bindgen#1683 isn't still resolved 😞

So, I don't know if there is a better option than your one.

Your changes for changing the build.rs to work in any platform look great, the only thing that I'd like to do is not have to call the go build in the uplink-c and call the Makefile, but I cannot see that the current uplink-c Makefile compile to a Windows DLL, however, I'm quite ignorant of the nowadays Windows platforms.

I found an issue in the uplink-c (storj/uplink-c#15) that somebody was compiling in Windows and had issues with. I'd like that we fix that in the uplink-c rather than everybody that needs to compile it in Windows having to come their way.

Would you mind sending a PR with only the changes that transform the build.rs to platform independent?
With that, we could close this issue and open one for resolving the Windows issues and continue our discussion about adding precompiled libraries or not to the crate.

I want to say thanks again for your endeavors.

@Wicpar
Copy link

Wicpar commented May 10, 2023

The makefile is fundamentally incompatible with windows, not only because of the commands it uses, but also because it fails to find the go command on wsl even when on the path.

There are other issues with this crate like blocking functionality that requires heavy alterations to use in an async context safely, and the Move + Sync guarantees are not necessarily properly implemented. In addition it is unable to create presigned urls and thus is way less scalable than the s3 compatible API.

Due to all these factors I cannot choose to go with this library for high reliability production use cases, due to reliability and performance concerns, and cannot help you much further in improving the crate.

Even if i make the PR it doesn't solve the bingen and makefile issues which are crucial for this library to work and build properly.

In my opinion a pure async rust implementation would be the solution that is needed.

@ifraixedes
Copy link
Collaborator Author

Thank you for your feedback.

Regarding not being able to create presigned URLS, is it because isn't offered in the uplink-c or because uplink crate crashes o doesn't expose it?

In my opinion a pure async rust implementation would be the solution that is needed.

Do you mean to create natively a Rust crate and not rely on uplink-c?

@Wicpar
Copy link

Wicpar commented May 10, 2023

Regarding not being able to create presigned URLS, is it because isn't offered in the uplink-c or because uplink crate crashes o doesn't expose it?

I think there is no way to upload to the storj network directly from the frontend at the moment, which is necessary for my system to be truly scalable. Right now from what i understand the presigned urls go to storj hosted s3 compatibility servers which do accept the presigned upload. Ideally one could create a new encryption key per object on the frontend and just send that on the backend while uploading directly to the decentralized network. Maybe it truly is impossible if storj opted for RPC instead of http, so we would need browser RPC support.

Do you mean to create natively a Rust crate and not rely on uplink-c?

I mean a native rust crate, but it obviously is too much work to maintain multiple ecosystems for storj right now, maybe in the future when they are more successful :) I'll try to contribute to that by buying the service.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate:uplink-sys Specific issues / PRs for uplink-sys crate scope:build Issues / PRs realted with the build process type:enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants