-
Notifications
You must be signed in to change notification settings - Fork 9
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
Build for selected Android targets #119
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.
Self-review.
/// armeabi-v7a, | ||
/// x86_64, | ||
/// x86 | ||
/// arm64-v8a,armeabi-v7a,x86_64,x86 |
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.
Small usability improvement: I found myself copy pasting this from the help text.
"{{ t }}" | ||
{%- if !loop.last %}, {% endif %} | ||
{%- endfor %} | ||
} |
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.
Oh my, all that for this!
@@ -92,6 +92,9 @@ where | |||
for<'a> T: Deserialize<'a>, | |||
{ | |||
let file = file.as_ref(); | |||
if !file.exists() { | |||
anyhow::bail!("File {file} does not exist"); | |||
} |
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.
Small usability fix.
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.
Haven't actually tested this yet but with the little context I currently have on the code base, nothing jumped out at me.
crates/ubrn_cli/src/codegen/mod.rs
Outdated
Ok(()) | ||
} | ||
} | ||
|
||
pub(crate) trait RenderedFile: DynTemplate { | ||
fn path(&self, project_root: &Utf8Path) -> Utf8PathBuf; | ||
fn project_root(&self) -> Utf8PathBuf { | ||
// This provides a convenience for templates to calculate | ||
// relative paths between one anoter. |
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.
// relative paths between one anoter. | |
// relative paths between one another. |
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.
LGTM
Fixes #106 This issue is conceptually simple: we need to add one line to the `build.gradle` template. Driving that line in the file is data about the targets being built. `targets` comes from the `ProjectConfig` (a.k.a. `ubrn.config.yaml`), which makes this straightforward. HOWEVER, `--targets` can be used when building! SO, we can mutate the `ProjectConfig` while we're building. BUT we don't want the second command overwriting the `build.gradle` so carefully specified in the first. ```sh ubrn build android --and-generate --config ubrn.config.yaml --targets aarch64-linux-android,armv7-linux-androideabi ubrn build ios --and-generate --config ubrn.config.yaml ``` THIS MEANT: adding platform specificity to the each of the generated files.
1078abd
to
d90a74a
Compare
Just to report back, I've tested this in https://github.com/unomed-dev/react-native-matrix-sdk and it appears to work great. 👍 |
Fixes #106
According to The Big O of Code Reviews, this is a O(n) change.
This issue is conceptually simple: we need to add one line to the
build.gradle
template.Driving that line in the file is data about the targets being built.
targets
comes from theProjectConfig
(a.k.a.ubrn.config.yaml
), which makes this straightforward.HOWEVER,
--targets
can be used when building!SO, we can mutate the
ProjectConfig
while we're building.BUT we don't want the second command overwriting the
build.gradle
so carefully specified in the first.THIS MEANT: adding platform specificity to the each of the generated files.
Drive-by Review also requested from @Johennes , if interested.