Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix: bring back services ftp for python/java/node #5716

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yihong0618
Copy link
Contributor

Which issue does this PR close?

This path bring back services-ftp for java/node/python binding.

this feature comment in #3659
since issue #4090
and fixed in async-rs/async-tls#55 and #4091

I wonder they can be reopen again cc @messense

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Copy link
Contributor Author

And it might be a breaking change, if merge doc things is needed I think

@Xuanwo
Copy link
Member

Xuanwo commented Mar 9, 2025

I have initiated a discussion upstream: veeso/suppaftp#100

I'm a bit concerned about relying on async-tls these days, as it always brings an extra copy of rustls.

@Xuanwo
Copy link
Member

Xuanwo commented Mar 10, 2025

Hi, @yihong0618, the upstream fixed veeso/suppaftp#100, maybe you wanna try upgrade suppaftp and test if everything works as expected?

@yihong0618
Copy link
Contributor Author

Hi, @yihong0618, the upstream fixed veeso/suppaftp#100, maybe you wanna try upgrade suppaftp and test if everything works as expected?

will try this tonight

Signed-off-by: yihong0618 <zouzou0208@gmail.com>
@yihong0618
Copy link
Contributor Author

@Xuanwo

Chore the dep but the futures-rustls doc in README is wrong they did not chore it...

// Create a root certificate store
let root_store = RootCertStore::empty();
// Optionally, add certificates (e.g., system certs or custom ones)
// For now, we'll leave it empty as an example
Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing we need to add system credentials at the very least?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems no need follow the old default will change the comment

@messense
Copy link
Member

Note that pulling in aws-lc-rs which uses cmake may cause problem when cross compiling Python wheels.

@yihong0618
Copy link
Contributor Author

Note that pulling in aws-lc-rs which uses cmake may cause problem when cross compiling Python wheels.

will take a look

@yihong0618
Copy link
Contributor Author

@Xuanwo
Copy link
Member

Xuanwo commented Mar 10, 2025

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants