-
Notifications
You must be signed in to change notification settings - Fork 151
Conversation
* Add targets file in order to automatically include code contracts on nuget install * Place all contents under build/ directory
* Remove extra space from library path * Use relative path when including analysis targets so that the same file can be used for multiple MsBuild versions
e96531d
to
631e1d2
Compare
@@ -12,6 +12,7 @@ | |||
<licenseUrl>https://github.com/Microsoft/CodeContracts/blob/master/LICENSE.txt</licenseUrl> | |||
</metadata> | |||
<files> | |||
<file src="**\*" /> | |||
<file src="**\*" target="build\" /> | |||
<file src="..\..\..\..\..\..\Dotnet.Contracts.targets" target="build\" /> |
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.
Indentation seems off on this line.
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.
probably tabs instead of spaces.
Will fix
</PropertyGroup> | ||
|
||
<!-- Use MsBuild specific targets file if availible --> | ||
<Import Project="$(CodeContractsInstallDir)\MsBuild\v$(MSBuildToolsVersion)\Microsoft.CodeContracts.targets" |
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.
Why mix MSBuildToolsVersion
with VisualStudioVersion
?
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.
Seems like I missed to change it. It was originally VisualStudioVersion from another .targets file but MSBuildTools seemed correct. Will fix
<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003"> | ||
|
||
<PropertyGroup> | ||
<CodeContractsInstallDir>$(MSBuildThisFileDirectory)</CodeContractsInstallDir> |
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.
does it set it unconditionally? A condition could be added in order to be able to override the value in the project file.
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.
Why would you want to override the value for "where is Code Contracts" to something other than "right here" when using the NuGet package?
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.
I am of the same opinion as yaakov-h, I think that you add the nuget package in order to use that specific version and no other.
We could relatively simple make it use the system installed version if one is installed and ignore the nuget version, (easier than the other way around), but I would find that un-intuitive and confusing.
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's backwards from how the package would typically be used.
If you don't have CC installed, the package should use it's own version.
If you do have CC installed and no package, you would use the system version.
If you have CC installed and you have projects that use different package versions, each project should use the version that it requires, or was last tested with, or whatever. Modifying the system version should introduce changes to a project that has taken a dependency on a specific version of the NuGet package.
I see absolutely no point in having system Code Contracts installed, then installing a NuGet package, and having the package just defer to the global installation that was already there and being used. The package becomes redundant and confusing.
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.
Ok, agreed :)
@Daniel-Svensson I would really love to see this completed. Can you sign the contributor license agreement? |
@jeremysimmons i could try looking in a week or two if required (I am a bit unsure this is complex enough to warrant signing a cla). I did not find any info about any CLA in the main readme.md do you know where to do so for this project? |
Closing since the project seems abandoned |
This PR makes it possible to use the code contracts nuget package by just adding Dotnet.Contracts as a nuget reference. (first commit)
It also makes some minor improvements to the target files such as adding support for .Net 4.6.2 (second commit). This can be moved to a second PR if requested.
Nuget improvements
The .targets files will be automatically included in the Project and no further action is required unless the MSI is also installed .
It should solve the following issues
It should also allowing code contract rewriter to run for VS 2017 #451
However if you have installed the MSI you do need to include the following workaround at the top of your project file (it must be before "Microsoft.CSharp.targets" is included) which is mentioned in Issue #368
.target file improvements
Use relative path when including analysis targets so that the same file can be used for multiple MsBuild versions