Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Increase matrix of supported Android CLT and Java versions #74

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bartekpacia
Copy link
Contributor

@bartekpacia bartekpacia commented Aug 20, 2024

resolve #73

discovered during process of working on this PR - cirruslabs/cirrus-ci-docs#1288

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 21, 2024

Ughhh... what? :(

@fkorotkov @edigaryev

Screenshot 2024-08-21 at 01 06 41

@fkorotkov
Copy link
Contributor

@bartekpacia sorry about that. Unblocked your account. Your first iteration looked too suspicious to our crypto mining detection mechanism.

@edigaryev
Copy link

Do I understand correctly that this won't produce any new container images and/or tags? Is this intended?

@bartekpacia
Copy link
Contributor Author

This is still WIP, sorry!

@bartekpacia bartekpacia marked this pull request as draft August 21, 2024 18:23
@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 21, 2024

@fkorotkov Smoke Test is failing. I guess it's because it tries to build sdk/tools/Dockerfile, which now requries 2 arguments, but those args aren't provided. How can I provide those 2 args to the implicit docker_builder building that image?

@edigaryev
Copy link

How can I provide those 2 args to the implicit docker_builder building that image?

Docker ARG's instruction can accept default values.

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 22, 2024

@edigaryev @fkorotkov I think I got tools Dockerfile matrix done. Now, how should I go about making 34 and 34-ndk dockerfile also have that matrix?

Do I need to create new directories for each combination?

Or, should I parametrize the FROM instruction here:

FROM ghcr.io/cirruslabs/android-sdk:tools

to be like this:

ARG jdk_version=17

FROM ghcr.io/cirruslabs/android-sdk:tools-${jdk_version}

@bartekpacia bartekpacia marked this pull request as ready for review August 26, 2024 16:42
@bartekpacia
Copy link
Contributor Author

ping @fkorotkov :)

Copy link
Contributor

@fkorotkov fkorotkov left a comment

Choose a reason for hiding this comment

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

Should we do the same for ghcr.io/cirruslabs/android-sdk? Have variants for it's FROM?

@bartekpacia
Copy link
Contributor Author

@fkorotkov I'm not sure. Your call!

The end result I'd like to see is to have each of the 3 different images in this repo (tools, 34, and 34-ndk), to have 4 JDK version variants.

So that'd give 12 images per release (3 images * 4 variants).

@fkorotkov
Copy link
Contributor

I think we need to add the arguments to sdk/34/Dockerfile too. Lastly to preserve backward compatibility ghcr.io/cirruslabs/android-sdk:tools-jdk17 should be tagged as ghcr.io/cirruslabs/android-sdk:tools-jdk17. Alternatively I don't mind if we'll just build ghcr.io/cirruslabs/android-sdk:tools with the defaults from the arguments.

@bartekpacia
Copy link
Contributor Author

bartekpacia commented Aug 30, 2024

@fkorotkov Gotcha. Please see my latest commit, I hope I got it right.

Re: Java version - I think that latest JDK LTS should be the default. See #71. That said I'm OK with leaving openjdk17 as default.

@bartekpacia bartekpacia requested a review from fkorotkov August 30, 2024 15:44
@bartekpacia
Copy link
Contributor Author

Hm, it looks like we're hitting this problem: docker buildx fails to show result in image list

The answer suggests to add --output type=docker argument, but then we hit docker/buildx#59

@bartekpacia
Copy link
Contributor Author

ping @fkorotkov, any ideas here?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add more dimensions, such as JDK and Android OS system image
3 participants