chore(cordova): build and depend on plugin in build/ #1521

fortuna · 2022-12-16T23:28:45Z

By adding a first stage to build the plugin and depend on it in the build directory, it will be a lot easier to make the plugin standalone.

This also paves the way to create the Cordova project in build/, which will allow us to do smarter things, like having different config.xml per platform, inject version numbers, but also get rid of the www/, platforms/ plugin/ in the root, and simplify clean.

fortuna · 2022-12-16T23:44:47Z

Damm, I need to figure out how to have the plugin in the package.json, but not be installed....
And I need to be able to build only one platform plugin, so I can build Android on Linux.
Ugh, this is hard... I need to put different platforms in different directories and build on demand.
I can't really build a plugin for all cordova platforms.

daniellacosse · 2022-12-19T15:40:54Z

Damm, I need to figure out how to have the plugin in the package.json, but not be installed.... And I need to be able to build only one platform plugin, so I can build Android on Linux. Ugh, this is hard... I need to put different platforms in different directories and build on demand. I can't really build a plugin for all cordova platforms.

This strengthens the case for a per-platform approach no? Think of a structure like this:

/cordova
  /ios
    /plugin
    build.action.mjs # => when called, calls cordova/plugin/build ios, populating cordova/ios/plugin
  /macos
     /plugin
     build.action.mjs # => when called, calls cordova/plugin/build macos, populating cordova/macos/plugin
  /android
     /plugin
     build.action.mjs # => when called, calls cordova/plugin/build android, populating cordova/android/plugin
  /plugin
    build.action.mjs

fortuna · 2022-12-19T15:57:03Z

Yes, this PR needs to change to build the plugin for each platform.

I was thinking of a structure like this inside build/:

cordova/
- app/
  - ${clientOs}-{buildMode}/ (e.g. ios-release, android-debug)
- plugin/
  - ${clientOs}/

By appending the buildmode to the directory, we can have the builds coexist and not interfere with each other. It also prevents the release of debug code. It also makes it clear that that component takes the build mode into account.

daniellacosse · 2022-12-19T17:50:15Z

Yes, this PR needs to change to build the plugin for each platform.

I was thinking of a structure like this inside build/:

cordova/

app/

${clientOs}-{buildMode}/ (e.g. ios-release, android-debug)

plugin/

${clientOs}/

By appending the buildmode to the directory, we can have the builds coexist and not interfere with each other. It also prevents the release of debug code. It also makes it clear that that component takes the build mode into account.

Here's my ideal: each platform project has a .gitignore'd build folder. That way the artifacts live right next to the code that builds them. You can only "build" a production binary. Anything "debug" just launches in a live mode that is ephemeral (opens the simulator and then tears it down when you're done working).

I think this is an okay intermediate step, seeing as how the current build is centralized in the root and we'd need improvements to our build infrastructure to realize this vision.

src/cordova/get_cordova_build_parameters.mjs

jyyi1 · 2022-12-19T23:15:19Z

src/cordova/build_plugin.action.mjs

+
+import {cpSync, mkdirSync} from 'fs';
+import path from 'path';
+import rmfr from 'rmfr';


discussion it seems we have references to packages with duplicated functionalities, like rmfr here is duplicated with rimraf.

I'm justing reusing what was already being used somewhere else. I think rimraf is a command line tool?

rimraf is both a node library and a cli implementing rm -rf. I think it's ok for this pr, we may clean them up in the future.

i believe rimraf doesn't support async, rmrf doesn't have a cli

to clean it up, we'd need an async wrapper for rimraf

src/cordova/build_plugin.action.mjs

jyyi1

LGTM, thanks!

daniellacosse

Looking good, just some ideas/comments

daniellacosse · 2022-12-20T15:08:38Z

package.json

@@ -152,6 +152,6 @@
    ]
  },
  "workspaces": [


I guess we can remove the workspace for now, it's not persistent anymore

daniellacosse · 2022-12-20T15:11:39Z

src/cordova/build_plugin.action.mjs

+  await rmfr(PLUGIN_OUTPUT);
+  await mkdir(PLUGIN_OUTPUT, {recursive: true});
+
+  await cp(path.join(process.env.ROOT_DIR, 'src', 'cordova', 'plugin'), PLUGIN_OUTPUT, {recursive: true});


can we prefer to use getRootDir? Just for the sake of consistency. We can do something similar with buildDir later (i would prefer it to be a parameter)

daniellacosse · 2022-12-20T15:14:32Z

src/cordova/build_plugin.action.mjs

+    const ANDROID_LIB_DIR = path.join(PLUGIN_OUTPUT, 'android', 'libs');
+    await mkdir(ANDROID_LIB_DIR, {recursive: true});
+    await cp(
+      path.join(process.env.ROOT_DIR, 'third_party', 'outline-go-tun2socks', 'android', 'tun2socks.aar'),


nit - how would you feel about having a "apple dependencies" array and a "android dependencies" array and then iterating over those here?

daniellacosse · 2022-12-20T15:15:53Z

src/cordova/build_plugin.action.mjs

@@ -0,0 +1,96 @@
+// Copyright 2022 The Outline Authors


how would you feel about this living in src/cordova/plugin?

(so it would be src/cordova/plugin/build)

this would require moving the plugin source into a subfolder in cordova/plugin

fortuna · 2022-12-28T21:54:08Z

For some reason this is succeeding on the CI, but was failing from XCode for macOS.

I'll wait for #1527 to be in, before making this one work for real. I don't want to block that one, which is higher impact.

fortuna added 2 commits December 16, 2022 16:51

Build xcframeworks in the build directory

ab342f6

Build and depend on plugin in build/

6e07c31

fortuna requested review from a team as code owners December 16, 2022 23:28

github-actions bot added the size/M label Dec 16, 2022

fortuna changed the title ~~Build and depend on plugin in build/~~ chore(cordova): build and depend on plugin in build/ Dec 16, 2022

fortuna marked this pull request as draft December 16, 2022 23:44

fortuna changed the title ~~chore(cordova): build and depend on plugin in build/~~ [WIP] chore(cordova): build and depend on plugin in build/ Dec 16, 2022

Only build what's needed for the platform

1971932

fortuna changed the title ~~[WIP] chore(cordova): build and depend on plugin in build/~~ chore(cordova): build and depend on plugin in build/ Dec 19, 2022

fortuna added 2 commits December 19, 2022 17:13

Restore most of package-lock.json

78a6033

Fix packages

93219da

fortuna marked this pull request as ready for review December 19, 2022 22:30

fortuna requested review from daniellacosse and jyyi1 December 19, 2022 22:30

jyyi1 reviewed Dec 19, 2022

View reviewed changes

fortuna added 3 commits December 19, 2022 18:22

Remove logging

b319428

Clarify platform

caf7825

Use fs promises

6bcf36d

jyyi1 approved these changes Dec 20, 2022

View reviewed changes

daniellacosse reviewed Dec 20, 2022

View reviewed changes

fortuna marked this pull request as draft December 28, 2022 21:56

Merge branch 'master' into fortuna-move

f41fa71

github-actions bot added size/S and removed size/M labels Dec 28, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(cordova): build and depend on plugin in build/ #1521

chore(cordova): build and depend on plugin in build/ #1521

fortuna commented Dec 16, 2022

fortuna commented Dec 16, 2022

daniellacosse commented Dec 19, 2022

fortuna commented Dec 19, 2022 •

edited

Loading

daniellacosse commented Dec 19, 2022 •

edited

Loading

jyyi1 Dec 19, 2022

fortuna Dec 19, 2022

jyyi1 Dec 20, 2022

daniellacosse Dec 20, 2022 •

edited

Loading

jyyi1 left a comment

daniellacosse left a comment

daniellacosse Dec 20, 2022

daniellacosse Dec 20, 2022

daniellacosse Dec 20, 2022

daniellacosse Dec 20, 2022

daniellacosse Dec 20, 2022 •

edited

Loading

daniellacosse Dec 20, 2022

fortuna commented Dec 28, 2022

chore(cordova): build and depend on plugin in build/ #1521

Are you sure you want to change the base?

chore(cordova): build and depend on plugin in build/ #1521

Conversation

fortuna commented Dec 16, 2022

fortuna commented Dec 16, 2022

daniellacosse commented Dec 19, 2022

fortuna commented Dec 19, 2022 • edited Loading

daniellacosse commented Dec 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniellacosse Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

jyyi1 left a comment

Choose a reason for hiding this comment

daniellacosse left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniellacosse Dec 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fortuna commented Dec 28, 2022

fortuna commented Dec 19, 2022 •

edited

Loading

daniellacosse commented Dec 19, 2022 •

edited

Loading

daniellacosse Dec 20, 2022 •

edited

Loading

daniellacosse Dec 20, 2022 •

edited

Loading