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

Remove shared lib byte code files from source code & use GitHub Pipeline to build #91

Closed
wants to merge 24 commits into from

Conversation

KendallWeihe
Copy link
Contributor

@KendallWeihe KendallWeihe commented Jul 25, 2024

Moving this PR to a Draft because I'm not entirely sure we should move forward with everything -- @nitro-neal @ALRubinger @leordev I'm hoping this can be a collaborative space.

As what is currently in main, in order to publish a new Kotlin lib we must:

  1. Locally (yes, on our dev machines 🫢) run all four builds inside of bindings/tbdex_uniffi/libtargets/
  2. Commit the share lib byte code files into git
  3. Use a git tag, and JitPack will automatically pick it up

This is undesirable for several reasons:

  • Committing the byte code files to git bloats the git history
  • Building locally, as opposed to a pipeline environment, is no bueno -- laborious & improper
  • JitPack doesn't let us (easily) override the default Maven Artifact's groupId and artifactId

Note

With regards to that last bullet point, configuring the values in JitPack may be possible but would require a much more involved solution, which negates the value proposition of JitPack which is that it's simple, easy and "just works." JitPack automatically sets <groupId>com.github.TBD54566975</groupId> and <artifactId>tbdex-rs</artifactId> but that's undesirable, even as a temporary measure because it's "Rust." This is not a show stopper, but would be nice to fix.

And so, the original intent of this work was to alleviate those three undesirable bullet points.


Changes

  • Add .github/workflows/publish-kt.yml
  • Add byte code files to .gitignore
  • Add bound/kt/README.md to instruct local developers to first execute their binding
  • Edit bound/kt/pom.xml
    • Add GitHub Packages distribution (⚠️ ⚠️ ⚠️ @ALRubinger @leordev -- keep in mind, this PR is in a Draft state for good reason)
      • This doesn't actually fully work right now due to what I believe is a permissions issues we have set at the org level?
      • We're able to publish the package without issue
      • But then trying to consume it in an application fails with a 401
    • Remove fat jar (this was a relic from early integration testing)
  • Remove all 4 byte code files

Add .github/workflows/publish-kt.yml

This is where most of the action is, and so I would imagine even if we don't move forward with this PR then we may pull this Workflow code out of this branch and use it in future work.

Here is a visual of the workflow:

Screenshot 2024-07-25 at 2 27 55 PM

The sequence of events goes:

  1. Build all four byte code targets
  2. Package the Maven artifact
  3. Execute tests across all four targets (this ensures the shared libs built in (1) are successfully able to be loaded in the given target environment, but all from a single Maven artifact built in (2))
  4. Publish to GitHub Packages (in a recent commit I disabled this for anything other than main branch push events, but the last workflow run wherein this executed is here)

Proposal

@ALRubinger @leordev @nitro-neal I know that GitHub Packages is not our solution for Maven hosting, and so I see one of two ways forward:

A. We do not merge this, but we use a lot of the code changes here (copy/paste + massage into place) in what will be our long term solution.
B. We alleviate the 401 issue in the consumption of the GitHub Package Maven artifact and we move forward with this as a one-step-closer to where we want to be.

@KendallWeihe KendallWeihe changed the title Kendall/GitHub mvn repo Remove shared lib byte code files from source code & use GitHub Pipeline to build Jul 25, 2024
@KendallWeihe
Copy link
Contributor Author

I see the kt-test workflow is failing, I can work to fix that if we want to move forward with these changes

@nitro-neal
Copy link
Contributor

image this brings it home to me, basically exactly as @ALRubinger was discussing

@KendallWeihe
Copy link
Contributor Author

Closing in favor of #96

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

Successfully merging this pull request may close these issues.

2 participants