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

Remove split subdirectories as part of #372 modularization #374

Merged
merged 4 commits into from
Dec 21, 2022

Conversation

rib
Copy link
Contributor

@rib rib commented Dec 7, 2022

This PR removes ndk-glue, ndk-macro, cargo-apk, ndk-build, ndk-examples and ndk-context from this repo as part of the modularization effort described in issue #372

Supersedes: #373

The ndk-glue and ndk-macro subdirectores have been split into a
stand-alone repository at: https://github.com/rust-mobile/ndk-glue

This is being done as part of the modularization effort described in
issue rust-mobile#372

The stand-alone repository was filtered like this:

```
git clone https://github.com/newren/git-filter-repo
git clone https://github.com/rust-windowing/android-ndk-rs.git ndk-glue
cd ndk-glue
py ../git-filter-repo/git-filter-repo \
    --path ndk-glue --path ndk-macro \
    --path ndk-examples --path-rename ndk-examples:examples \
    --path LICENSE-APACHE --path LICENSE-MIT \
    --path Cargo.toml --path rustfmt.toml \
    --path .github --path .gitignore
```

The last commit in this repo that is included within the
stand-alone repository is 107f03e

Although ndk-glue is also being deprecated at the same time as
splitting it out this commit doesn't make any README changes for
this repo yet which will be addressed separately.
The ndk-context subdirectory has been split into a
stand-alone repository at: https://github.com/rust-mobile/ndk-context

This is being done as part of the modularization effort described in
issue rust-mobile#372

The stand-alone repository was filtered like this:

```
git clone https://github.com/newren/git-filter-repo
git clone https://github.com/rust-windowing/android-ndk-rs.git ndk-glue
cd ndk-glue
py ../git-filter-repo/git-filter-repo \
    --path ndk-context \
    --path ndk-examples --path-rename ndk-examples:examples \
    --path LICENSE-APACHE --path LICENSE-MIT \
    --path Cargo.toml --path rustfmt.toml \
    --path .github --path .gitignore
```

The last commit in this repo that is included within the
stand-alone repository is 107f03e
…modularization

The cargo-apk, ndk-build + ndk-examples subdirectores have been split into a
stand-alone repository at: https://github.com/rust-mobile/cargo-apk

This is being done as part of the modularization effort described in
issue rust-mobile#372

The stand-alone repository was filtered like this:

```
git clone https://github.com/newren/git-filter-repo
git clone https://github.com/rust-windowing/android-ndk-rs.git ndk-glue
cd ndk-glue
py ../git-filter-repo/git-filter-repo \
    --path cargo-apk --path ndk-build \
    --path ndk-examples --path-rename ndk-examples:examples \
    --path LICENSE-APACHE --path LICENSE-MIT \
    --path Cargo.toml --path rustfmt.toml \
    --path .github --path .gitignore
```

The last commit in this repo that is included within the
stand-alone repository is 9b4dbfa

The CI for this repo is considerably simpler now that the examples
have been moved to the `cargo-apk` repository since it now just checks
that the `ndk` and `ndk-sys` crates successfully cross compile without
running a hello_world example under an emulator.

Runtime tests could be re-added in the future, but especially while
there is no significant test coverage from the examples it's more
valuable to keep those for testing `cargo-apk` packaging and simplify
testing in this repo.
@rib rib marked this pull request as ready for review December 9, 2022 18:16
@rib
Copy link
Contributor Author

rib commented Dec 9, 2022

@MarijnS95 it would be good if you could also ACK/review this PR if you get a moment

@rib
Copy link
Contributor Author

rib commented Dec 12, 2022

Just a quick ping @MarijnS95 since I see you're looking at the standalone cargo-apk repo now.

I'd like to use the merging of this PR as the final commitment that the standalone repos are acceptable so if you've had a chance to review the state of the new repo it would be good if you can ACK here that you're happy for this to be merged.

I take it @dvc94ch is happy with the ndk-context repo, and hopefully you are both ok with the ndk-glue repo too.

My stance is that until we merge this then we can still potentially re filter the standalone repos if we absolutely need to so we probably shouldn't go too far ahead with using the new repos before agreeing that we're happy with them?

I think if we can get an OK from you @MarijnS95 then we can probably just go ahead and merge this.

@MarijnS95
Copy link
Member

MarijnS95 commented Dec 12, 2022

Just a quick ping @MarijnS95 since I see you're looking at the standalone cargo-apk repo now.

It's Monday now so I'm slowly going though all the things ;)

I'd like to use the merging of this PR as the final commitment that the standalone repos are acceptable so if you've had a chance to review the state of the new repo it would be good if you can ACK here that you're happy for this to be merged.

And yes, that implies I'm having a little look in all different repos and see if everything is in place. For cargo-apk I'm committed to use that repo after the stupidly slow emulator finishes a couple hours later, and curious if the release process still works too (with my own secret, since I mostly publish it anyway).

Comment on lines +78 to +79
- name: Install cargo-ndk
run: cargo install cargo-ndk
Copy link
Member

Choose a reason for hiding this comment

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

/me pukes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I was worried you were going to dislike it.

It makes sense to me for now though, we just want to cross compile a library crate and that's not really what cargo apk is for.

Hopefully this can just be seen as a minimal adaptation that's pragmatic for now?

Copy link
Member

Choose a reason for hiding this comment

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

As long as it works... The CI is green, so it surprisingly does?

Comment on lines +57 to +60
- { triple: 'armv7-linux-androideabi', abi: armeabi-v7a }
- { triple: 'aarch64-linux-android', abi: arm64-v8a }
- { triple: 'i686-linux-android', abi: x86 }
- { triple: 'x86_64-linux-android', abi: x86_64 }
Copy link
Member

Choose a reason for hiding this comment

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

I saw you created an issue about the abi mess, hope it'll be resolved soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a bit of a weird quirk with cargo ndk

# Use one of our supported targets to lint all crates including
# the target-specific `ndk` in one go.
# This assumes our host-tools (cargo-apk and dependencies)
# also compile cleanly under this target.
# Use a cross-compilation target (that's typical for Android) to lint everything
Copy link
Member

@MarijnS95 MarijnS95 Dec 12, 2022

Choose a reason for hiding this comment

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

Can you clean this up in the inverse way in the cargo-apk repository, since it specifically does not have to run on the target arch anymore, but on a (set of) host(s)?

OTOH that host might as well be an Android phone; linting for all the triples is fine by me but the comment is dated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it'd make sense in cargo-apk to lint with host targets, since it's a tool that runs on the host. It should be easy to follow up on iterating / cleaning up the CI in any of the standalone repos.

My first changes were just focused on making minimal CI updates that could ensure everything was building with green lights in the standalone repos.

Copy link
Member

Choose a reason for hiding this comment

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

Mostly the comment that's off now, I'm fine having "android" itself as a host if someone wants to build APKs from Android for Android (Termux anyone?).

@MarijnS95
Copy link
Member

This PR doesn't seem to update the root README.md, nor has a new root readme been added (or symlinked) in at least cargo-apk. Can you look into that? I'm not sure what we should make it look like, the matrix was always relevant and useful to easily click through, adding repository links and a small "use with this crate to achieve X" description might be nice too (since most crates are rather standalone and compatible with alternative systems now).

@dvc94ch
Copy link
Contributor

dvc94ch commented Dec 12, 2022

When did we decide to comply with the ridiculous woke agenda and rename all branches to main? I don't live in an oppressive white patriarchy where people are so traumatized by their ancestors enslavement that happened to people of all races that they have PTSD when they see the word master, despite it being derived from the older term master copy, not slave master. I'm also not interested in appeasing or condoning this sick world view in any way

@rib
Copy link
Contributor Author

rib commented Dec 12, 2022

This PR doesn't seem to update the root README.md, nor has a new root readme been added (or symlinked) in at least cargo-apk. Can you look into that? I'm not sure what we should make it look like, the matrix was always relevant and useful to easily click through, adding repository links and a small "use with this crate to achieve X" description might be nice too (since most crates are rather standalone and compatible with alternative systems now).

Yeah, this is intentional and noted in #372. The intention is to follow up with README changes seperately and not hold up the initial separation (since I guess there will be more discussion / iteration around readme changes)

@rib
Copy link
Contributor Author

rib commented Dec 12, 2022

When did we decide to comply with the ridiculous woke agenda and rename all branches to main? I don't live in an oppressive white patriarchy where people are so traumatized by their ancestors enslavement that happened to people of all races that they have PTSD when they see the word master, despite it being derived from the older term master copy, not slave master. I'm also not interested in appeasing or condoning this sick world view in any way

I'm not sure how to respond to this. This is far too heated, and politically charged, for a GitHub PR discussion.

main is a conventional name for the main branch that's also the default recommended by GitHub. I copy and pasted the commands from GitHub when pushing the standalone repos.

@dvc94ch
Copy link
Contributor

dvc94ch commented Dec 12, 2022

main is a conventional name for the main branch that's also the default recommended by GitHub. I copy and pasted the commands from GitHub when pushing the standalone repos.

the traditional name is master, and it bothered no one until recently. it clearly is a political statement, otherwise no one would have bothered to change it. it requires accepting the premises of a radical ideology that is less than a decade old.

@rib
Copy link
Contributor Author

rib commented Dec 13, 2022

I've renamed the branch for ndk-context to master as this repo is owned by @dvc94ch.

@MarijnS95 can rename the branch for cargo-apk or ndk-glue if he wants.

@rib
Copy link
Contributor Author

rib commented Dec 13, 2022

Okey, hopefully we can move on from that.

Considering @MarijnS95 has gone ahead and spun a release out of the standalone cargo-apk repo, including the addition of tags I think that effectively commits us to the standalone cargo-apk repo at this point (any rebasing/re-filtering would change commit hashes) and e.g. from this comment from @MarijnS95 rust-mobile/cargo-apk#4 (comment) he's ok with moving ahead based on PRs/issues to address any feedback with the commits I made (such as for the porting of examples that @MarijnS95 had some comments on already).

So I think this is ready to merge a this point 🤞

Copy link
Member

@MarijnS95 MarijnS95 left a comment

Choose a reason for hiding this comment

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

Fine by me if you can follow up on the readme changes here and elsewhere. I'll slowly start processing more of #372 and the resulting repos but, as mentioned before, I'm quite occupied unfortunately 😅 😢

@MarijnS95
Copy link
Member

And I really don't care about main vs master and won't waste a second discussing/thinking about it.

@dvc94ch
Copy link
Contributor

dvc94ch commented Dec 13, 2022

And I really don't care about main vs master and won't waste a second discussing/thinking about it.

pretty sure writing that sentence took at least a second.

on another note if you care about ios, I'm making some great progress on using xbuild to build/upload apps to the appstore from linux. I think that will be quite an exciting feature once it lands

@dvc94ch
Copy link
Contributor

dvc94ch commented Dec 20, 2022

so can this be merged? on another note, we probably need some way to communicate about rust-mobile. should we suggest to take over the maintenance of android_logger-rs? see https://github.com/Nercury/android_logger-rs/pull/59 for background

@rib
Copy link
Contributor Author

rib commented Dec 20, 2022

I think this is ready to be merged, yeah. @MarijnS95 are you waiting for anything, or maybe just waiting for someone else to merge? I feel like it's up to you to merge as the current maintainer here @MarijnS95.

regarding communicating about rust-mobile I've at least just created an admins team for the organization to be able to use the github discussion boards between the organisation owners.

regarding android_logger - we should certainly extend an invitation for them to put the repo under the rust-mobile org if they want. It looks like tyranron is happy to pick up maintenance of the repo and could be set as the owner for the repo along with Nercury.

@dvc94ch
Copy link
Contributor

dvc94ch commented Dec 20, 2022

well, since they found new maintainers there's probably no need

@rib
Copy link
Contributor Author

rib commented Dec 20, 2022

The main benefit would probably just be that rust-mobile can serve as a place to collate utility crates that facilitate mobile development with Rust (to help with discoverability) but also for smaller projects like android_logger there's probably a higher chance of maintainers moving on and having them under one umbrella may help with finding someone willing to help with maintenance if wanted in the future.

@MarijnS95
Copy link
Member

Would be great to have them under the unified umbrella, if only for visibility/collation and to decrease bus factor as mentioned elsewhere :)

I think this is ready to be merged, yeah. @MarijnS95 are you waiting for anything, or maybe just waiting for someone else to merge? I feel like it's up to you to merge as the current maintainer here @MarijnS95.

I think this PR is ready to go, just curious where we are for the other repos WRT readme and last-minute comments I had (for example to fix the release workflow) - have those been addressed?

@MarijnS95 MarijnS95 merged commit ce1e652 into rust-mobile:master Dec 21, 2022
@rib
Copy link
Contributor Author

rib commented Dec 22, 2022

Whoop, thanks for merging @MarijnS95 !

I think this PR is ready to go, just curious where we are for the other repos WRT readme and last-minute comments I had (for example to fix the release workflow) - have those been addressed?

Some of this is tracked as remaining tasks under #372 or issues that I think you opened, or hopefully we'll remember some of them in the short term (or I'll also get around to creating more tracking issues too). I was travelling a bit last week so I didn't look at any of those things yet but I was thinking I'd look at opening a PR soon for adding a short-term notice about where some of the subdirectories have moved (before looking at more README updates).

@MarijnS95
Copy link
Member

Heh hope you can still recover those GitHub notifications before force-pushing, I mostly replied to pushed commits on the repo. It's trivial things but they need to happen nevertheless, otherwise I have to not forget them before entering the release cycle :)

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.

3 participants