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

fix(electron/windows): build windows on linux #1818

Merged
merged 8 commits into from
Jan 19, 2024

Conversation

daniellacosse
Copy link
Contributor

No description provided.

@daniellacosse daniellacosse requested a review from a team as a code owner January 11, 2024 17:24
@daniellacosse daniellacosse requested a review from fortuna January 11, 2024 17:24
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e8dcd79) 32% compared to head (4c154b9) 32%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1818   +/-   ##
======================================
  Coverage      32%     32%           
======================================
  Files          45      45           
  Lines        2609    2609           
  Branches      337     337           
======================================
  Hits          859     859           
  Misses       1750    1750           
Flag Coverage Δ
apple 15% <ø> (ø)
ios ?
maccatalyst 15% <ø> (ø)
macos ?
unittests 32% <ø> (ø)
www 40% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -74,13 +74,10 @@ jobs:

windows_debug_build:
name: Windows Debug Build
runs-on: windows-2019
runs-on: ubuntu-20.04
timeout-minutes: 10
Copy link
Contributor

Choose a reason for hiding this comment

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

nit we might need to increase this timeout because we've experienced timeouts when building Windows in the past. it's OK to increase it in another PR.

@jyyi1
Copy link
Contributor

jyyi1 commented Jan 11, 2024

The job is failing:

⨯ wine is required, please see https://electron.build/multi-platform-build#linux  

@daniellacosse
Copy link
Contributor Author

daniellacosse commented Jan 12, 2024

The job is failing:

⨯ wine is required, please see https://electron.build/multi-platform-build#linux  

@fortuna this will require us to install wine every time, which seems like it could take awhile: https://wiki.winehq.org/Ubuntu

Here are some options on how to proceed. I could:
a) install wine
b) switch back to the node scripts I wrote that already fixes this problem
c) in the tun2socks/build.action.mjs, run go build if the os matches the target platform - otherwise call the makefile

I prefer C, but I don't know if you have any concerns I'm not aware of.

@daniellacosse
Copy link
Contributor Author

daniellacosse commented Jan 13, 2024

The job is failing:

⨯ wine is required, please see https://electron.build/multi-platform-build#linux  

@fortuna this will require us to install wine every time, which seems like it could take awhile: https://wiki.winehq.org/Ubuntu

I could: a) install wine b) switch back to the node scripts I wrote that already fixes this problem c) in the tun2socks/build.action.mjs, run go build if the os matches the target platform - otherwise call the makefile

I prefer C, but I don't know if you have any concerns I'm not aware of.

@fortuna this PR isn't working yet, need you to weigh in here on how to proceed. Are you okay with option C?

@fortuna
Copy link
Collaborator

fortuna commented Jan 16, 2024 via email

@daniellacosse
Copy link
Contributor Author

daniellacosse commented Jan 17, 2024

How do we currently built out for release? I don’t remember needing wine. And how would the node script help? I like the idea of the CI doing what e we do for release.

On Fri, Jan 12, 2024 at 8:54 PM Daniel LaCosse @.> wrote: The job is failing: ⨯ wine is required, please see https://electron.build/multi-platform-build#linux @fortuna https://github.com/fortuna this will require us to install wine every time, which seems like it could take awhile: https://wiki.winehq.org/Ubuntu I could: a) install wine b) switch back to the node scripts I wrote that already fixes this problem c) in the tun2socks/build.action.mjs, run go build if the os matches the target platform - otherwise call the makefile I prefer C, but I don't know if you have any concerns I'm not aware of. @fortuna https://github.com/fortuna this PR isn't working yet, need to to weigh in here on how to proceed. Are you okay with option C? — Reply to this email directly, view it on GitHub <#1818 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAA3XHPQQCBBPLFLRHGNDP3YOHSOTAVCNFSM6AAAAABBW3P772VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJQGI2DONJUG4 . You are receiving this because you were mentioned.Message ID: @.>

The issue is with cross-platform compilation. If we're going to use the Linux runner, we need to install Wine. If we're going to use the Windows runner, we can't use XGO. It's easiest to detect the platform the script is being run on in Node.

@fortuna
Copy link
Collaborator

fortuna commented Jan 17, 2024

This is how we do conditional build in the CI for the SDK:
https://github.com/Jigsaw-Code/outline-sdk/blob/bb5a7590955a5e82e897eb4cbde635cc99f7b270/.github/workflows/test.yml#L40

We can do the same here. My suggestion:

  • Skip building the client on Windows, or build as much as we can (Go and Web code). We don't really support it.
  • On Linux, install wine, or skip the part of the electron build that fails. Probably best to just install Wine, but it's up to you.
  • On macOS, build everything, since that's what we usually use for releases. As far as I understand, we don't need Wine on macOS.

@fortuna
Copy link
Collaborator

fortuna commented Jan 17, 2024

I guess my answer doesn't really address the fact that the Makefile doesn't let you do that.

Yes, option C makes sense

c) in the tun2socks/build.action.mjs, run go build if the os matches the target platform - otherwise call the makefile

@daniellacosse daniellacosse requested a review from a team as a code owner January 18, 2024 19:02
@daniellacosse daniellacosse requested a review from fortuna January 19, 2024 16:55
@daniellacosse daniellacosse merged commit 973aa55 into master Jan 19, 2024
17 checks passed
daniellacosse added a commit that referenced this pull request Jan 23, 2024
* 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
daniellacosse added a commit that referenced this pull request Jan 25, 2024
…y version (#1816)

* attempt #1

* unzip jni

* fix(electron/windows): build windows on linux (#1818)

* 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

* change gradle filepath

* remove jni
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants