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

Rebase unity bro exe onto master #3

Open
wants to merge 13 commits into
base: unity_bro_exe_replacement
Choose a base branch
from

Conversation

rachelmsimm
Copy link
Collaborator

@rachelmsimm rachelmsimm commented Jun 13, 2023

Upon approval and being merged, this branch should replace unity_bro_exe

This PR is for taking our commits from the unity_bro_exe branch and applying them to the updated version of brotli (our master branch is already in sync with the updated version of brotli, and purposely excludes these Unity specific commits). The branch unity_bro_exe is the one that we reference in the editor as the brotli executable.

I did the following to test the changes:

  1. Update unity_bro_exe to my branch name in: https://github.com/Unity-Technologies/emscripten-builder/blob/main/build_brotli.py
  2. Build brotli: https://unity-ci.cds.internal.unity3d.com/project/949?nav=jobDefinitions
  3. Manually copy the zip here: https://github.cds.internal.unity3d.com/unity/unity/tree/trunk/External/Compression/Brotli/Brotli
  4. Create a new trunk build with the change and test out building and playing a game with brotli compression to make sure things look as expected

I only tested this on Mac so far, I plan to test on Windows but will reach out to QA for help testing Linux. Decompression fallback should also be tested.
These are the brotli builds I generated with my changes (step 2 above):

Copy link
Collaborator

@juj juj left a comment

Choose a reason for hiding this comment

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

LGTM

@juj
Copy link
Collaborator

juj commented Jun 14, 2023

Before overwriting the old branch, it would be good to test the new Brotli against Unity web builds with brotli compression enabled.

@rachelmsimm
Copy link
Collaborator Author

Before overwriting the old branch, it would be good to test the new Brotli against Unity web builds with brotli compression enabled.

@juj Is there a difference between what you're saying here and the testing method I outlined in the PR description?

@juj
Copy link
Collaborator

juj commented Jun 15, 2023

Ah right, no the description in the PR comment reads great. 👍

Initialize comment as it was throwing a seg fault since it was uninitialized
Copy link
Collaborator

@brendan-duncan brendan-duncan left a comment

Choose a reason for hiding this comment

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

I didn't see this until review request now...

@rachelmsimm
Copy link
Collaborator Author

I didn't see this until review request now...

@brendan-duncan that's okay, thanks for reviewing it. This work is on hold anyways while I work on some higher pri tickets.

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.

4 participants