-
-
Notifications
You must be signed in to change notification settings - Fork 11.6k
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
Integrate shadowsocks-rust #2454
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
The naming issue should be fixed via a51ad37. |
I've finished the Further development:
|
Easy peasy. I will try to do that in this weekend. |
@madeye In my proposal, I was saying that there is no need for a reverse DNS lookup -- this can be easily done using a reverse lookup map in |
@Mygod Okay, if so, we need to implement the local resolver in sslocal. Can you help on this? I think it can be implemented based on trust-dns. The good news is shadowsocks-rust has already used it for safe DNS resolving. What we need is to extend it to a ChinaDNS like local resolver. |
I could if y'all can wait for me to pick up rust first. :) |
Oh I was thinking of putting the code at |
It seems that adding a flow stat for I am going to merge implementation of manager stat report in |
No hurry... In shadowsocks-libev, I have a counter in the context and a timer to push the counter value to shadowsocks-android. |
Because |
Well it looks like this repo is going to take more than 4GB of RAM to build. Let me try again tomorrow. :) |
Consider disable LTO. It takes quite a lot of memory while linking. But output binary would be larger. |
build.gradle
Outdated
@@ -14,6 +14,7 @@ buildscript { | |||
junitVersion = '4.13' | |||
androidTestVersion = '1.2.0' | |||
androidEspressoVersion = '3.2.0' | |||
ndkVersion = '21.0.6113669' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to specify ndkVersion btw?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the warning message that ANDROID_NDK_HOME
is deprecated now. Any suggestion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we are using that env?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, I cannot get the project built with Android Studio 4.0...
core/build.gradle
Outdated
noDefaultBut "sodium", "android", "rc4", "aes-cfb", "aes-ctr", "camellia-cfb", "openssl-vendored" | ||
} | ||
exec { spec, toolchain -> | ||
spec.environment("RUSTFLAGS", "-C link-arg=-o -C link-arg=target/${toolchain.target}/${profile}/libsslocal.so") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would think that the plugin would take care of the output so name... Is this an upstream issue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's actually a limitation of Cargo, which cannot customize the output binary name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Their sample app is far less involved than this. I would assume it's upstream bug (I wonder if anyone is actually using the plugin).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got the workaround here: rust-lang/cargo#1706 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think they already have a workaround: https://github.com/mozilla/rust-android-gradle/blob/3dc5511b2bbed4b66d03e360302b8147d7138877/plugin/src/main/kotlin/com/nishtahir/CargoBuildTask.kt#L193
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah as I figured nobody is actually maintaining this plugin.
deb5de4
to
1bd347a
Compare
I think the submodule is not correctly moved in this branch... |
The latest commit should fix the issue. |
Yeah sorry for force pushing lol. @madeye |
Sadly it seems like I don't have time to understand the rust part for now, so instead I just added a JVM part for |
P.S. It is only enabled for There is a possibly more efficient implementation -- we can let |
Currently, there's no DNS queries from shadowsocks-rust, as the SOCKS5 requests from tun2socks are all IPv4 or IPv6 types, no domain name. That's also why ACL (domain name) doesn't work now. With the |
@madeye No... I am thinking that |
I would like to propose the following to-do list (based on the one in OP), with priorities.
Lower priority:
Lowest priority: (these are usually new features, can implement these in the future)
Thanks everyone again for the hard work! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't reviewed the rust part yet (probably next thing to do for me) but Android part looks good to me, besides the following comments:
- This is related to multiple upstreams but we should implement UDP fallback support?
- We should remove hosts setting or add hosts support?
- We should remove TFO setting or TFO support?
- Build speed is unreasonably slow. (use debug profile for debugging and CI builds? enable one target when building debug build? if only the gradle plugin is still being maintained)
For future:
- Implement async I/O for LocalSocket in JNI?
- Other "lowest priority" tasks mentioned in Integrate shadowsocks-rust #2454 (comment)
url = https://github.com/google/re2.git | ||
[submodule "core/src/main/rust/shadowsocks-rust"] | ||
path = core/src/main/rust/shadowsocks-rust | ||
url = https://github.com/madeye/shadowsocks-rust |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be changed to shadowsocks/shadowsocks-rust
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shadowsocks/shadowsocks-rust#211 is not merged yet.
After that, we should change the submodule to upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned I think that PR doesn't seem too important for this PR right now. 🤣
A future plan maybe.
Ditto. For now, I think we can remove this feature.
I suggest remove TFO as well, which is kind of broken in real usage.
Yes, I think we should do that. Previously, I tried to use rust debug target in debug build, but failed. It's not a bad idea to do some cleanup now. IMO, many features like HOSTS and TFO are not quite useful. |
I agree except for UDP fallback because this is the only way to get UDP support for plugins. (UDP is useful for VoIP, gaming, etc and plugin is useful for apparent reasons). Maybe it's best to resolve this before merging it? |
Okay, let's assume no multi-upstream for now. So, the UDP fallback logic should be the same as before. |
Addressed UDP fallback via a252f19 for now (not tested yet) but the real fix should probably be implemented via some kind of multi-upstream, as now we are putting DNS relay inside sslocal. What's left to do is probably to address build speed and binary size... (22 minutes of CI build is really slow) |
Now the CI duration dropped to 10m 8s. https://app.circleci.com/pipelines/github/shadowsocks/shadowsocks-android/8/workflows/8ad4e812-60c7-4aa5-8423-e2d944ac0b20/jobs/526 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-u
does not seem to be working in shadowsocks-rust, other than that, LGTM.
(I guess this would be the best binary size we could get, good enough for me)
P.S. Once we fix UDP fallback, we can merge this and remove TFO and hosts in separate PRs.
Oh should probably update license too due to gitmodules changes... |
UDP relay should work now. |
Tested locally, fallback UDP relay works now. |
I think this PR is ready for merge. Thanks for all the contributions! |
This PR aims to replace shadowsocks-libev with shadowsocks-rust.
Fix issue #2452.
Nice to have:
Future development plan (maybe not covered by this PR)