-
Notifications
You must be signed in to change notification settings - Fork 5
Update a little and allow pulling to projects using jitpack.io #11
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial thoughts. Currently, I think only Matt can merge things here.
|
||
groupId = 'com.github.rust-mobile' | ||
artifactId = 'rustview' | ||
version = '1.0.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Version 1.0.0 seems optimistic here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I (and maintainers) can set any version, i used 1.0.0 just for check it is working
from components.release | ||
|
||
groupId = 'com.github.rust-mobile' | ||
artifactId = 'rustview' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name "rustview" isn't really something we've used for this before. I agree that "Android View" is a bit hard to justify here, but it's probably worth using that for now. rust-mobile
already explains that this is Rust, so I think sticking with Android View is best
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of android native library "Android View"
Can you please provide a link to this? I've been unable to find this through searching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
afterEvaluate { | ||
publishing { | ||
publications { | ||
release(MavenPublication) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, this is required for jitpack, right?
That is, this isn't doing automatic pushing to Maven Central?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only for jitpack, MavenPublication is just class describing output format
android { | ||
compileSdk 31 | ||
namespace 'org.linebender.android.rustview' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be moved to com.github.rust-mobile
(as the others should as well)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This namespace should be similar to projects folder structure as on screenshot in my previous comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. I guess we can make the move away from org.linebender
a follow-up
let _ = tracing_subscriber::registry() | ||
tracing_subscriber::registry() | ||
.with(tracing_android_trace::AndroidTraceLayer::new()) | ||
.try_init(); | ||
.init(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change necessary? And why hasn't the comment above needed changing as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be panic if android platform specified incorrectly when using cargo ndk. Ignoring this possible error gives a false sense of security
I totally don't know who is Matt, i just needed it and found it in internet and did this changes for myself, i think it could be useful but feel free editing it |
No description provided.