-
-
Notifications
You must be signed in to change notification settings - Fork 372
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
Bump Android Command-line Tools from 6.0 to 9.0 #1397
Bump Android Command-line Tools from 6.0 to 9.0 #1397
Conversation
aa9b9f1
to
71c68d7
Compare
During a cutover from a previous version of Briefcase to this one:
I think this approach provides a good strategy to creating better reproducibility with SDK use while also providing a repeatable method for bumping the Command-line Tools version. Please let me know if there's an aspect I'm failing to consider. |
71c68d7
to
5d4b0c3
Compare
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.
This fundamentally all makes sense to me, and seems to work in my testing; however, I'd like @mhsmith to give it a quick once over to make sure I haven't missed anything subtle in the consequences of an Android update.
I'm also wondering if there's anything we can do around the cosmetics of the "existing user upgrade" process. I'm not overly concerned about having multiple versions of the SDK tools in the cache directory; however, having a "latest" folder that clearly isn't the latest any more seems like bad housekeeping (and potentially something that could bite us in future).
It's also a minor annoyance that the user-facing message is "couldn't find SDK", rather than "outdated SDK found". If you weren't expecting a download, you might interpret this as an indication that you've screwed up something and you've lost your old Briefcase configuration. Looking forward, I'm guessing that when we bump SDK_MANAGER_VER to "10.0" (or whatever), the same issue will arise - it will be identified as "not found", not "outdated".
- If A "latest" directory and "8092744" marker file exist, mark the install as an upgrade
- Maintain a list of "historical" SDK manager versions (currently empty) that, if a folder of that name exists in the cmdlinetools folder, causes theinstall to be marked as an upgrade
In case (2), all the "upgrade" flag does is modify the message; it doesn't do any cleanup of existing versions.
It might be worth doing a cleanup of case (1) to avoid having a latest folder that isn't the lastest version. My only hesitation on this is the churn for any user who is moving between old and new versions of briefcase. I'm not sure that's enough of a concern to be worried about, though.
Great feedback; thanks. I had some similar thoughts once I was going through some of the workflows.
There's currently a divergence for how this "upgrade" is handled for a Briefcase-managed SDK vs a User-managed SDK. Instead, if I just use the same method to install a specific version of the Command-line Tools for the Briefcase-managed SDK, then it won't look like fresh install and create that confusion. It will also leave the current install codepath to only run for an actually fresh install.
Yeah; that's true...but even if someone is switching that much, the download isn't that large....and if they really want to, they could set up separate SDK installs for their purposes. So, I'm thinking I'll just delete the |
Realized there's a bit a bootstrapping problem with my idea. If I'm planning to delete the There is another option I was considering. In both these cases, Briefcase-managed and user-managed, we're depending on some hopefully existing state of the SDK to perform the upgrade. Alternatively, we could just download the |
Here's what I think will help simplify this and create the best experience:
"Looking like an Android SDK" is defined as containing at least one of the Taking a closer look at what Android Studio does (as well as some of its source), it definitely appears to just download the
Given this, I feel mostly comfortable with doing this procedure even for User-managed SDKs as well as the Briefcase-managed SDK. |
That all sounds reasonable. But what does "perform a new install" mean beyond just installing the command-line tools? |
It really only changes what the user is told about what's happening. If no SDK exists, then the user is told "Android SDK not found; downloading and installing." Alternatively, if parts of the SDK already exist, they'll be told "Installing required SDK packages"....which would just be the specific version of
Unfortunately, though, this isn't proving entirely true. If you just download the ZIP file and unpack it in to an SDK that managed by Android Studio...Android Studio won't actually register the install in its UI. In the very least, when you run Therefore, I don't think it's really safe to modify an externally-managed SDK without using However, we're back to the bootstrapping problem: if the I'm thinking now it is safest to simply always download and unpack the needed version of |
This makes at least some sense; if the tools are downloaded in to
That definitely make sense for the Briefcase-managed case; however, I'm not sure how it helps with the externally managed case. Unless Studio consistently produces a I guess the good news is that the directory structure is just version numbers, so it's not too hard to discover the highest version number - but I'm not sure I'm wild about that as an approach. |
If one downloads and installs Android Studio today, it doesn't install any version of
I don't think I'm following...I'm simply proposing that Briefcase keeps a copy of the Does that make sense? If Briefcase has a reliable copy of I'm proposing this for 3 goals:
|
Oh...in case I haven't brought this up... The
|
How does an Android SDK user get a command line sdkmanager? It might not be installed by default, but my understanding was that it was selectable from the in-Studio GUI for package management.
Is it going to be safe for a Briefcase-managed SDK instance to alter a non-briefcase managed SDK install? It seems like asking for trouble to me... For my money, supporting To that end, I'd suggest the following behavior:
It might be possible to extend (2.ii) to include a couple of other likely candidates (e.g., looking for |
It is an optional selection within the Studio's Android SDK management UI.
I don't really see any reason to think this would be a problem;
This is basically what my previous commit is; I'm fine with this approach. Furthermore, though, I think we should get away from unpacking the cmdline-tools zip file in to the Briefcase-managed SDK. It is quite clear to me at this point this isn't really considered a valid installation by the Command-line Tools themselves. For instance, if you run |
- Previously, Android's Command-line Tools were installed into `cmdline-tools/latest` by Briefcase. - However, the SDK supports a versioned layout for installed packages. Therefore, the Command-line Tools are now installed in to `cmdline-tools/9.0` in the Briefcase data directory. - This also ensures that user-provided SDKs are providing the same version of Command-line Tools as Briefcase installs.
bc57925
to
d49d541
Compare
d49d541
to
9af3b8b
Compare
This is implemented now. The only deviation is notifying the user about Additionally, though, I guess I had never actually really paid attention to the build output:
But...now I know where the This also made me realize what's installing Is this all controlled by the Briefcase android template? or something else? [edit] apply plugin: 'com.android.application'
apply plugin: 'com.chaquo.python' |
The only deviation is notifying the user about I know it doesn't impact on usage - my argument for the warning was that if the user can end up with a "latest" folder that isn't actually the latest, then that's a foot-gun waiting to go off. However, it's also a user-managed SDK install, and we're using the factory-provided "upgrade" mechanism, so if it doesn't do the right thing, I'm also happy to put this in the "you said you want to manage your SDK install - so manage it" basket. |
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.
One clarification, and a couple of cosmetic cleanups; but I we're getting close.
of the Android SDK instance such that | ||
|
||
${sdk_env_source}/cmdline-tools/latest/bin/sdkmanager | ||
${sdk_source_env}/cmdline-tools/{cls.SDK_MANAGER_VER}/bin/sdkmanager |
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.
Minor nit - this instruction won't be right on Windows, because of the .bat extension. Might as well clean this up while we're in the area.
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.
Good catch; updated to use the existing methods to get the path for sdkmanager
.
tools.logger.info( | ||
"To use an existing Android SDK instance, specify its root " | ||
"directory path in the ANDROID_HOME environment variable." | ||
) |
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.
IMHO, if someone already has a working Briefcase install and is upgrading, there's no need to let them know that they can use an existing Android SDK instance. I think we can bury the "to use an existing" warning unless we're doing a first install.
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.
That makes sense. Details about ANDROID_HOME
are only shown now for a new install.
I'm still not following, though; this seems to be the nature of the beast that Google has created. Whatever is in the TBH, I don't know why Google has this designation of Do I have the wrong understanding of the Android SDK tools? I just feel like this would be a strange warning given my understanding of |
It's entirely possible I have the wrong understanding. I know that when I set up the cmdline-tools (the v6 version we're purging here), it installed into I assumed that Either way - I think it's a moot point. We're definitely cleaning up |
2d0603c
to
7a2bab0
Compare
This is still Google's recommendation....although, given that
As best I can tell,
I would also like to manage the other tools that are installed...that is, the However, it seems this will require much more cooperation with what the APK build process does....since that's pretty opaque to me, I'm not sure how well that would all work out. |
👍
Conceptually, I'm on board with this as an idea; with the obligatory "but you gotta make it work first" caveats :-) |
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.
One cosmetic tweak to an uninstall message, but otherwise, this looks really good. Thanks for your persistence with wrangling Android (and myself) through this.
Changes
cmdline-tools/latest
by Briefcase.cmdline-tools/9.0
in the Briefcase data directory.latest
version of Command-line Tools.latest
version isn't installed, the user is warned and the Briefcase-managed SDK is used.latest
Notes
PR Checklist: