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

Github Actions: add android.yml to build for Android #263

Closed

Conversation

leleliu008
Copy link

Signed-off-by: leleliu008 [email protected]

@leleliu008 leleliu008 force-pushed the add-github-actions-android-yml branch from 66bbea3 to 9ff7a5c Compare January 21, 2022 11:29
@leleliu008 leleliu008 force-pushed the add-github-actions-android-yml branch from 9ff7a5c to e4dabf0 Compare January 21, 2022 11:37
Copy link
Collaborator

@niklasmohrin niklasmohrin left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! I have some questions about the setup before we can merge this


- run: brew install tree file

- run: patch -p1 < android.patch
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not just apply the patch and persist that change?

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean why not I file a pull request to apply the android.patch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, let's just apply the patch and commit these changes instead of having to maintain the patch file

Copy link
Author

Choose a reason for hiding this comment

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

28

I have said in #260

have you read the patch? do you accept this patch file. I'm fairly new to Rust language. I have no ablity to write perfect code in Rust now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The patch seems to go in the right direction, not a lot of Rust is required (and we can help you / tell you what to do to fix errors if they come up). Just run patch -p1 < android.patch, remove android.patch and push all of these changes here (if you need git help, feel free to reach out too) :)

matrix:
target-abi: [armeabi-v7a, arm64-v8a, x86, x86_64]

runs-on: macos-10.15
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need to build on mac? It usually takes longer until one gets a mac machine for actions and they are slower than the linux counterparts

Copy link
Author

Choose a reason for hiding this comment

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

I can change it to Ubuntu-20.04

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the other workflow we use ubuntu-latest, so that should do it :)

@@ -0,0 +1,27 @@
name: cross compile with android-ndk
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we can just put this job into the workflow file we already have

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I will do it.

@@ -0,0 +1,285 @@
#!/bin/sh
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where does this file come from? It it related to this repository with the same name? I would definitely prefer if we wouldn't have to vendor this script in our repository (and surely not in the root directory)

Copy link
Author

Choose a reason for hiding this comment

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

this file is written by me and it is nothing to do with cargo-ndk project

I used to wanna use cargo-ndk project, but it has bugs, and finally I decide to write my own version in shell. It is very simple and readable.

I developed a tool call ndk-pkg, if you want keep the workflow simple, you can use this tool.

name: Ubuntu

on:
  push:
    branches: [ master ]
  pull_request:
    branches: [ master ]

jobs:
  test:

    runs-on: ubuntu-20.04

    steps:
      - uses: actions/checkout@v2

      - run: curl -LO https://raw.githubusercontent.com/leleliu008/ndk-pkg/master/bin/ndk-pkg

      - run: chmod +x ndk-pkg

      - run: ./ndk-pkg update

      - name: create formula
        run:  |
          cat > ~/.ndk-pkg/repos.d/offical/formula/tealdeer.sh << EOF
          package set summary "Very fast implementation of tldr in Rust"
          package set git.url "https://github.com/dbrgn/tealdeer.git"
          package set src.url "dir://${{ github.workspace }}"
          package set license "Apache-2.0"
          package set dep.pkg "openssl"
          package set bsystem "cargo"
          EOF

      - run: ./ndk-pkg install tealdeer --tree

Copy link
Author

Choose a reason for hiding this comment

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

I have port this project to ndk-pkg, here is a github action example https://github.com/leleliu008/ndk-pkg-formula-repository/runs/4992613697?check_suite_focus=true

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah that looks interesting 👍 If you have the tealdeer formula in your package base, we don't need to run cargo-ndk here, right? I am happy with applying the android.patch and committing this here, so you don't need to patch the code in the formula. Additionally, we can run a standard cargo build for the android target in CI so that it doesn't break out of the blue (testing is harder). Does that sound good to you?

Copy link
Author

Choose a reason for hiding this comment

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

If you have the tealdeer formula in your package base, we don't need to run cargo-ndk here, right?

In my ndk-pkg formula repo, I use the stable tarball file https://github.com/dbrgn/tealdeer/archive/v1.5.0.tar.gz , not fetch from the github source tree HEAD

we can run a standard cargo build for the android target in CI so that it doesn't break out of the blue (testing is harder).

just run cargo build --target armv7-linux-androideabi is not enough. some certain environment variables must be set, that's why I wrote cargo-ndk.sh.

@niklasmohrin
Copy link
Collaborator

I must admit, I am not comfortable with cargo-ndk, this just adds too much code to maintain to this repository. In #274, I noted that there is the possibility of using cross which cross-compiles using docker containers. This works great, bat uses it, I use it in another project and it is very easy to use with the actions-rs/cargo action we already use. About the compile error from the issue: We should just remove this function altogether, because the Other variant does not exist anyways

@dbrgn
Copy link
Owner

dbrgn commented Jun 14, 2022

In #274, I noted that there is the possibility of using cross which cross-compiles using docker containers.

Does that just build for ARM, or actually on an Android device?

@niklasmohrin
Copy link
Collaborator

You can choose any of the rust targets (for example this list), we would probably be most interested in aarch64-linux-android and friends. If I understand it correctly, cross uses a docker container for cross compiling and qemu to run tests - or maybe they also compile in qemu. If we just build without testing, we don't need to worry about any ndk stuff, otherwise, we may have to a little bit of code to hook into that. I don't really know anything about android development, but that's just another reason why I don't want to host these specifics in our repo :D

@niklasmohrin niklasmohrin added the waiting-for-author The PR needs an update before it can be considered for merging. label Oct 1, 2022
@niklasmohrin
Copy link
Collaborator

@leleliu008 Do you want to continue working on this, or should we close this PR for now?

@leleliu008
Copy link
Author

Since the feature has been implemented in #274, this is no need to go on. Please feel free to close it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting-for-author The PR needs an update before it can be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants