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

refactor: Unzip Android deps*.zip #5907

Closed

Conversation

randombk
Copy link
Contributor

@randombk randombk commented Jan 9, 2025

Checklist

Required

Purpose of change

Pre-compressing the android dependencies into a zip is redundant as Git gzips blobs anyways. Squishing everything into deps.zip just means dependencies can't be modified without uploading a new binary blob of all the dependencies.

This is relevant for #5777 , as that PR introduces new dependencies (sqlite) for the first time in ~3 years.

Describe the solution

This commit removes the deps.zip setup and leaves all the dependencies as loose files in the repo.

NOTE: This does remove the '-Pdeps' functionality which makes it easier to swap out dependencies with debug binaries, but I'm doubtful it's worth the cost and if anyone is actively using that. Holler at me if this is an issue.

Describe alternatives you've considered

We could leave this setup as-is and just package new dependencies into deps.zip and deps_debug.zip, but (1) I'm not a fan of mysterious zip files with no traceable provenance, and (2) that just kicks the issue down the road if we ever want to update or add more dependencies in the future.

Testing

CI compile test. Once it builds we'll see if it runs.

Additional context

  Pre-compressing the android dependencies into a zip
  is redundant as Git gzips blobs anyways. Squishing
  everything into deps.zip just means dependencies can't
  be modified without uploading a binary blob of *all* the
  dependencies.

  This commit removes the deps.zip setup and leaves all the
  dependencies as loose files in the repo.

  NOTE: This does remove the '-Pdeps' functionality, but
  I'm doubtful it's worth the cost and if anyone is actively
  using that. Holler at me if this is an issue.

Signed-off-by: David Li <[email protected]>
@github-actions github-actions bot added the docs PRs releated to docs page label Jan 9, 2025
@randombk
Copy link
Contributor Author

randombk commented Jan 9, 2025

Compiled and ran successfully: https://github.com/randombk/Cataclysm-BN/releases/tag/2025-01-08

@scarf005
Copy link
Member

while i'm not against merging this PR, still bit worried on vendoring source code as it increases already heavy repo history. maybe would it be possible to use cmake/gradle/other means to download these dependencies?

@randombk
Copy link
Contributor Author

I don't think there's any urgency in landing this now. Adding sqlite will need a slightly different method regardless of this because of some weird issues with using the precompiled binaries.

@randombk
Copy link
Contributor Author

This is no longer needed by #5777 now. We don't need to make a decision on this now, so we could stick to the status quo and discard this PR.

@scarf005 scarf005 added this to the Stable v0.8.0 milestone Jan 19, 2025
@scarf005
Copy link
Member

This is no longer needed by #5777 now. We don't need to make a decision on this now, so we could stick to the status quo and discard this PR.

closing for now.

@scarf005 scarf005 closed this Jan 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs PRs releated to docs page
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants