-
Notifications
You must be signed in to change notification settings - Fork 64
[build] Add support for Configuration.Override.props
#45
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
| @@ -0,0 +1,19 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <Project DefaultTargets="Build" ToolsVersion="4.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | |||
| <!-- Note: MUST be imported *after* $(Configuration) is set! --> | |||
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.
Maybe you can add:
<PropertyGroup>
<Configuration Condition="'$(Configuration)' == ''">ConfigurationNotSetInConfigurationPropsForProject-$(MSBuildProjectFile)</Configuration>
</PropertyGroup>
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.
That'll result in obscure/bizarre errors, as a non-existent configuration could be used for building:
<!-- "bad" .csproj -->
<Project ...>
<Import Project="..\..\Configuration.props" />
<PropertyGroup>
<Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
</PropertyGroup>
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' " />
<PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' " />
</Project>
With such a "bad" project, $(Configuration) will be set, causing MSBuild to look for the ConfigurationNotSetInConfigurationPropsForProject|AnyCPU configuration, which doesn't exist.
At best, it'll still compile but you have no idea where the output file is.
More likely -- if #if is used at all -- is that the project will fail to build with some "bizarro" error that in no way suggests that the error is "an invalid $(Configuration) value was specified, dummy!".
7d93f21 to
5346cfd
Compare
Configuration.props
Outdated
| Condition="Exists('$(MSBuildThisFileDirectory)bin\Build$(Configuration)\MonoInfo.props')" | ||
| /> | ||
| <PropertyGroup> | ||
| <UtilityOutputFullPath Condition=" '$(UtilityOutputPath)' == '' ">$(MSBuildThisFileDirectory)bin\$(Configuration)\</UtilityOutputFullPath> |
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.
Um.. shouldn't $(UtilityOutputFullPath) be the "public" property, instead of $(UtilityOutputPath) ?
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.
Indeed!
5346cfd to
52f0a07
Compare
README.md
Outdated
| * `$(UtilityOutputFullPath)`: Directory to place various utilities such as | ||
| [`class-parse`](tools/class-parse), [`generator`](tools/generator), | ||
| and [`logcat-parse`](tools/logcat-parse). By default this is | ||
| `bin/$(Configuration)`. |
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.
um and accordingly this (bin/$(Configuration)) should be updated to show an absolute path or explicitly mention that here.
|
Other than the doc related comment, LGTM! |
For proper use, the [xamarin-android][xa] build needs to place various
Java.Interop utilities such as class-parse.exe and generator.exe into
`$prefix/lib/mandroid`, so that Xamarin.Android.Build.Tasks.dll will
properly verify the installation environment.
There are three ways this could be accomplished:
1. The `xamarin-android` Makefile could explicitly build these
utilities and override `$(OutputPath)`:
xbuild external/Java.Interop/tools/class-parse /p:OutputPath=`pwd`/bin/$(CONFIGURATION)/lib/mandroid
The problem with this is that we want to have the xamarin-android
build system rely on MSBuild as much as possible, and this
approach, while workable, runs counter to those desires.
2. We could add additional project configurations to control where the
output directory should be. This was suggested by
[@atsushieno][pr41].
My concern with this approach is that it's not easily extensible:
it's not just a few projects that need to place files into
`$prefix/lib/mandroid`, but all of their dependencies as well.
Such an approach would thus require adding lots of new
configurations to lots of projects.
3. Java.Interop could adopt a `xamarin-android`-style
`Configuration.props` system. This would allow xamarin-android to
*generate* a `Configuration.Override.props` file to specify the
correct output path for those utilities.
(3) is the chosen solution. It allows adding e.g.
`external/Java.Interop/tools/generator/generator.csproj` to
`Xamarin.Android.sln`, allowing it to be built "normally" from the
`xamarin-android` build system, while causing the built files to be
placed into e.g. `xamarin-android/bin/Debug/lib/mandroid` instead of
the less useful `xamarin-android/external/Java.Interop/bin/Debug`.
[xa]: https://github.com/xamarin/xamarin-android/
[pr41]: dotnet#41
52f0a07 to
b4ed684
Compare
Changes in xamarin-android-tools between 75530565b6aa903b3a0e52b61df4dd94475a19fc and 9e78d6ee586b498d0ea082b3bc00432c23583dd1: 9e78d6e (HEAD, origin/master, origin/HEAD, master) [tests] fix test failures on Windows (dotnet#47) bdf0158 Better support no installed JDKs on macOS (dotnet#48) 6353659 Log what is happening during path selection (dotnet#46) 3ef860b Take BUILD_NUMBER into consideration for Version sorting (dotnet#45) d3de054 Allow an optional locator to be provided to JdkInfo (dotnet#43) 917d3b3 Don't require quotes around `release` values (dotnet#41) 7427692 [tests] Unit tests for finding NDK location based on $PATH (dotnet#40) dbc517b Merge pull request dotnet#38 from jonpryor/jonp-ndk-via-path 511d580 Allow finding NDK location based on $PATH b42c217 [tests] Fix DetectAndSetPreferredJavaSdkPathToLatest() test (dotnet#37) a4aad18 Add AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest() (dotnet#35) fae7e0a [tests] Remove temporary directories (dotnet#36) 07c4c2b [Xamarin.Android.Tools.AndroidSdk] Revert JDK validation (dotnet#34)
Changes in xamarin-android-tools between 75530565b6aa903b3a0e52b61df4dd94475a19fc and 9e78d6ee586b498d0ea082b3bc00432c23583dd1: 9e78d6e (HEAD, origin/master, origin/HEAD, master) [tests] fix test failures on Windows (#47) bdf0158 Better support no installed JDKs on macOS (#48) 6353659 Log what is happening during path selection (#46) 3ef860b Take BUILD_NUMBER into consideration for Version sorting (#45) d3de054 Allow an optional locator to be provided to JdkInfo (#43) 917d3b3 Don't require quotes around `release` values (#41) 7427692 [tests] Unit tests for finding NDK location based on $PATH (#40) dbc517b Merge pull request #38 from jonpryor/jonp-ndk-via-path 511d580 Allow finding NDK location based on $PATH b42c217 [tests] Fix DetectAndSetPreferredJavaSdkPathToLatest() test (#37) a4aad18 Add AndroidSdkInfo.DetectAndSetPreferredJavaSdkPathToLatest() (#35) fae7e0a [tests] Remove temporary directories (#36) 07c4c2b [Xamarin.Android.Tools.AndroidSdk] Revert JDK validation (#34)
For proper use, the xamarin-android build needs to place various
Java.Interop utilities such as class-parse.exe and generator.exe into
$prefix/lib/mandroid, so that Xamarin.Android.Build.Tasks.dll willproperly verify the installation environment.
There are three ways this could be accomplished:
The
xamarin-androidMakefile could explicitly build theseutilities and override
$(OutputPath):The problem with this is that we want to have the xamarin-android
build system rely on MSBuild as much as possible, and this
approach, while workable, runs counter to those desires.
We could add additional project configurations to control where the
output directory should be. This was suggested by
@atsushieno.
My concern with this approach is that it's not easily extensible:
it's not just a few projects that need to place files into
$prefix/lib/mandroid, but all of their dependencies as well.Such an approach would thus require adding lots of new
configurations to lots of projects.
Java.Interop could adopt a
xamarin-android-styleConfiguration.propssystem. This would allow xamarin-android togenerate a
Configuration.Override.propsfile to specify thecorrect output path for those utilities.
(3) is the chosen solution. It allows adding e.g.
external/Java.Interop/tools/generator/generator.csprojtoXamarin.Android.sln, allowing it to be built "normally" from thexamarin-androidbuild system, while causing the built files to beplaced into e.g.
xamarin-android/bin/Debug/lib/mandroidinstead ofthe less useful
xamarin-android/external/Java.Interop/bin/Debug.