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

Provide pre-built binaries for SPM (compiling current package takes >20 seconds) #1369

Closed
arielelkin opened this issue Oct 11, 2021 · 17 comments

Comments

@arielelkin
Copy link

The current Swift Package involves building the entire Sentry SDK from source, which takes 10-30 seconds:

image

Could we have a swift package that just provides the the Sentry binary .frameworks. Implementation is really straightforward:
https://developer.apple.com/documentation/swift_packages/distributing_binary_frameworks_as_swift_packages

A broader discussion is whether Sentry's default swift package should contain source code or the pre-built framework. I presume most people using Sentry are just integrating it as a third-party framework (rather than developing it) and faster compilation times are more important to them than having access to the source code. Have you considered providing two separate Swift packages; one for the source and one for the pre-built binary?

@arielelkin arielelkin changed the title Provide pre-built binaries for SPM Provide pre-built binaries for SPM - compiling current package takes >20 seconds Oct 11, 2021
@arielelkin arielelkin changed the title Provide pre-built binaries for SPM - compiling current package takes >20 seconds Provide pre-built binaries for SPM (compiling current package takes >20 seconds) Oct 11, 2021
@philipphofmann
Copy link
Member

Thanks, @arielelkin, for pointing that out. I agree that this should be a relatively easy task. We are happy to accept PRs if you are motivated :)

@arielelkin
Copy link
Author

I could take a quick look...

For this to work, the zip file you make available in Releases must have the xcframework in its root directory (currently it's in Carthage/Build/Sentry.xcframework).

Could you please fix that?

@philipphofmann
Copy link
Member

Thanks, for the info. I guess it's better if I do it then because getting all the paths right without breaking something might be a bit hard. Thanks for your help.

@arielelkin
Copy link
Author

@philipphofmann hello! I'd like to follow up on this.

I believe this is important. Building Sentry from source as is currently the case makes sense if most developers were interested in inspecting or debugging the Firebase code, but I suspect that's a tiny fraction of us. Feels like the rest of us waste a lot of computing time by constantly building from source instead of using precompiled binaries.

What's the value in building Sentry from source instead of using precompiled binaries?

I'd be happy to send a PR addressing this. As I mentioned, the zip file you make available in Releases must have the xcframework in its root directory (currently it's in Carthage/Build/Sentry.xcframework).

Could you please fix that?

@philipphofmann
Copy link
Member

@arielelkin, sorry we didn't get to this yet. If you could open a PR, it would be awesome 😃 . I think it should be an easy fix.

@arielelkin
Copy link
Author

@philipphofmann The binary asset's url specified in Package.swift needs to point to a release asset and a tag, but that creates a chicken and egg problem.

One solution is the following:
https://forums.swift.org/t/spm-support-basic-auth-for-non-git-binary-dependency-hosts/37878/103

Point the Package.swift to the draft release assets
Push the updated Package.swift + tag
Update the draft release to use the new tag
Move the release from draft to published

Alternatively, we could have a separate repo hosting the release assets.. Easier to implement, but has some redundancy.

What are your thoughts?

@philipphofmann
Copy link
Member

Sorry, @arielelkin will get back to you tomorrow.

@philipphofmann
Copy link
Member

Alternatively, we could have a separate repo hosting the release assets. Easier to implement, but has some redundancy.

I would like to avoid a repo for hosting, as we need to set it up, maintain, etc.

@arielelkin, we know the binary URL before creating the release. See https://github.com/getsentry/sentry-cocoa/releases/tag/8.3.1 has https://github.com/getsentry/sentry-cocoa/releases/download/8.3.1/Sentry.xcframework.zip.

Another question: can't we use the binarytarget(name:path:)?

@arielelkin
Copy link
Author

we know the binary URL before creating the release. See https://github.com/getsentry/sentry-cocoa/releases/tag/8.3.1 has https://github.com/getsentry/sentry-cocoa/releases/download/8.3.1/Sentry.xcframework.zip.

Unfortunately that doesn't help; the issue is knowing the binary URL before creating the tag, not the release:

  • Package.swift lives at a specific tag and must specify the binary file's URL
  • We know that URL only once the file has been added to a GitHub release as an asset
  • That GitHub release is only created after we push a tagged commit, in which Package.swift file specifies the... file's URL, which doesn't exist at that stage as the release hasn't been created yet
  • 🐔 🥚

Another question: can't we use the binarytarget(name:path:)?

For that to work, the .xcframework must be checked into the repo.

@philipphofmann
Copy link
Member

philipphofmann commented Mar 21, 2023

We know the URL beforehand, as it follows the pattern https://github.com/getsentry/sentry-cocoa/releases/download/sdk-version/Sentry.xcframework.zip , but the problem is the checksum. It doesn't matter if we host our own repo for the framework.zip or if we use the artifacts of GH releases. We need to make a commit with the checksum of the framework.zip after GH actions created it.

Currently, our release process pulls the artifact from the commit created by crafts preReleaseCommand, such as 8.3.2. After a Sentry manager approves the release, such as 8.3.2, the publish GH action workflow would need to pull the artifact and create another commit to update the checksum of the SPM .binaryTarget. As far as I know, this is not possible now with our process of managing release. Craft would need a similar hook as the pre-release-command, which runs during craft publish. The hook would need to run before publishing the targets.

We understand that there is a valid use case for this improvement, but it's not a low-hanging fruit, and therefore, we can't give you an ETA for it.

@tcwalther
Copy link

Another option is to create a second repository for this, like a mirror. I've found https://github.com/malcommac/sentry-cocoa-sdk-xcframeworks/ that does exactly that, but trusting a binary framework from an unofficial source is always a bit "meh" (plus, rn their workflow script is a bit broken and doesn't actually store the framework in the root folder - I created an issue for this, but it shows it'd be better to have an official channel).

@philipphofmann
Copy link
Member

philipphofmann commented Mar 23, 2023

Thanks for the update, @tcwalther, but I prefer using the same repository. For everyone, please upvote in the first comment in this issue #1369 (comment) to let us know how many users would love to see this being implemented.

@scogeo
Copy link

scogeo commented Mar 24, 2023

I'm chiming in for a +1 to have a separate repository to host the Package.swift containing binaryTargets and keep the current repository as a build from source(s) package. This gives the most flexibility for building the library from source if needed and gives an option to access the officially sanctioned pre-built library at a different URL. For example, if there was a new repo named sentry-cocoa-xcframework, then one could just change the URL in the package reference.

// Use for source version
.package(url: "https://github.com/getsentry/sentry-cocoa", from: "8.3.2"),
// Or, use the xcframework version
.package(url: "https://github.com/getsentry/sentry-cocoa-xcframework", from: "8.3.2"),

The repo that hosts the xcframework doesn't have to actually host or build the xcframework, it could just point at URLs back in the main sentry-coca repository releases. Then just add a workflow in GitHub that updates the xcframework repo whenever a release is published in the main repo.

It would just need a Package.swift file similar to the following:

let package = Package(
    name: "Sentry",
    platforms: [.iOS(.v11), .macOS(.v10_13), .tvOS(.v11), .watchOS(.v4)],
    products: [
        .library(name: "Sentry", targets: ["Sentry"]),
    ],
    dependencies: [],
    targets: [
        .binaryTarget(name: "Sentry",
                      url: "https://github.com/getsentry/sentry-cocoa/releases/download/8.3.2/Sentry.xcframework.zip",
                      checksum: "e25814a875efe595b24e0a5fddd519b899aab129ab8e257673be90ae85fb4bc8")
    ]
)

Each release just needs to update the URL and the checksum. Then just update the Carthage build so that the .zip file has the xcframework at the root and it will be compatible with both SPM and Carthage.

@arielelkin
Copy link
Author

In the meantime, here's how to easily host your sentry xcframework binaries:

  1. create a new folder
  2. download the latest sentry xcframework from https://github.com/getsentry/sentry-cocoa/releases, unzip and place it at the root of the directory
  3. create the following Package.swift file
  4. commit and push
  5. in your xcode project's swift package section, replace the sentry package with this
  6. reduce your build time substantially
// swift-tools-version:5.5
import PackageDescription

let package = Package(
    name: "Sentry-binaries",
    platforms: [.iOS(.v11), .macOS(.v10_13), .tvOS(.v11), .watchOS(.v4)],
    products: [
        .library(
            name: "Sentry-binaries",
            targets: ["Sentry"]
        )
    ],
    targets: [
        .binaryTarget(
            name: "Sentry",
            path: "Sentry.xcframework"
        )
    ]
)

@philipphofmann philipphofmann moved this from Backlog to Needs Discussion in Mobile & Cross Platform SDK Mar 29, 2023
@lionel-alves
Copy link

Hi, any update on this? Thanks!

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 2 Jan 26, 2024
@philipphofmann
Copy link
Member

@lionel-alves, no update, sorry.

@brustolin
Copy link
Contributor

brustolin commented Mar 5, 2024

We added pre-built binaries in this PR and will be available with the next release.

@github-project-automation github-project-automation bot moved this from In Progress to Done in Mobile & Cross Platform SDK Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Archived in project
Development

No branches or pull requests

8 participants