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

chore(client/cordova/apple): introduce XCode subproject #1787

Merged
merged 12 commits into from
Jul 23, 2024
Merged

Conversation

fortuna
Copy link
Collaborator

@fortuna fortuna commented Nov 21, 2023

This PR introduces a subproject (OutlineLib) to the main Cordova project, and moves the VPN Extension to it.

This has some advantages:

  • Allows us to define bundles outside the Cordova project, which is not possible in Swift Packages. This reduces the need to edit the main project.
  • Allows us to move away from the Swift Package

Notice that the new project still depends on the Swift Package. The goal is to eventually move all the code to the OutlineLib subproject, so we can get rid of the Swift package.

I'm hoping that after we finish this, the Cordova project generated by iOS can be minimally edited to just add the subproject dependency and set the dependencies of the main app target, which could perhaps be done automatically.

image

@fortuna fortuna requested a review from sbruens November 21, 2023 22:56
@fortuna fortuna requested a review from a team as a code owner November 21, 2023 22:56
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f38b935) 32% compared to head (b76c86d) 32%.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #1787   +/-   ##
======================================
  Coverage      32%     32%           
======================================
  Files          45      45           
  Lines        2609    2609           
  Branches      337     337           
======================================
  Hits          859     859           
  Misses       1750    1750           
Flag Coverage Δ
apple 15% <ø> (ø)
ios 15% <ø> (ø)
maccatalyst 15% <ø> (ø)
macos 15% <ø> (ø)
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.

@fortuna fortuna changed the title chore(cordova/ios): Introduce XCode subproject chore(cordova/apple): introduce XCode subproject Nov 21, 2023
@@ -0,0 +1,29 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't need version information injected on bulid, right?

@fortuna fortuna changed the title chore(cordova/apple): introduce XCode subproject chore(client/cordova/apple): introduce XCode subproject Jul 22, 2024
@fortuna
Copy link
Collaborator Author

fortuna commented Jul 22, 2024

@sbruens this is ready for review.

It was a pain to get it working again, but it will allow us to move quicker, because with the side project we don't need to deal with the copy into the iOS and macOS projects that get rsynced.

Next step would be to start moving code out of the Swift Package into the Project directly, and eventually drop the Swift package. There's a circular dependency with the PacketTunnelProvider and OutlineTunnel that needs to be addressed though.

The OutlineLauncher should also move into the new project, since we can define bundles easily now.

@github-actions github-actions bot added size/XXL and removed size/XL labels Jul 22, 2024
@@ -47,18 +48,6 @@ let package = Package(
"OutlineNotification",
]
),
.target(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It turns out no other target depends on Tun2socks, so after removing the unused references, I was able to move PacketTunnelProvider into the side project and out of the Swift Package!

/* Begin PBXFileReference section */
522987032C4F273E009EE577 /* PacketTunnelProvider.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = PacketTunnelProvider.h; sourceTree = "<group>"; };
522987042C4F273E009EE577 /* PacketTunnelProvider.m */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.objc; path = PacketTunnelProvider.m; sourceTree = "<group>"; };
5229870D2C4F2873009EE577 /* Tun2socks.xcframework */ = {isa = PBXFileReference; lastKnownFileType = wrapper.xcframework; name = Tun2socks.xcframework; path = ../../../../output/build/apple/Tun2socks.xcframework; sourceTree = "<group>"; };
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is currently a reference to the pre-built binary, but I think we can potentially build this on demand by messing with the project build.

@@ -167,6 +167,8 @@ It's safe to unregister all the Outline VPN Extensions, since the system will lo
pluginkit -r $APP_EXTENSION_PATH
```

Make sure that you list the registered plugins again after unregistering them, since they may fallback to other versions of it, which you may also need to unregister.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New finding on debugging. We need to keep unregistering and checking until they are all gone.

@@ -10,21 +10,22 @@ let package = Package(
// CocoaLumberjack 3.8.0 dropped support for iOS < 11 and macOS < 10.13.
// See https://github.com/CocoaLumberjack/CocoaLumberjack/releases/tag/3.8.0.
// These cannot be upgraded without also upgrading the entire project.
.iOS(.v11),
.iOS(.v13),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a cleanup from #2057?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The code from the previous PR is not broken, but we will need to update this anyway before we start using new code in the swift package.

@fortuna fortuna enabled auto-merge (squash) July 23, 2024 16:57
@fortuna fortuna merged commit dff8f19 into master Jul 23, 2024
16 checks passed
@fortuna fortuna deleted the fortuna-ios branch July 23, 2024 16:59
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