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

Rework Android support #3675

Open
lefou opened this issue Oct 5, 2024 · 22 comments
Open

Rework Android support #3675

lefou opened this issue Oct 5, 2024 · 22 comments

Comments

@lefou
Copy link
Member

lefou commented Oct 5, 2024

Some thinks I noticed which we should reconsider before finalizing the API:

  • Various tasks like sdkUrl or buildToolsVersion should be prefixed with android
  • Hardcoded buildToolsVersion defaults to 35.0.0. I suggest to just remove the default.
  • Hardcoded platformsVersion defaults to android-35. We could derive it from buildToolsVersion.
  • In general lots of various paths which are just sub-paths of buildToolsPath have their own tasks. Instead we could have some "config" class, which returns common paths from a buildToolsPath, e.g. by applying a (version specific) layout/sub-paths.
  • AndroidAppModule.androidSdkModule should have the type ModuleRef[AndroidSdkModule].
  • androidResources hardcodes a path to src/main/AndroidManifest.xml. It should either lookup the file from sources task, or be it's own androidManifest: Source task.
  • androidKeystore is probably meant to be a persistent task
@himanshumahajan138
Copy link
Contributor

himanshumahajan138 commented Oct 5, 2024

@lefou Sir Do i need to resolve these tasks also for Bounty Completion ?

@lihaoyi
Copy link
Member

lihaoyi commented Oct 5, 2024

@himanshumahajan138 no need, I'll transfer the bounty based on the initial merge, we can continue to iterate on the code in main

@lihaoyi
Copy link
Member

lihaoyi commented Oct 5, 2024

@lefou yeah we can take a few more passes at it. I marked the whole thing as @experimental for now which it definitely is

@lihaoyi
Copy link
Member

lihaoyi commented Oct 5, 2024

I'll submit a PR with the suggested updates

@himanshumahajan138
Copy link
Contributor

himanshumahajan138 commented Oct 5, 2024

BTW i am Interested in contributing to this also and after this i want to work with kotlin examples

Few things i want to mention:
Sir Discord link is not working i want to join but unable to do so...

@lihaoyi
Copy link
Member

lihaoyi commented Oct 5, 2024

Various tasks like sdkUrl or buildToolsVersion should be prefixed with android

I think in this case we can leave them un-prefixed. The prefixing is mostly for mixin/stackable traits where there is risk of name collisions, e.g. AndroidAppModule stacks on top of JavaModule. AndroidSdkModule seems pretty standalone so not sure if we need to prefix all the tasks

@lefou
Copy link
Member Author

lefou commented Oct 5, 2024

Various tasks like sdkUrl or buildToolsVersion should be prefixed with android

I think in this case we can leave them un-prefixed. The prefixing is mostly for mixin/stackable traits where there is risk of name collisions, e.g. AndroidAppModule stacks on top of JavaModule. AndroidSdkModule seems pretty standalone so not sure if we need to prefix all the tasks

I was thinking a sdkUrl is perfectly reasonable for a Java SDK too, which we will get sooner or later when we are able to select the Java version. Although I overlooked that AndroidSdkModule is standalone. we don't know what users are going to do, e.g. combine a Java and Android SDK module. If we follow our de-facto rule and prefix tasks with the common module name, it won't hurt.

lihaoyi added a commit that referenced this issue Oct 5, 2024
Tries to address some of the issues in
#3675


* [ ] Various tasks like `sdkUrl` or `buildToolsVersion` should be
prefixed with `android`
* [x] Hardcoded `buildToolsVersion` defaults to `35.0.0`. I suggest to
just remove the default.
* [x] Hardcoded `platformsVersion` defaults to `android-35`. We could
derive it from `buildToolsVersion`.
* [ ] In general lots of various paths which are just sub-paths of
`buildToolsPath` have their own tasks. Instead we could have some
"config" class, which returns common paths from a `buildToolsPath`, e.g.
by applying a (version specific) layout/sub-paths.
* [x] `AndroidAppModule.androidSdkModule` should have the type
`ModuleRef[AndroidSdkModule]`.
* [x] `androidResources` hardcodes a path to
`src/main/AndroidManifest.xml`. It should either lookup the file from
`sources` task, or be it's own `androidManifest: Source` task.
* [x] `androidKeystore` is probably meant to be a persistent task
@lihaoyi
Copy link
Member

lihaoyi commented Oct 6, 2024

There's two separate discussions here:

  1. For this specific case, I don't mind renaming sdkUrl to androidSdkUrl. It's not too verbose and does clarify things a bit. Even adding android* to all the other members isn't too verbose, although it does get repetitive.

  2. As a more general policy, I don't think every trait should have all of its members prefixed.

    • e.g. the "base traits" like JavaModule or ScalaModule or ZincWorkerModule don't prefix everything with java* or scala* or zincWorker*.
    • OTOH traits like PalantirFormatModule or TestModule do tend to prefix everything with palantirFormat* or test*, since their primary use case is to be mixed into other traits that collide.

We haven't had a very strict rule in the past, but I think if we want to formalize one then having some distinction between "base traits" and "mixin traits" is useful, since they do tend to be used pretty differently in practice, and so different constraints apply. I don't think we should have an expectation that arbitrary totally unrelated module traits can be mixed together and continue working without issue.

Maybe in 0.13.0 we can consider turning some of the "base traits" into abstract classes so they can't be used as mixins, but that would be a bit further off.

@lefou
Copy link
Member Author

lefou commented Oct 7, 2024

It's a bit unclear to me, why we don't come up with some default instances of the AndroidSdkModule. Are there any user-specific limitations like licence agreements or personal tokens? If, they should be explicitly mentioned in the docs. If not, it would be nice to have some ready-to-used objects for current major Android versions. Otherwise, users will do repeated work over and over.

@lefou
Copy link
Member Author

lefou commented Oct 7, 2024

And if we can provide it pre-configured, we probably should overthink the use of a ModuleRef, since it can't be switched based on a target result. Instead, we should make it some config class and a worker task, so we can infer the best "AndroidSDKSupport" from the configured androidVersion.

@himanshumahajan138
Copy link
Contributor

himanshumahajan138 commented Oct 7, 2024

actually, @lefou Sir you are saying right to have readymade setup but what i think could me more better is to use CI (github actions) for installing android of specific version just like we do for java during setup of artifacts...

i think this will eliminate this android sdk module totally and we have to rely only on the AndroidAppModule

Plus point: this will also eliminate reinstalling of android sdk and tools again and again for every build
what are your thoughts ....

@lihaoyi
Copy link
Member

lihaoyi commented Oct 7, 2024

@lefou I went with a separate module because i wasnt sure how much would need to be typically configured; if it's always the same few instances we can provide them, if they're always specially configured then providing default instances doesnt really help. TBH I don't know enough about typical Android development to have an intuition here so the choice of manually instantiated SDK modules is pretty arbitrary

@lefou
Copy link
Member Author

lefou commented Oct 7, 2024

I guess that users doing a lot of Android stuff will prefer to have some system (or user) installed SDK and want to just point the SDK path to that. But for occasional users who only checkout some project that also has some Android app, Mill should handle it smoothly without any local installations. We probably want a shared external worker module that can fetch and unpack standard SDKs, but accepts environment variable or some other means of shared SDKs.

@0xnm
Copy link
Contributor

0xnm commented Oct 8, 2024

But for occasional users who only checkout some project that also has some Android app, Mill should handle it smoothly without any local installations

The common expectation is still to have Android SDK installed before using the build tool. The main concern is probably accepting the license: while when running CI tasks for the Mill project it can be accepted there, because end-users are Mill contributors, accepting it on behalf of the user in case of the user builds is no-go.

This is what Android Gradle Plugin is doing, if I'm not wrong - it expects Android SDK to be already installed and be registered via env variable or have a path declaration in Gradle properties.

@himanshumahajan138
Copy link
Contributor

Exactly, i am too considering installation during Ci as best option as this will make it general and available for everyone during testing just like we do with java setup

And as u said that gradle suppose to have sdk already installed then according to this we should also make sure that android is present during ci testing and rest is user work they have to install Android sdk on their system for local usage.

@lefou
Copy link
Member Author

lefou commented Oct 10, 2024

Ok, then the answer is: The user needs to pre-install the Android SDK because of license requirements.

It's quite like the situation we had for older Java SDKs. If we want to configure it in Mill, but need to refer to it via some local configuration, we probably need some mapping of configured version to local path. Either we look for some env var pattern or some config file under .config, e.g. .config/mill-android-sdks or .config/mill-android-sdk.<version> where <version> is the same as in AndroidSdkModule.buildToolsVersion.

@0xnm
Copy link
Contributor

0xnm commented Oct 10, 2024

Either we look for some env var pattern or some config file under .config, e.g. .config/mill-android-sdks or .config/mill-android-sdk. where is the same as in AndroidSdkModule.buildToolsVersion.

Not exactly like that. To be precise, there is only one Android SDK and to get it one can go to https://developer.android.com/studio, scroll to Command line tools only and download the archive. It then should be unpacked and the final path should be registered under ANDROID_SDK_ROOT (or ANDROID_HOME) env variable (to be precise, the location of cmdline tools shouldn't be the same as Android SDK root folder, but for the simplicity it can be like that).

And then by using sdkmanager coming with cmdline tools build system (Gradle, Mill) can pull the API sources/build tools/etc. version it needs. Just on the first usage user needs to accepts the license, and after it is done, build system should be able to get everything it needs.

To simplify: first Mill should do env variable lookup to locate Android SDK root and fallback to the some configuration path entry if not found. Once path is known, Mill can pull the components needed by using sdkmanager.

@lefou
Copy link
Member Author

lefou commented Oct 10, 2024

@0xnm So, each new version of the SDK comes with all older libraries of older releases included and grows ever larger? Or how are we supposed to target older Android versions?

@himanshumahajan138
Copy link
Contributor

himanshumahajan138 commented Oct 10, 2024

Let me make this conversation a lil bit easy

Actually we need to follow exactly same steps present in the AndroidSdkModule.scala means same cmd commands needs to be implemented using actions or what we use currently and then talking about version we have update the version manually when new updates comes coz there is no official github actions available for Android and finally we will export the to env and use them from there

I think this will clear the whole process

@0xnm
Copy link
Contributor

0xnm commented Oct 10, 2024

@lefou No, the set of the components (build tools, sources for the specific API, etc.) can be controlled with sdkmanager which is a part of the cmdline tools. cmdline tools just should just exist under the registered Android SDK root location and then can be leveraged to get everything the particular build system needs.

@lefou
Copy link
Member Author

lefou commented Oct 10, 2024

Thanks for the clarifications. Since me and probably also other Mill devs are not that familiar with Android tooling, let me summarize it:

There is only one Android SDK location, which is required to be at least as new as the target version, we want to use. It needs to pre-exists and can't automatically installed due to licensing requirements.

In this SDK is a tool sdkmanager which a user or Mill can use to configure (read "fetch") support (libs, tools) for a specific target version. These files are stored inside the SDK, so we later just assemble the path from the SDK location, the target version and the tools name.

I think want to support the (most likely) already defined env var ANDROID_SDK_ROOT. We could fall back to read a .config/mill-android-sdk file.

As a consequence, we should redesign AndroidSdkModule to only have a sdkUrl and a worker that manages the target versions. That worker can be ask for the version specific tools support, which can on first access use the sdkmanager to prepare the target version (if not already present).

@lefou
Copy link
Member Author

lefou commented Oct 10, 2024

As a implementation hint, we should enable the revalidate flag on all PathRefs which point to Android SDK locations (outside of Mill's cache), so Mill can detect if these paths are no longer present or changed.

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

No branches or pull requests

4 participants