-
Notifications
You must be signed in to change notification settings - Fork 533
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
[build] Reimplement NdkUtils #6083
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
grendello
requested review from
dellis1972,
pjcollins,
jonathanpeppers and
jonpryor
July 12, 2021 14:42
grendello
force-pushed
the
ndkutils-rework
branch
2 times, most recently
from
July 13, 2021 16:43
e554928
to
c145f56
Compare
jonpryor
reviewed
Jul 13, 2021
src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/NdkToolKind.cs
Outdated
Show resolved
Hide resolved
grendello
force-pushed
the
ndkutils-rework
branch
2 times, most recently
from
July 14, 2021 07:05
1441c63
to
8386aca
Compare
grendello
force-pushed
the
ndkutils-rework
branch
from
July 14, 2021 07:53
8386aca
to
c90586c
Compare
Context: #5996 |
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
jonpryor
reviewed
Jul 14, 2021
src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClang.cs
Outdated
Show resolved
Hide resolved
jonpryor
reviewed
Jul 14, 2021
src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoPlatforms.cs
Outdated
Show resolved
Hide resolved
jonpryor
reviewed
Jul 14, 2021
src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoPlatforms.cs
Outdated
Show resolved
Hide resolved
grendello
force-pushed
the
ndkutils-rework
branch
from
July 14, 2021 16:58
c90586c
to
82bfefe
Compare
jonpryor
reviewed
Jul 14, 2021
src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangWithPlatforms.cs
Outdated
Show resolved
Hide resolved
grendello
force-pushed
the
ndkutils-rework
branch
2 times, most recently
from
July 14, 2021 20:43
a778817
to
a07e025
Compare
jonpryor
reviewed
Jul 14, 2021
src/Xamarin.Android.Build.Tasks/Utilities/NdkTools/WithClangNoBinutils.cs
Show resolved
Hide resolved
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
force-pushed
the
ndkutils-rework
branch
from
July 14, 2021 21:05
a07e025
to
1982999
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context: #5996
Context: #5964
Context: #5964 (comment)
The
NdkUtils
class used by Xamarin.Andrid.Build.Tasks to find toolingshipped 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 accessits 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 doit.
NdkTools
doesn't require such initialization, instead it provides afactory method called
Create
which takes path to the NDK as itsparameter and returns an instance of
NdkTools
child class (ornull
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 afterthe 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 andWithClang
for thenewer ones. The other breaking changes are the addition of unified
headers in r19, removal of the
platforms
directory in r22 and removalof 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.