Fixed
Status Update
Comments
uc...@google.com <uc...@google.com>
xa...@google.com <xa...@google.com>
jo...@google.com <jo...@google.com> #2
Proof of concept demo on Github: https://github.com/casperbang/MissingNonLocalizedDrawableCrash
No problem compiling, lint'ing or running the app on a Danish device. Crash when running on a device set to any other locale.
No problem compiling, lint'ing or running the app on a Danish device. Crash when running on a device set to any other locale.
rc...@gmail.com <rc...@gmail.com> #3
I should probably generalize the translation detector's extra strings check, which is exactly this issue (but currently limited to strings checks).
rc...@gmail.com <rc...@gmail.com> #4
Finally got to this one. This is implemented for 3.2 canary 7 (along with a bunch of other fixes; it can now run the translation checks on the fly in the IDE; there are some quickfixes, etc.)
Here's what I get on the sample project:
> Task :app:lintDebug
/home/tnorbye/AndroidStudioProjects/bugs/MissingNonLocalizedDrawableCrash/app/src/main/res/drawable-da-xxxhdpi/sticker_ci.png: Error: The drawable "sticker_ci" in drawable-da-xxxhdpi has no declaration in the base drawable folder or in a drawable-densitydpi folder; this can lead to crashes when the drawable is queried in a configuration that does not match this qualifier [MissingDefaultResource]
Explanation for issues of type "MissingDefaultResource":
If a resource is only defined in folders with qualifiers like -land or -en,
and there is no default declaration in the base folder (layout or values
etc), then the app will crash if that resource is accessed on a device
where the device is in a configuration missing the given qualifier.
As a special case, drawables do not have to be specified in the base
folder; if there is a match in a density folder (such as drawable-mdpi)
that image will be used and scaled. Note however that if you only specify
a drawable in a folder like drawable-en-hdpi, the app will crash in
non-English locales.
There may be scenarios where you have a resource, such as a -fr drawable,
which is only referenced from some other resource with the same qualifiers
(such as a -fr style), which itself has safe fallbacks. However, this still
makes it possible for somebody to accidentally reference the drawable and
crash, so it is safer to create a default dummy fallback in the base
folder. Alternatively, you can suppress the issue by adding
tools:ignore="MissingDefaultResource" on the element.
(This scenario frequently happens with string translations, where you might
delete code and the corresponding resources, but forget to delete a
translation. There is a dedicated issue id for that scenario, with the id
ExtraTranslation.)
1 errors, 0 warnings
Thanks for the report!
Here's what I get on the sample project:
> Task :app:lintDebug
/home/tnorbye/AndroidStudioProjects/bugs/MissingNonLocalizedDrawableCrash/app/src/main/res/drawable-da-xxxhdpi/sticker_ci.png: Error: The drawable "sticker_ci" in drawable-da-xxxhdpi has no declaration in the base drawable folder or in a drawable-densitydpi folder; this can lead to crashes when the drawable is queried in a configuration that does not match this qualifier [MissingDefaultResource]
Explanation for issues of type "MissingDefaultResource":
If a resource is only defined in folders with qualifiers like -land or -en,
and there is no default declaration in the base folder (layout or values
etc), then the app will crash if that resource is accessed on a device
where the device is in a configuration missing the given qualifier.
As a special case, drawables do not have to be specified in the base
folder; if there is a match in a density folder (such as drawable-mdpi)
that image will be used and scaled. Note however that if you only specify
a drawable in a folder like drawable-en-hdpi, the app will crash in
non-English locales.
There may be scenarios where you have a resource, such as a -fr drawable,
which is only referenced from some other resource with the same qualifiers
(such as a -fr style), which itself has safe fallbacks. However, this still
makes it possible for somebody to accidentally reference the drawable and
crash, so it is safer to create a default dummy fallback in the base
folder. Alternatively, you can suppress the issue by adding
tools:ignore="MissingDefaultResource" on the element.
(This scenario frequently happens with string translations, where you might
delete code and the corresponding resources, but forget to delete a
translation. There is a dedicated issue id for that scenario, with the id
ExtraTranslation.)
1 errors, 0 warnings
Thanks for the report!
jo...@google.com <jo...@google.com> #5
Hey Robert,
From what you wrote I think this is intended behavior. If you change the CMake script then the CMake-generated Ninja project must be regenerated. Since there's a new Ninja project there needs to be a full build. There's no concept of delta build that survives changes to build command files (CMakeLists.txt or ninja build files) because any behavior difference is possible.
From what you wrote I think this is intended behavior. If you change the CMake script then the CMake-generated Ninja project must be regenerated. Since there's a new Ninja project there needs to be a full build. There's no concept of delta build that survives changes to build command files (CMakeLists.txt or ninja build files) because any behavior difference is possible.
rc...@gmail.com <rc...@gmail.com> #6
Most of the time, a CMakeLists.txt is modified to add, remove, or rename source files. CMake uses this list of source files as a way to know if it should regenerate to update its ninja scripts to build the new files, if new files were added.
If you do a traditional CMake build from the command line, CMake will regenerate but it will not clean your previously compiled translation units (the object files). Those remain in-place. This allows you to perform another build (e.g. execute "ninja" command) and the build tool and compiler working together will be smart enough to know which files need to be rebuilt.
From this standpoint, I disagree with your assertions. There's no functional reason AFAIC to "clean" build every time a CMake script changes. If command line parameters to CMake change in such a way that it would completely invalidate previously compiled translation units outside of the knowledge of ninja and the toolchain programs, such as changing the Android ABI via toolchain arguments, then yes you would need to clean everything and rebuild. However, beyond that, you should be relying on ninja and the compiler to perform delta builds on previously compiled translation units.
As you have it now, I am unable to do a simple task like add new CPP files without having to rebuild my entire project. This wastes about 15-20 minutes of my time (that's how long a full rebuild takes), versus less than 60 seconds to just build the new file.
I think you should aim for smarter introspection into how this mechanism works. I see two very simple scenarios:
1) A build & sync was triggered due to parameter changes from gradle (those that affect the externalNativeBuild configuration, such as native ABI changes and custom user arguments)
2) A build was triggered because CMake.exe told you that a regeneration was needed since it detected a CMakeLists.txt changed
#1 should perform a clean & rebuild
#2 should not require any special action from you; you should simply run "ninja" command as if the scripts hadn't changed at all, and let Ninja run CMake to regenerate as needed
I feel like #2 is being treated as #1 right now. Also I noticed that Android Studio wants to re-sync Gradle when a CMakeLists.txt file changed. I do not think Android Studio should be "watching" CMake scripts. It should only resync if gradle scripts changed. If a CMake script changes, let your "ninja" command handle it.
As a final note, I realize I'm probably oversimplifying this a little. I'm sure there's reasons and edge cases as to why you need that level of control over CMake. However, at some point the full clean build has to be triggered in a smarter way to avoid these giant productivity problems. You obviously know much more about how CMake integrates with Android Studio than I do; I can only offer feedback based on my direct usage of CMake outside of AS. However, I hope my feedback makes sense and is agreeable.
If you do a traditional CMake build from the command line, CMake will regenerate but it will not clean your previously compiled translation units (the object files). Those remain in-place. This allows you to perform another build (e.g. execute "ninja" command) and the build tool and compiler working together will be smart enough to know which files need to be rebuilt.
From this standpoint, I disagree with your assertions. There's no functional reason AFAIC to "clean" build every time a CMake script changes. If command line parameters to CMake change in such a way that it would completely invalidate previously compiled translation units outside of the knowledge of ninja and the toolchain programs, such as changing the Android ABI via toolchain arguments, then yes you would need to clean everything and rebuild. However, beyond that, you should be relying on ninja and the compiler to perform delta builds on previously compiled translation units.
As you have it now, I am unable to do a simple task like add new CPP files without having to rebuild my entire project. This wastes about 15-20 minutes of my time (that's how long a full rebuild takes), versus less than 60 seconds to just build the new file.
I think you should aim for smarter introspection into how this mechanism works. I see two very simple scenarios:
1) A build & sync was triggered due to parameter changes from gradle (those that affect the externalNativeBuild configuration, such as native ABI changes and custom user arguments)
2) A build was triggered because CMake.exe told you that a regeneration was needed since it detected a CMakeLists.txt changed
#1 should perform a clean & rebuild
#2 should not require any special action from you; you should simply run "ninja" command as if the scripts hadn't changed at all, and let Ninja run CMake to regenerate as needed
I feel like #2 is being treated as #1 right now. Also I noticed that Android Studio wants to re-sync Gradle when a CMakeLists.txt file changed. I do not think Android Studio should be "watching" CMake scripts. It should only resync if gradle scripts changed. If a CMake script changes, let your "ninja" command handle it.
As a final note, I realize I'm probably oversimplifying this a little. I'm sure there's reasons and edge cases as to why you need that level of control over CMake. However, at some point the full clean build has to be triggered in a smarter way to avoid these giant productivity problems. You obviously know much more about how CMake integrates with Android Studio than I do; I can only offer feedback based on my direct usage of CMake outside of AS. However, I hope my feedback makes sense and is agreeable.
jo...@google.com <jo...@google.com>
rc...@gmail.com <rc...@gmail.com> #7
Just to clarify, in the logs this is the exact problem:
External native generate JSON x86Debug: - dependent build file missing or changed
External native generate JSON x86Debug: removing stale contents from 'E:\code\frontend\source\Applications\zPayService\TestTool\.externalNativeBuild\cmake\x86Debug\x86'
The "removing stale contents"... those contents aren't stale at all. A build could be executed that reuses those existing object files if all I did was modify the cmake script to add new files to compile.
External native generate JSON x86Debug: - dependent build file missing or changed
External native generate JSON x86Debug: removing stale contents from 'E:\code\frontend\source\Applications\zPayService\TestTool\.externalNativeBuild\cmake\x86Debug\x86'
The "removing stale contents"... those contents aren't stale at all. A build could be executed that reuses those existing object files if all I did was modify the cmake script to add new files to compile.
rc...@gmail.com <rc...@gmail.com> #8
Also CMake will think CMake scripts changed if you swap between git branches:
1. Go from branch A to branch B
2. Go from branch B back to A
Cmake script is still physically the same, but because you swapped branches around, the timestamp of the Cmake script changed so this triggers a full rebuild.
At this point I'd say this is our biggest issue at Ziosk and the one causing the most productivity problems. Our team of about 15 engineers is constantly complaining. There's concern on how long Google will take to address these issues, some have proposed rolling back to Eclipse + ANT. If we're a year out from any improvements I may have to consider it.
1. Go from branch A to branch B
2. Go from branch B back to A
Cmake script is still physically the same, but because you swapped branches around, the timestamp of the Cmake script changed so this triggers a full rebuild.
At this point I'd say this is our biggest issue at Ziosk and the one causing the most productivity problems. Our team of about 15 engineers is constantly complaining. There's concern on how long Google will take to address these issues, some have proposed rolling back to Eclipse + ANT. If we're a year out from any improvements I may have to consider it.
jo...@google.com <jo...@google.com> #9
I'm prototyping a change to address this. There is a concern that CMake doesn't operate incrementally under certain circumstances. For example, changing the compiler. I think Refresh Linked C++ Projects should be enough to cover this case. I'm still looking for other cases that would be more concerning
jo...@google.com <jo...@google.com> #10
Another kind of change to CMakeLists.txt is affected. If a build target is removed then CMake won't delete the .so and related .o files. Gradle packages *.so rather than the explicit list of .so files because build systems sometimes copy extra .so files into the output folder to be packaged.
I think I can cover this case by specific removing only unrecognized .so files during sync.
I think I can cover this case by specific removing only unrecognized .so files during sync.
rc...@gmail.com <rc...@gmail.com> #11
To your example: Changing compiler means changing command line arguments (e.g. `-G Ninja`) or alternatively, changing environment such as C and CXX variables. In the latter case, normal CMake does not watch for changes to environment variables, so it's the responsibility of the user (or in your case, the IDE) to monitor changes to environment and control CMake accordingly.
However, this still falls into the pattern of behavior I've outlined previously. Indirect or direct changes to Gradle or Gradle scripts that result in a different command line invocation of CMake should result in a full clean (delete binary dir + regenerate using CMake). Changes to the CMake scripts themselves shouldn't be monitored by Android Studio at all. If you have weird cases where CMake is doing work the NDK toolchain should be doing, such as lots of system introspection and runtime changes to toolchain, I'd simply document that Google does not support that usage of CMake, and all changes should go through Gradle instead (or file a bug with the NDK team and have them fix the toolchain file).
However, this still falls into the pattern of behavior I've outlined previously. Indirect or direct changes to Gradle or Gradle scripts that result in a different command line invocation of CMake should result in a full clean (delete binary dir + regenerate using CMake). Changes to the CMake scripts themselves shouldn't be monitored by Android Studio at all. If you have weird cases where CMake is doing work the NDK toolchain should be doing, such as lots of system introspection and runtime changes to toolchain, I'd simply document that Google does not support that usage of CMake, and all changes should go through Gradle instead (or file a bug with the NDK team and have them fix the toolchain file).
rc...@gmail.com <rc...@gmail.com> #12
Concerning stale *.so files: What I did for my ANT integration custom targets in CMake was to delete all *.so files before running a build, that way when a build is finished, you're left with only *.so files that were copied to that destination by custom commands tied to real targets. So if a target was completely removed, its corresponding *.so file would not make it to the temporary/intermediate copy destination where you look for *.so files to package into the APK.
This solution is simple and doesn't require introspection into CMake targets. If you have more direct access to targets defined by CMake, you could keep track a list of targets found during configuration and query the OUTPUT property on each of them and correlate that to files you find in your CMake output directory. Not sure if this has much benefit over the idea above, though.
This solution is simple and doesn't require introspection into CMake targets. If you have more direct access to targets defined by CMake, you could keep track a list of targets found during configuration and query the OUTPUT property on each of them and correlate that to files you find in your CMake output directory. Not sure if this has much benefit over the idea above, though.
jo...@google.com <jo...@google.com> #13
I've submitted a fix for this. Unless we find a problem the fix will appear in the next 3.1 canary
rc...@gmail.com <rc...@gmail.com> #14
Could you explain the fix here so I (and others) can get a good understanding of how the behavior is changing as well as what the root cause (original behavior) was?
Description
I'd like to suggest that the gradle sync be a little more intelligent: It should only delete the CMake binary directories *if and only if* parameters change in Gradle that cause cmake toolchain parameters to change (such as platform version, abi, toolchain) or parameters passed to CMake to change.
Deleting the binary directory unnecessarily creates unnecessary rebuilds of native code.
Build: 3.1 Canary 3, AI-171.4435470, 201711061907,
AI-171.4435470, JRE 1.8.0_152-release-1012-b01x64 JetBrains s.r.o, OS Windows 10(amd64) v10.0 , screens 1920x1080, 1920x1080
IMPORTANT: Please read