search

polymonster / premake-android-studio

premake5 module for android-studio and gradle build.
MIT License
34 stars 15 forks source link

Some fixes to work with modern Gradle/Android #21

Closed tritao closed 4 months ago

tritao commented 5 months ago

Updated PR with the changes requested. Also added https://github.com/polymonster/premake-android-studio/pull/21/commits/5cc63edd9fee34a70e40caae0076f2b182e19d3f just to clean the log a bit. But happy to remove from the PR if you want to keep it.

polymonster commented 5 months ago

Thanks for the update, a few more things to chew on there. Hopefully you can still work with your changes locally and we can carefully move toward merging into main.

There aren’t really enough test cases in the CI here to move safely. so I am just picking through the changes to make them as low friction and as backwards compatible as possible to maintain any existing lua configs

tritao commented 5 months ago

The change looks good in principle and thanks for checking the gradle version thoroughly.

I am just wondering about some scenarios, and am not entirely sure what the package attribute does.

you mentioned the androidnamespace comment, so this package attribute is now replaced with the androidnamespace?

I am wondering what happens in the scenario where the package attribute is deprecated (but not removed) we omit this code, but the user does not supply an androidnamespace in the lua config.

I think one issue might exists if the user supplies a recent enough gradle version but does not provide the androidnamespace key. We can check for this case and provide a warning/error to the user. Let me know if that would work and I can add it.

Regarding the message verbosity issue, we can use verbosef, would that work?

There aren’t really enough test cases in the CI here to move safely. so I am just picking through the changes to make them as low friction and as backwards compatible as possible to maintain any existing lua configs

Aside from the CMake minimum version, which should not really be an issue (3.0 was released 10 years ago, but can add an option as you wish), the behaviour does not change for existing code. It only changes behaviour for recent Gradle Android versions (which is broken at the moment), so in theory it should only improve things and never break anything. I also tested and if you provide androidnamespace for old Gradle versions it still works, but I can remove the output for older Gradle versions, just to be even safer.

polymonster commented 5 months ago

Can we generate a default androidnamespace value for gradle versions that require it? Much like how the previous package attribute was derived by the project name?

verbosef sounds good for the log.

With regards to CMake version, I really think being able to specify it in the config is the best way to go, it provides more flexibility.

tritao commented 5 months ago

Can we generate a default androidnamespace value for gradle versions that require it? Much like how the previous package attribute was derived by the project name?

Sounds like a good solution, will try that.

verbosef sounds good for the log.

With regards to CMake version, I really think being able to specify it in the config is the best way to go, it provides more flexibility.

Alright, though that raises the question of the default value. Using 2.6 will still give trouble as I got an error with that version by default, without any changes to the generated CMake projects.

polymonster commented 4 months ago

Took these changes in the end, to fix windows build issues. seems like it's easiest just to upgrade the toolchain to gradle 8 at least now