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 Android init methods for more flexibility #159

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mcginty
Copy link
Contributor

@mcginty mcginty commented Feb 12, 2025

After integrating rustls-platform-verifier into a codebase that uses Android, I wrote this PR to propose a more flexible initialization API. Specifically, it would be nice to, rather than passing in a context JObject that isn't Copy/Clone, to instead allow people to pass in a GlobalRef directly, which is what already happens behind the scenes in init_hosted.

I also modified the names of the existing methods as well to try to be more clear about what each of the 3 init options offer. I also kept the old names and marked them as deprecated to allow for backwards-compatibility.

Old New
fn init_hosted(env: &mut JNIEnv, context: JObject) -> Result<(), JNIError> fn init_with_env(env: &mut JNIEnv, context: JObject) -> Result<(), JNIError>
fn init_external(runtime: &'static dyn Runtime) fn init_with_runtime(runtime: &'static dyn Runtime)
fn init_with_refs(java_vm: JavaVM, context: GlobalRef, loader: GlobalRef)

@mcginty mcginty force-pushed the android_ref_dev branch 6 times, most recently from 5ce176b to 06a3b12 Compare February 12, 2025 01:35
@djc
Copy link
Member

djc commented Feb 12, 2025

IMO it would be nice to split this into three commits, one for each rename and one for the new API. I'm not entirely sure whether it's worth it to force downstream users to go through a deprecation at this point for the cosmetic benefit of a better name -- maybe start with a soft deprecation as part of a comment?

@mcginty
Copy link
Contributor Author

mcginty commented Feb 12, 2025

@djc good call, I like that plan better too. It also makes me think I can do a better job at documenting the differences in the rustdocs, so I'll take care of that and report back when they're broken into independent commits.

this is a soft deprecation, only noted as a rustdoc.
this is a soft deprecation, only noted as a rustdoc.
@mcginty
Copy link
Contributor Author

mcginty commented Feb 12, 2025

Alright, done! How does this rendition look?

Copy link
Member

@djc djc left a comment

Choose a reason for hiding this comment

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

This is much better!

My review is by no means sufficient for this code, but this looks okay to me.

rustls-platform-verifier/src/android.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@complexspaces complexspaces left a comment

Choose a reason for hiding this comment

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

This looks good, thanks for the addition. I built the docs locally to confirm the warning text shows up correctly as well:
image

rustls-platform-verifier/src/android.rs Outdated Show resolved Hide resolved
Copy link
Member

@cpu cpu left a comment

Choose a reason for hiding this comment

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

Just a few small nits to note. Nice tidy :-)

rustls-platform-verifier/src/android.rs Outdated Show resolved Hide resolved
rustls-platform-verifier/src/android.rs Outdated Show resolved Hide resolved
@mcginty
Copy link
Contributor Author

mcginty commented Feb 14, 2025

Pushed a commit with all the fixes for the typos mentioned in the review and while going through it one more time, realized that the warning block had some unnecessary commas and wasn't rendering markdown backticks as expected, so I just removed them.

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