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

Simplify build setup process #356

Closed
wants to merge 1 commit into from
Closed

Simplify build setup process #356

wants to merge 1 commit into from

Conversation

voczi
Copy link

@voczi voczi commented Mar 25, 2024

Hi!
I made these changes to, hopefully, make it easier for people to build the project. I've ran this on Windows and it's worked fine so far.
Please let me know if there's any issues with the changes. One thing I am unsure about is whether or not the workflows will work properly after this.

@voczi voczi changed the title Change build instructions accordingly Simplify build setup process Mar 25, 2024
@voczi voczi force-pushed the main branch 2 times, most recently from b997947 to 3c60d56 Compare March 25, 2024 21:01
@dae
Copy link
Contributor

dae commented Mar 26, 2024

Please provide an explanation of why we should change the existing working code. What advantages does this new approach have? What trouble did you have with the existing setup?

@voczi
Copy link
Author

voczi commented Mar 26, 2024

Please provide an explanation of why we should change the existing working code. What advantages does this new approach have? What trouble did you have with the existing setup?

The troubles I had with the existing setup was that there were a few hard-coded restrictions set in build_rust for reasons I didn't quite understand. Mostly related to multi-arch builds.
The main purpose of these changes is to remove those restrictions and let the user configure the build process themselves. To be fair, running this through Gradle isn't necessary and the options could be set as environment variables instead. So unsure if the main build approach, as documented in the README, really should be switched to building through Gradle instead.

@dae
Copy link
Contributor

dae commented Mar 26, 2024

Multi-arch builds are restricted to macOS, because you can only legally target macOS from macOS.

The current approach tries to keep the Rust build process independent from Gradle. That avoids any startup time, and makes things easier to debug. Gradle is only required later, when taking the Rust outputs and bundling them into an .aar/.jar.

@voczi
Copy link
Author

voczi commented Mar 26, 2024

Yep, that makes sense. But the current Rust code throws a panic when trying to do a multi-arch build, because the multi-arch setting for tests (Robolectric) and regular builds (JNI) is not separated. Anyhow, the code I proposed avoids throwing a panic altogether. Maybe the panic can still be kept, indicating a wrong combination; tests.multi_arch+non-macOS=panic (?)
For the other part, I can agree on that Gradle made it harder to debug the cargo builds.

So maybe the Gradle part, of these changes, isn't quite necessary. But should the environment variables be kept for better configuration of the builds?

@dae
Copy link
Contributor

dae commented Mar 26, 2024

If the user asks for a multi-arch build when it's not possible, I think stopping is the best course of action. In practice, multi-arch builds are only going to be done in CI (or when testing the build process on a Mac machine) - it's not something that's going to get used during regular development.

better configuration of the builds

Sorry, could you explain this better? We have a working build process at the moment. What advantages will these changes bring?

@voczi
Copy link
Author

voczi commented Mar 26, 2024

This was more intended for improving local builds, rather than for the automated ones. In my case, I wanted to create a local multi-arch build so I can run any changes on my personal phone (arm64) too.

@dae
Copy link
Contributor

dae commented Mar 26, 2024

Ok, I see. This was not something I tried to support, as once the code has been tested locally in the emulator, it can be pushed to CI and the artifacts for other platforms can be downloaded from the CI assets.

If you still want the ability to do this locally, a PR that makes the minimum changes necessary to accomplish this would be good. Maybe the introduction of an env var that controls the desired android platform? If not set, it could keep defaulting to the current arch.

@voczi
Copy link
Author

voczi commented Mar 26, 2024

That, also, sounds good. I'll convert this to a draft for now and work on it later.
Thank you!

@voczi voczi marked this pull request as draft March 26, 2024 11:38
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.

None yet

2 participants