-
-
Notifications
You must be signed in to change notification settings - Fork 912
fix: Specify build tools version for :api #2673
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
base: compose-dev
Are you sure you want to change the base?
Conversation
This PR should be merged immediately when approved. |
What is the default one |
The default was build tool 35.0.0 (The default value is grabbed from the AGP 8.9.1, however as of right now the value is still the same with the latest version) |
I don't think we should hardcode. AGP should update to it |
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.
I just realized something. Is there a reason why this is not done for app
as well?
That's because :app already have specified build tools |
AGP default (likely well tested in this case) hold the same weight as AGP minimum, so it should be fine to bump it. https://developer.android.com/build/releases/gradle-plugin#compatibility |
Didn't realize that. I probably would have checked if I had access to a computer. |
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.
Approved unless the issue can be solved by bumping AGP.
Latest AGP still has 35.0.0 as default: |
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 PR should not be merged. Instead you should wait for AGP to update to the fixed build tools version, because this PR adds unnecessary maintenance burden (updating build tools manually) and doesn't even use the version catalog. This applies to the :app module
I've checked this, the AGP source does not set or dictate the specific dependency version needs to be used. If developers didn't specify which version to use, then AGP will get its default value, which by no means is the recommended version because there is none. However if they do the AGP will let us know by crashing the right away or put it in documentation: see: https://developer.android.com/build/releases/gradle-plugin#updating-gradle |
Why does agp not default to the latest version |
Library or dependency don't need to automatically update its default dependency or grab whatever latest from the server. That just wouldn't make sense in general. |
But Everytime agp is updated I'd suppose they update build tools too? |
They don't, 35.0.0 has been the default value since AGP 8.8.0. The latest AGP is 8.11, still version 35.0.0 is the default. |
That is odd. Why wouldn't they bump? |
That's the minimum version (which happens to also be the default/fallback version), AGP support build tools 35.0.0 and above, the minimum is updated on an ad-hoc basis. Version on build tools doesn't really matter much, because building a newer version of Android like Android 16 (36) works fine on older build tools like 35.0.0 |
The |
What I am saying applies to the app module as well. However as insignificant as it may look, it's an additional tech debt that you need to avoid if possible, and not just when it "looks significant". Setting the build tools version would require everyone to have that specific version too. Is that the case when unset? |
I don't see why would bumping build tools would be considered as technical debt to the project? This PR bump the default of 35.0.0 to 35.0.1, this isn't done randomly but to fix an issue, so we actually reduce a tech debt by not having to wait for AGP to bump the minimum/default version to 35.0.1 or make workaround for user whose the first character of the directory is a "u". But maybe when AGP finally does update the default/minimum version to 36.0.01, we can remove it from the build config. Footnotes
|
Because now you have to bump it from time to time manually.
Not doing something and waiting for something to happen is not a debt.
I personally would apply the ostrich algorithm and let time heal the wounds instead of casting an indefinite technical debt upon us. Is there a bump cycle? If it is not too long, then that's definitely the better course of action. |
Using ostrich to ignore critical problem should not be the reason why this PR can't be merged when the fix is just one line and does not violate any android development rule.
There is no specific bump cycle for default/minimum version of build tools. If we were to have educated guesses from last time the minimum was bump, it would take around 14 months for AGP to bump it (AGP 8.2.0 Nov 2023 -> 8.8.0 Jan 2025) (additionally I have checked the latest alphas and they're still on 35.0.0)
It is a debt because we didn't fix the issue that arises with using the default configuration.
I don't see why this would be a problem as this is equivalent to updating a dependency. |
In my opinion its a very niche problem and not critical that doesn't justify increasing tech debt.
Its not our problem to fix. AGP updates automatically which would fix the issue without any intervention. If you hardcode now, you cast tech debt upon yourself for a one time niche problem (which isn't even our problem)
Because its yet another hardcoded value in a random location you'll forget in two weeks. |
Use build tool 35.0.1 or above to stop build tool from generating AIDL file with unicode escape when the first character of the directory contain a "u"
See: https://issuetracker.google.com/issues/354735915
fixes #2672