-
Notifications
You must be signed in to change notification settings - Fork 564
[Xamarin.Android.Build.Tasks] Use Path.Combine() in <CalculateProjectDependencies/> #4681
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
Conversation
…Dependencies/> Fixes: dotnet#4677 `<CalculateProjectDependencies/>` was always using `/` as the path separator for the `build-tools` and `platforms` output items: Task "CalculateProjectDependencies" Task Parameter:TargetFrameworkVersion=v10.0 Task Parameter:ManifestFile=C:\Source\xamarin-android\bin\TestDebug\temp\InstallAndroidDependenciesTest\Project\Properties\AndroidManifest.xml Task Parameter:BuildToolsVersion=29.0.2 Task Parameter:PlatformToolsVersion=29.0.5 Task Parameter:ToolsVersion=26.1.1 Task Parameter:NdkVersion=16.1 Task Parameter:NdkRequired=False Output Item(s): AndroidDependency= platforms/android-29 build-tools/29.0.2 Version=29.0.2 platform-tools Version=29.0.5 tools Version=26.1.1 Done executing task "CalculateProjectDependencies". This meant the commercial `<InstallAndroidDependencies/>` task wouldn't ever install the `build-tools` or `platforms` components on Windows because none of the candidate `IAndroidComponent.FileSystemPath` values that were available via the SDK installer matched the input `ITaskItem.ItemSpec` values. Fix this by changing `<CalculateProjectDependencies/>` to use `Path.Combine()` instead of `/`. Test changes: `InstallAndroidDependenciesTest` did not yet enforce the expected behavior for this bug because `<ResolveSdks/>` was automatically falling back to the default Android SDK installation. Update `InstallAndroidDependenciesTest` to enforce the expected behavior. To minimize download requirements for the test, add a mock `tools/source.properties` file to the target `android-sdk` directory during each test run. This reduces the total downloads from about 1 GB to under 100 MB, and, because the `tools` component isn't needed for the current default project configuration, `b.Build (proj)` still completes successfully.
| var sdkVersion = Math.Max (targetApiLevel.Value, manifestApiLevel); | ||
| dependencies.Add (CreateAndroidDependency ($"platforms/android-{sdkVersion}", $"")); | ||
| dependencies.Add (CreateAndroidDependency ($"build-tools/{BuildToolsVersion}", BuildToolsVersion)); | ||
| dependencies.Add (CreateAndroidDependency (Path.Combine ("platforms", $"android-{sdkVersion}"), $"")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This must have changed behaviour in the sdk installer. I seem to remember the spec required the system to use / as a separator :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Relevant spec: https://microsoft-my.sharepoint.com/:w:/p/mhutch/ETjdNgT_yfJMszzuRRW2hUYB133Goo831A5U2rAFwgI8bA?e=77b68c4ccf5841139a242a0d99cd0368
Forward slash / is explicitly mentioned in the spec:
For items in the main Android repository manifest, the ID shall be the path formatted in *nix format, e.g. “platforms/android-25” or “ndk-bundle”. For add-ons, the ID shall be the add-on’s “name-id” property. We will also have our own manifest format where the fileSystemPath will be exposed directly.
We do not want to use Path.Combine() here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So in that case the issue must be in the task which calls the SDK Manager or the SDK Manager code itself I guess.
|
Interestingly on windows doing the following does not install anything
where the |
|
It works fine on a Mac. |
|
This was fixed in https://github.com/xamarin/android-sdk-installer/pull/452 so we can close this PR. |
Fixes: #4677
<CalculateProjectDependencies/>was always using/as the pathseparator for the
build-toolsandplatformsoutput items:This meant the commercial
<InstallAndroidDependencies/>task wouldn'tever install the
build-toolsorplatformscomponents on Windowsbecause none of the candidate
IAndroidComponent.FileSystemPathvaluesthat were available via the SDK installer matched the input
ITaskItem.ItemSpecvalues.Fix this by changing
<CalculateProjectDependencies/>to usePath.Combine()instead of/.Test changes:
InstallAndroidDependenciesTestdid not yet enforce the expectedbehavior for this bug because
<ResolveSdks/>was automatically fallingback to the default Android SDK installation.
Update
InstallAndroidDependenciesTestto enforce the expectedbehavior.
To minimize download requirements for the test, add a mock
tools/source.propertiesfile to the targetandroid-sdkdirectoryduring each test run. This reduces the total downloads from about 1 GB
to under 100 MB, and, because the
toolscomponent isn't needed for thecurrent default project configuration,
b.Build (proj)still completessuccessfully.