-
Notifications
You must be signed in to change notification settings - Fork 528
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
[Xamarin.Android.Build.Tasks] guard AutoImport.props
against empty values
#7837
Conversation
…values There is an email thread about ASP.NET projects in VS -- and something is potentially slowing down builds when optional workloads are installed. One concern is our `AutoImport.props`, take for instance the following `foo.targets` file: <Project> <ItemGroup> <AndroidResource Include="$(MonoAndroidResourcePrefix)\*\*.xml" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\*\*.axml" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\*\*.png" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\*\*.jpg" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\*\*.gif" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\*\*.webp" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\font\*.ttf" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\font\*.otf" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\font\*.ttc" /> <AndroidResource Include="$(MonoAndroidResourcePrefix)\raw\*" Exclude="$(MonoAndroidResourcePrefix)\raw\.*" /> <AndroidAsset Include="$(MonoAndroidAssetsPrefix)\**\*" Exclude="$(MonoAndroidAssetsPrefix)\**\.*\**" /> </ItemGroup> <Target Name="Build" /> </Project> In this case `$(MonoAndroidResourcePrefix)` and `$(MonoAndroidAssetsPrefix)` will be blank, and we get: msbuild foo.targets -bl ... MSBUILD : warning MSB5029: The value "\**\.*\**" of the "Exclude" attribute in element <ItemGroup> in file "foo.targets (13,61)" is a wildcard that results in enumerating all files on the drive, which was likely not intended. Check that referenced properties are always defined. 1 Warning(s) 0 Error(s) Time Elapsed 00:02:45.14 I'm not sure this is causing an actual problem, but I think it's a good idea to add some safety here. I will make a similar change in Xamarin.Legacy.Sdk.
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/Sdk/AutoImport.props
Outdated
Show resolved
Hide resolved
src/Xamarin.Android.Build.Tasks/Microsoft.Android.Sdk/Sdk/AutoImport.props
Show resolved
Hide resolved
@@ -35,8 +35,14 @@ https://github.com/dotnet/designs/blob/4703666296f5e59964961464c25807c727282cae/ | |||
<AndroidResource Include="$(MonoAndroidResourcePrefix)\font\*.otf" /> | |||
<AndroidResource Include="$(MonoAndroidResourcePrefix)\font\*.ttc" /> | |||
<AndroidResource Include="$(MonoAndroidResourcePrefix)\raw\*" Exclude="$(MonoAndroidResourcePrefix)\raw\.*" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup Condition=" '$(MonoAndroidAssetsPrefix)' != '' and '$(EnableDefaultAndroidItems)' == 'true' and $([MSBuild]::VersionEquals($(TargetFrameworkVersion), '8.0')) "> | |||
<!-- Default Asset file inclusion --> | |||
<AndroidAsset Include="$(MonoAndroidAssetsPrefix)\**\*" Exclude="$(MonoAndroidAssetsPrefix)\**\.*\**" /> |
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.
Similar to above, but…
Why support $(MonoAndroidAssetsPrefix)
at all anymore? Consider @(TransformFile)
, @(AndroidLibrary)
, @(AndroidJavaSource)
, etc., below: none of those use $(MonoAndroidAssetsPrefix)
, and thus aren't susceptible to pulling in C:\**\*
.
Alternatively, perhaps we could require/force $(MonoAndroidAssetsPrefix)
to end with "directory-separator-char", which would allow us to change these includes to e.g.:
<AndroidResource Include="$(MonoAndroidAssetsPrefix)**\*" />
which would also prevent us from trying to traverse C:\**\*
.
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.
After discussion and thinking about AutoImports.props
MSBuild ordering, we decided to just format my code better...
Spec if needed: https://github.com/dotnet/designs/blob/main/accepted/2020/workloads/workload-resolvers.md#workload-props-files
Context: dotnet/android#7837 Context: #46 I ended up making this change in xamarin-android to avoid a potential issue in all .NET projects, so bringing it here as well. In cases where the `android` workload is not installed, Xamarin.Legacy.Sdk can accidentally wildcard your entire disk: MSBUILD : warning MSB5029: The value “\**\.*\**” of the “Exclude” attribute in element <ItemGroup> in file “/Users/moljac/.nuget/packages/xamarin.legacy.sdk/0.2.0-alpha2/Sdk/AutoImport.Android.props (26,61)” is a wildcard that results in enumerating all files on the drive, which was likely not intended. Check that referenced properties are always defined. Add checks that `$(MonoAndroidAssetsPrefix)` and `$(MonoAndroidResourcePrefix)` are not blank.
* main: Bump to dotnet/installer@632ddca 8.0.100-preview.3.23128.1 (dotnet#7836) LEGO: Merge pull request 7852 [ci] Reduce overhead for MSBuildIntegration unit test jobs. (dotnet#7832) [ci] Allow dynamic`$(NuGetArtifactName)` values (dotnet#7848) [Xamarin.Android.Build.Tasks] guard `AutoImport.props` against empty values (dotnet#7837) [Mono.Android] Print type & member remapping info (dotnet#7844) [Mono.Android] Tweak AndroidMessageHandler behavior for WCF support (dotnet#7785) LEGO: Merge pull request 7845 Localized file check-in by OneLocBuild Task (dotnet#7842) [ci] Use compliance stage template (dotnet#7818) [build] pass `--skip-sign-check` to `dotnet workload` (dotnet#7840)
* main: Bump to dotnet/installer@632ddca 8.0.100-preview.3.23128.1 (dotnet#7836) LEGO: Merge pull request 7852 [ci] Reduce overhead for MSBuildIntegration unit test jobs. (dotnet#7832) [ci] Allow dynamic`$(NuGetArtifactName)` values (dotnet#7848) [Xamarin.Android.Build.Tasks] guard `AutoImport.props` against empty values (dotnet#7837) [Mono.Android] Print type & member remapping info (dotnet#7844) [Mono.Android] Tweak AndroidMessageHandler behavior for WCF support (dotnet#7785) LEGO: Merge pull request 7845 Localized file check-in by OneLocBuild Task (dotnet#7842) [ci] Use compliance stage template (dotnet#7818) [build] pass `--skip-sign-check` to `dotnet workload` (dotnet#7840) Replace K4os.Hash.xxHash with System.IO.Hashing (dotnet#7831) $(AndroidPackVersionSuffix)=preview.3; net8 is 34.0.0-preview.3 (dotnet#7839)
* main: (22 commits) Bump to dotnet/installer@632ddca 8.0.100-preview.3.23128.1 (dotnet#7836) LEGO: Merge pull request 7852 [ci] Reduce overhead for MSBuildIntegration unit test jobs. (dotnet#7832) [ci] Allow dynamic`$(NuGetArtifactName)` values (dotnet#7848) [Xamarin.Android.Build.Tasks] guard `AutoImport.props` against empty values (dotnet#7837) [Mono.Android] Print type & member remapping info (dotnet#7844) [Mono.Android] Tweak AndroidMessageHandler behavior for WCF support (dotnet#7785) LEGO: Merge pull request 7845 Localized file check-in by OneLocBuild Task (dotnet#7842) [ci] Use compliance stage template (dotnet#7818) [build] pass `--skip-sign-check` to `dotnet workload` (dotnet#7840) Replace K4os.Hash.xxHash with System.IO.Hashing (dotnet#7831) $(AndroidPackVersionSuffix)=preview.3; net8 is 34.0.0-preview.3 (dotnet#7839) [Xamarin.Android.Build.Tasks] Remove support for mkbundle (dotnet#7772) [Xamarin.Android.Build.Tasks] `unable to open file as zip archive`? (dotnet#7759) [monodroid] Properly process satellite assemblies (dotnet#7823) Bump to xamarin/java.interop/main@77800dda (dotnet#7824) [ci] Use AZDO built-in parallelization strategy. (dotnet#7804) Bump to dotnet/installer@e3ab0b5 8.0.100-preview.2.23123.10 (dotnet#7813) [ci] Run nunit tests with stable .NET version (dotnet#7826) ...
Context: dotnet/android#7837 Context: #46 I ended up making this change in xamarin-android to avoid a potential issue in all .NET projects, so bringing it here as well. In cases where the `android` workload is not installed, Xamarin.Legacy.Sdk can accidentally wildcard your entire disk: MSBUILD : warning MSB5029: The value “\**\.*\**” of the “Exclude” attribute in element <ItemGroup> in file “/Users/moljac/.nuget/packages/xamarin.legacy.sdk/0.2.0-alpha2/Sdk/AutoImport.Android.props (26,61)” is a wildcard that results in enumerating all files on the drive, which was likely not intended. Check that referenced properties are always defined. Add checks that `$(MonoAndroidAssetsPrefix)` and `$(MonoAndroidResourcePrefix)` are not blank.
There is an email thread about ASP.NET projects in VS -- and something is potentially slowing down builds when optional workloads are installed.
One concern is our
AutoImport.props
, take for instance the followingfoo.targets
file:In this case
$(MonoAndroidResourcePrefix)
and$(MonoAndroidAssetsPrefix)
will be blank, and we get:I'm not sure this is causing an actual problem, but I think it's a good idea to add some safety here.
I will make a similar change in Xamarin.Legacy.Sdk.