[Xamarin.Android.Build.Tasks] Unable to build projects with lint checks enabled against 15.3 #649
Conversation
|
no idea why the linux build failed there? |
|
build |
| @@ -221,7 +227,10 @@ protected override string GenerateCommandLineCommands () | |||
|
|
|||
| protected override string GenerateFullPathToTool () | |||
| { | |||
| return Path.Combine (ToolPath, ToolExe); | |||
| var fullToolPath = Path.Combine (ToolPath, ToolExe);; | |||
|
|
||
| if (!string.IsNullOrEmpty (ToolPath) && ToolPath != null) { | ||
| if (!File.Exists (Path.Combine (ToolPath, ToolExe))) | ||
| ToolPath = Path.Combine (ToolPath, "bin"); |
There was a problem hiding this comment.
...and if $"{ToolPath}/bin/{ToolExe}" doesn't exist, what should happen?
Perhaps ToolTask reports a sane error if the file doesn't exist? But I don't think so; I think e.g. on macOS it turns into a "cryptic" error 127.
I think we should do our own error checking with a decent error message.
| var fullToolPath = Path.Combine (ToolPath, ToolExe);; | ||
| if (File.Exists (fullToolPath)) | ||
| return fullToolPath; | ||
| return Path.Combine (ToolPath, "bin", ToolExe); |
There was a problem hiding this comment.
This method seems weirdly duplicative of what's being added to Execute(). Is that duplication necessary?
There was a problem hiding this comment.
If the ToolPath is set to empty (by the user) this code path will get hit. So we kinda need the same code. I will move it into one method
| @@ -750,12 +750,6 @@ because xbuild doesn't support framework reference assemblies. | |||
| /> | |||
| </CreateProperty> | |||
|
|
|||
| <CreateProperty Value="$(_AndroidToolsDirectory)bin\" Condition="Exists ('$(_AndroidToolsDirectory)bin\')"> | |||
| <Output TaskParameter="Value" PropertyName="LintToolPath" | |||
There was a problem hiding this comment.
This and the commit message confuses me.
We're going to make $(LintToolPath) unset/empty by default, and add additional File.Exists() checking to the <Lint/> task...?
Why not instead update the <ResolveSdks/> Task to output a ResolveSdks.LintToolPath property, and let <ResolveSdks/> check for the appropriate directories?
| @@ -121,6 +124,7 @@ public bool RunTask () | |||
| .Select (e => e.Value) | |||
| .ToArray (); | |||
| AndroidSequencePointsMode = sdk.Element ("AndroidSequencePointsMode")?.Value ?? "None"; | |||
| LintToolPath = sdk.Element ("LintToolPath")?.Value ?? Path.Combine (AndroidSdkBuildToolsPath); | |||
There was a problem hiding this comment.
Why have Path.Combine (AndroidSdkBuildToolsPath)? You're not combining anything.
…ks enabled against 15.3 Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=57532 The Directory test from commit 9cf367f did not really work. This is because the 'bin' directory existed prior to 25.3.0. But the lint tool only moved in 25.3.0. To fix this we are adding new code to the ResolveSdks Task. This will find the correct location of the `lint` tool. It will also use the `$(LintToolPath)` the user provides. If the tool cannot be found an appropriate error message will be raised.
…ks enabled against 15.3 (#649) Fixes: https://bugzilla.xamarin.com/show_bug.cgi?id=57532 The Directory test from commit 9cf367f did not really work. This is because the 'bin' directory existed prior to 25.3.0. But the lint tool only moved in 25.3.0. To fix this we are adding new code to the ResolveSdks Task. This will find the correct location of the `lint` tool. It will also use the `$(LintToolPath)` the user provides. If the tool cannot be found an appropriate error message will be raised.
Fixes https://bugzilla.xamarin.com/show_bug.cgi?id=57532
The Directory test from commit 9cf367f did not really work.
This is because the 'bin' directory existed prior to 25.3.0.
But the lint tool only moved in 25.3.0.
To fix this we are adding new code to the ResolveSdks Task. This
will find the correct location of the
linttool. It will alsouse the
$(LintToolPath)the user provides.If the tool cannot be found an appropriate error message will
be raised.