Audit static field usage in Xamarin.Android.Build.Tasks #5996
Assignees
Comments
grendello
added a commit
to grendello/xamarin-android
that referenced
this issue
Jul 14, 2021
Context: dotnet#5996 Context: dotnet#5964 Context: dotnet#5964 (comment) The `NdkUtils` class used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, has grown increasingly complicated over the years due to a number of incompatibilities between various versions of the NDK. The code became hard to follow and untidy. This commit attempts to address the issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that any task using it had to do it. `NdkTools` doesn't require such initialization, instead it provides a factory method called `Create` which takes path to the NDK as its parameter and returns an instance of `NdkTools` child class (or `null` if an error occurs) which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hierarchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR dotnet#6073 which will be merged once r23 is out of beta.
grendello
added a commit
to grendello/xamarin-android
that referenced
this issue
Jul 14, 2021
Context: dotnet#5996 Context: dotnet#5964 Context: dotnet#5964 (comment) The `NdkUtils` class used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, has grown increasingly complicated over the years due to a number of incompatibilities between various versions of the NDK. The code became hard to follow and untidy. This commit attempts to address the issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that any task using it had to do it. `NdkTools` doesn't require such initialization, instead it provides a factory method called `Create` which takes path to the NDK as its parameter and returns an instance of `NdkTools` child class (or `null` if an error occurs) which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hierarchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR dotnet#6073 which will be merged once r23 is out of beta.
grendello
added a commit
to grendello/xamarin-android
that referenced
this issue
Jul 14, 2021
Context: dotnet#5996 Context: dotnet#5964 Context: dotnet#5964 (comment) The `NdkUtils` class used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, has grown increasingly complicated over the years due to a number of incompatibilities between various versions of the NDK. The code became hard to follow and untidy. This commit attempts to address the issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that any task using it had to do it. `NdkTools` doesn't require such initialization, instead it provides a factory method called `Create` which takes path to the NDK as its parameter and returns an instance of `NdkTools` child class (or `null` if an error occurs) which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hierarchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR dotnet#6073 which will be merged once r23 is out of beta.
grendello
added a commit
to grendello/xamarin-android
that referenced
this issue
Jul 14, 2021
Context: dotnet#5996 Context: dotnet#5964 Context: dotnet#5964 (comment) The `NdkUtils` class used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, has grown increasingly complicated over the years due to a number of incompatibilities between various versions of the NDK. The code became hard to follow and untidy. This commit attempts to address the issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that any task using it had to do it. `NdkTools` doesn't require such initialization, instead it provides a factory method called `Create` which takes path to the NDK as its parameter and returns an instance of `NdkTools` child class (or `null` if an error occurs) which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hierarchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR dotnet#6073 which will be merged once r23 is out of beta.
jonpryor
pushed a commit
that referenced
this issue
Jul 15, 2021
Context: #5964 Context: #5964 (comment) Context: #5996 Context: #6073 The `NdkUtils` class is used by Xamarin.Andrid.Build.Tasks to find tooling shipped with the Android NDK, and has grown increasingly complicated over the years due to a number of incompatibilities between various NDK versions. The code became hard to follow and untidy. Address this issue by replacing the single static `NdkUtils` class with a hierarchy of dynamically instantiated classes rooted in a new base class, `NdkTools`. `NdkUtils` had to be initialized for each thread that needed to access its methods, which led to various issues with concurrency and lack of proper initialization since the initialization had to be done wherever `NdkUtils` was first accessed, meaning that every task using it had to do it; see accc846 & [this comment][0]. `NdkTools` doesn't require such initialization, instead it provides an `NdkTools.Create()` factory method takes the path to the NDK and returns an instance of an `NdkTools` subclass (or `null` if an error occurs), which the can be safely used by the caller. Callers need not concern themselves with what is the actual type of the returned instance, they access only methods and properties defined in the `NdkTools` base abstract class. The hierarchy of `NdkTools` derivatives is structured and named after the breaking changes in the NDK. For instance, NDK versions before 16 used the GNU compilers, while release 16 and above use the clang compilers - this is reflected in existence of two classes derived from `NdkTools`, `NoClang` for NDKs older than r16 and `WithClang` for the newer ones. The other breaking changes are the addition of unified headers in r19, removal of the `platforms` directory in r22 and removal of GNU Binutils in r23. NDK r23 is recognized in this commit but it is NOT supported. Support for r23 is being worked on in PR #6073, which will be merged once r23 is out of beta. [0]: #5964 (comment)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Related: #5964 (comment)
Question: How "safe" is it to have mutable (non-
const, non-readonly)staticfields inXamarin.Android.Build.Tasks.dllwhich are used by MSBuild tasks?The primary cause of this question is from investigating Issue #5964, in which the (thread-static!) mutable static field
NdkUtil.usingClangNDKwas inconsistently initialized, resulting in "not entirely obvious" runtime behavior.…and it occurs to me that, in the context of MSBuild, I'm not sure how safe it is to have mutable static fields in the first place! Continuing with
NdkUtil.usingClangNDK, it's set based on the NDK version, as determined by$(AndroidNdkDirectory)/etc. For a given build, this won't change, but within the IDE, it very well could change, e.g. by the customer changing the Android SDK directory.It "feels" very "dubious" to have potentially changeable information stored in
staticfields.The "obvious" replacement? Instance fields in new classes which can be cached in e.g.
IBuildEngine4.GetRegisteredTaskObject(), which -- to my mind, anyway -- allows understanding of the relation of the data to the overall build process.There are (currently) 27 fields that are
staticmutable fields:Note that many of these don't need to be mutable!
ZipFlushSizeLimitshould beconst,validIdentifierandAndroidXmlNamespaceshould bereadonly, etc.MonoAndroidHelper.SupportedVersionsandMonoAndroidHelper.AndroidSdkappear to be "obvious" candidates for a more "Task-oriented"IBuildEngine4.GetRegisteredTaskObject()caching strategy, as well asNdkUtilin general.The text was updated successfully, but these errors were encountered: