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

Support Android Targets #260

Open
leleliu008 opened this issue Jan 18, 2022 · 13 comments
Open

Support Android Targets #260

leleliu008 opened this issue Jan 18, 2022 · 13 comments

Comments

@leleliu008
Copy link

https://github.com/dbrgn/tealdeer/archive/v1.5.0.tar.gz

21

@dbrgn
Copy link
Collaborator

dbrgn commented Jan 18, 2022

Android is not currently supported. Just out of curiosity, what kind of Android device do you have that would benefit from a tldr installation?

@dbrgn dbrgn changed the title build for Android failed Support Android Targets Jan 18, 2022
@leleliu008
Copy link
Author

HUAWEI MatePad Pro
matepad-pro

@dbrgn
Copy link
Collaborator

dbrgn commented Jan 18, 2022

Ok 🙂 I guess it shouldn't be too hard to add proper Android support.

@leleliu008
Copy link
Author

--- a/src/types.rs
+++ b/src/types.rs
@@ -43,7 +43,7 @@
 }
 
 impl PlatformType {
-    #[cfg(target_os = "linux")]
+    #[cfg(any(target_os = "linux", target_os = "android"))]
     pub fn current() -> Self {
         Self::Linux
     }
@@ -66,6 +66,7 @@
 
     #[cfg(not(any(
         target_os = "linux",
+        target_os = "android",
         target_os = "macos",
         target_os = "freebsd",
         target_os = "netbsd",

@dbrgn
Copy link
Collaborator

dbrgn commented Jan 18, 2022

A pull request would be welcome. Note that "android" is a dedicated platform type supported by tldr (with some Android-specific pages available), so the PlatformType should be extended with an Android variant (and not just fall back to Linux).

@leleliu008
Copy link
Author

I am not familiar with Rust language syntax. I can only provide the simplest workaround as above shown.

@niklasmohrin
Copy link
Collaborator

A PR for this should also add a job to github actions that makes sure the build still works for android; I am not sure whether we can test in a simulated android environment, a little research from future implementers would be nice :)

@leleliu008
Copy link
Author

We usually just check if can be built for Android and do not check if can be run on Android. Because ARM-based emulators were too slow. but In the real world, 99% Android devices are ARM-based.

@dbrgn
Copy link
Collaborator

dbrgn commented Jun 14, 2022

Android support should be working since #267 was merged. Could you test building the main branch?

@dbrgn dbrgn closed this as completed Jun 14, 2022
@niklasmohrin
Copy link
Collaborator

I wouldn't consider this issue done until we build in CI, it shouldn't be difficult (see my comment in the other PR). I don't think we need to run tests for android in CI, but if its cheap we can do that too.

For me, #267 was mainly about the platform type being available on desktop builds, so I didn't require it there, but this is about supporting the platform which I think needs CI as a safety net. Reopening for now

@niklasmohrin niklasmohrin reopened this Jun 14, 2022
@leleliu008
Copy link
Author

I confired that 543fa22 can be successfully built for Android. I didn't check if can be successfully run on Android device. I think if have problems, end-users will report bugs.

@dbrgn
Copy link
Collaborator

dbrgn commented Jun 14, 2022

@leleliu008 thanks for testing!

@niklasmohrin I agree that having CI would be nice to have.

@leleliu008
Copy link
Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants