-
Notifications
You must be signed in to change notification settings - Fork 537
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] Move MonoAndroidAssetsDirIntermediate #8166
[Xamarin.Android.Build.Tasks] Move MonoAndroidAssetsDirIntermediate #8166
Conversation
Fixes: dotnet#8095 Context: f99fc81 Initialize `$(MonoAndroidAssetsDirIntermediate)` earlier in `Xamarin.Android.Common.targets`, so that the existing `$(_XARemapMembersFilePath)` property (f99fc81) is able to correctly use it.
<MakeDir | ||
Condition=" '@(_AndroidMamMappingFile->Count())' != '0' " | ||
Directories="$(MonoAndroidAssetsDirIntermediate)/xa-internal" | ||
Directories="$(MonoAndroidAssetsDirIntermediate)xa-internal" | ||
/> |
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.
Wait, does this really fix it? MSBuild does things in passes, a simplification/example of the ordering:
- Imports & Properties
- Item Groups
- Targets
Would $(MonoAndroidAssetsDirIntermediate)
have always have been set in pass 1, and these targets are pass 3?
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.
While $(MonoAndroidAssetsDirIntermediate)
would always be set in pass 1, there is still an ordering issue between properties, because properties are set in file order:
$(_XARemapMembersFilePath)
was set first: https://github.com/xamarin/xamarin-android/blob/69b4ab056badcb5d25a5faf39d08baeb1b8b7010/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L458$(MonoAndroidAssetsDirIntermediate)
was set afterward: https://github.com/xamarin/xamarin-android/blob/69b4ab056badcb5d25a5faf39d08baeb1b8b7010/src/Xamarin.Android.Build.Tasks/Xamarin.Android.Common.targets#L885
When (1) is evaluated, $(MonoAndroidAssetsDirIntermediate)
hasn't been set yet, thus $(_XARemapMembersFilePath)
will be xa-internal/xa-remap-members.xml
(which has the nice property of matching the bug report).
Indeed, when viewing the diagnostic build log, we see:
_XARemapMembersFilePath = xa-internal/xa-remap-members.xml
By moving $(MonoAndroidAssetsDirIntermediate)
to be defined before $(_XARemapMembersFilePath)
is defined, we ensure that $(MonoAndroidAssetsDirIntermediate)
doesn't expand to the empty string, because it will have been set by the time it is used.
* main: Bump to dotnet/installer@28d4a6b4be 8.0.100-preview.7.23330.16 (dotnet#8162) [Xamarin.Android.Build.Tasks] Move MonoAndroidAssetsDirIntermediate (dotnet#8166)
* main: Bump to dotnet/installer@28d4a6b4be 8.0.100-preview.7.23330.16 (dotnet#8162) [Xamarin.Android.Build.Tasks] Move MonoAndroidAssetsDirIntermediate (dotnet#8166) [xaprepare] update Debian dependencies for current unstable (trixie) (dotnet#8169) [CI] Use dotnet test slicer in nightly tests (dotnet#8154) [Xamarin.Android.Build.Tasks] MarshalMethodsAssemblyRewriter+new file (dotnet#8151) Bump to dotnet/installer@d2a244f560 8.0.100-preview.7.23325.5 (dotnet#8142)
Fixes: #8095
Context: f99fc81
Initialize
$(MonoAndroidAssetsDirIntermediate)
earlier inXamarin.Android.Common.targets
, so that the existing$(_XARemapMembersFilePath)
property (f99fc81) is able to correctly use it.