-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
chore(cordova/android): use src/tun2socks, not precompiled third_party version #1816
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1816 +/- ##
=======================================
+ Coverage 32% 40% +7%
=======================================
Files 45 39 -6
Lines 2609 1805 -804
Branches 337 337
=======================================
- Hits 859 738 -121
+ Misses 1750 1067 -683
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Nice!
Makefile
Outdated
@@ -20,6 +20,7 @@ android: $(BUILDDIR)/android/tun2socks.aar | |||
$(BUILDDIR)/android/tun2socks.aar: $(GOMOBILE) | |||
mkdir -p "$(BUILDDIR)/android" | |||
$(ANDROID_BUILD_CMD) -o "$@" $(IMPORT_PATH)/$(ROOT_PKG)/outline/tun2socks $(IMPORT_PATH)/$(ROOT_PKG)/outline/shadowsocks | |||
unzip "$@" 'jni/*' -d $(BUILDDIR)/android |
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 are you uncompressing the aar file?
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.
Sorry, still exploring. Let me set this to draft.
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.
It's required for the copy script, for some reason, and removing the copy script silently breaks the build
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.
Maybe not in this PR, but I think we should use the aar directly in the future. Intra already referenced the aar directly, but I don't know whether Cordova allows you to do that.
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.
It feels like a Cordova thing, yeah.
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.
@daniellacosse Can you check whether we need to unzip again?
Based on the new fixed build.gradle, it seems we refer to directly to the .aar file, not the extracted files.
* fix(electron/windows): build windows on linux * use go build when the target and host platforms match [WIP] * oops * scope to electron * revert windows job changes * lol ai * fix path * resolve current platform
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.
Looks good, but let's check the zip thing again.
Makefile
Outdated
@@ -20,6 +20,7 @@ android: $(BUILDDIR)/android/tun2socks.aar | |||
$(BUILDDIR)/android/tun2socks.aar: $(GOMOBILE) | |||
mkdir -p "$(BUILDDIR)/android" | |||
$(ANDROID_BUILD_CMD) -o "$@" $(IMPORT_PATH)/$(ROOT_PKG)/outline/tun2socks $(IMPORT_PATH)/$(ROOT_PKG)/outline/shadowsocks | |||
unzip "$@" 'jni/*' -d $(BUILDDIR)/android |
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.
@daniellacosse Can you check whether we need to unzip again?
Based on the new fixed build.gradle, it seems we refer to directly to the .aar file, not the extracted files.
I tried that and it didn't work. Cordova fails silently. The jni folder
gets copied to an obj folder and I guess Cordova needs that? I don't really
care to dig into it, but what further evidence do you require?
…On Wed, Jan 24, 2024, 11:20 AM Vinicius Fortuna ***@***.***> wrote:
***@***.**** commented on this pull request.
Looks good, but let's check the zip thing again.
------------------------------
In Makefile
<#1816 (comment)>
:
> @@ -20,6 +20,7 @@ android: $(BUILDDIR)/android/tun2socks.aar
$(BUILDDIR)/android/tun2socks.aar: $(GOMOBILE)
mkdir -p "$(BUILDDIR)/android"
$(ANDROID_BUILD_CMD) -o "$@" $(IMPORT_PATH)/$(ROOT_PKG)/outline/tun2socks $(IMPORT_PATH)/$(ROOT_PKG)/outline/shadowsocks
+ unzip "$@" 'jni/*' -d $(BUILDDIR)/android
@daniellacosse <https://github.com/daniellacosse> Can you check whether
we need to unzip again?
Based on the new fixed build.gradle, it seems we refer to directly to the
.aar file, not the extracted files.
—
Reply to this email directly, view it on GitHub
<#1816 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4V5VBUQYQR6G3OTME3Q23YQEYEHAVCNFSM6AAAAABBVPV326VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTQNBRHAYDIMRXHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
No description provided.