[.NETCore][RFC] Rewrite reading of csproj files #1029
Comments
|
This looks good. I've run into a problem with our XML-based solution not supporting "Exclude" anyway. |
SabotageAndi
added
the
SpecFlow Team Backlog
|
@SabotageAndi what are the area which should be changed for this task? it looks like relatively small and self-contained so I take a stab on it. |
|
and the current tests are here: https://github.com/techtalk/SpecFlow/blob/DotNetCore/Tests/TechTalk.SpecFlow.GeneratorTests/MsBuildProjectReaderTests.cs |
|
You are reading my thoughts. One last question, does tests has solutions with .NET Core, or that's up to me create such projects to test against them? There mention that Exclude is not supported in the project. Should I try to add test for that scenario as well ? |
|
Currently everything is still in full framework (most .NET 4.5, some higher) and it will take some time until we compile for .NET Standard/Core 2.0. Reason is that we need to upgrade our own test infrastructure first. @david1995, @nesli92 and I are working on that currently. About Exclude: yes, please add tests for that. And also other ugly MSBuild stuff you can imagine ;-) |
|
I try to replace the reading logic for MSBuild files, and try following approaches
The issue for both approaches, is that Buildalizer which dependes on the latest MSBuild API and MSBuild API v15 require at least /cc: @SabotageAndi |
|
@kant2002 I have to think about this tomorrow. I wasn't aware that MSBuild needs now net46. |
- Control usage of new implementation via SPECFLOW_USE_MSBUILDAPI to be able test both implementations side-by-side - Update TechTalk.SpecFlow.Generator to net46 to be able use MSBuild API, and propagate changes where required across projects which depends on the TechTalk.SpecFlow.Generator - Update MSBuild API to 15.6. This change may be not needed, but, I strongly suspect that it will play more nicely with all version of .NET Core up to not yet released .NET Core 2.1 - Add tests on the exclude folder - Change test a bit to take into account that paths `.\filename` and `filename` appear as two items via MSBuild API, and I dont think that duplicate items is desired behavior - Implementation of MSBuildApiProjectReader is just old resurrected implementation using MSBuild API with small changes See SpecFlowOSS#1029
- Control usage of new implementation via SPECFLOW_USE_MSBUILDAPI to be able test both implementations side-by-side - Update TechTalk.SpecFlow.Generator to net46 to be able use MSBuild API, and propagate changes where required across projects which depends on the TechTalk.SpecFlow.Generator - Update MSBuild API to 15.6. This change may be not needed, but, I strongly suspect that it will play more nicely with all version of .NET Core up to not yet released .NET Core 2.1 - Add tests on the exclude folder - Change test a bit to take into account that paths `.\filename` and `filename` appear as two items via MSBuild API, and I dont think that duplicate items is desired behavior - Implementation of MSBuildApiProjectReader is just old resurrected implementation using MSBuild API with small changes See SpecFlowOSS#1029
|
I added PR to show you the what could be done in case if you decide go with |
|
@SabotageAndi Any ideas how to proceed? |
|
@kant2002 Yes, but didn't had yet the time to write. My idea is to add some properties for the different TFM- combinations into the Directory.builds.props. So it is fine when you upgrade the projects to net46. About this PR: is this the old MSBuild code? Looks familiar. Some info about it: We switched to the XML parsing because we had caching issues with MSBuild. Sometimes it didn't get it, when you add new files. For that reason, I wanted to have a look at design time builds. After reading this blogpost (https://daveaglick.com/posts/running-a-design-time-build-with-msbuild-apis) I was sure, I don't want to call the MSBuild API direct and use Buildalyzer instead. |
|
@SabotageAndi Buildalizer support only .NET Standard 2.0 So my question, should project targeting for Generator and all other projects to be bumped to .NET 4.7 which support NET Standard 2.0, or I should go to Buildalizer and made PR to support 4.6. This is depends mostly on your willingness to support VS2015, if follow same logic, then generator process would be on the machine which has VS2017, then net47 is fine, and Buildalizer could be used without modifications. |
|
Go for net47/netstandard 2.0 and let's see if we get feedback to support on the generator side lower frameworks. |
|
@SabotageAndi Followup with changes which not related to major task, but I do anyway (or want to do).
Questions:
|
|
See #1127 as additional motivation for me to have |
|
build.bat - stupid solution items, I forgot to delete it there. PostCompile.bat: this is currently needed for Integration & Specs tests. We want to get rid of it. This work will happen in this PR: #1128. The tests doesn't run in the DotNetCore at the moment at all, so simple don't change it. Wenn we merge #1128 it should hopefully be gone. I was with my comment above to vague and misunderstanding. Before we add netstandard2.0 to the TargetFrameworks I would like to clean up things at get other things to work. So at least issues #1035, #1036, #1031. If we don't do the things before, the addition of netstandard2.0 will get messy. So until that, please stay at the full framework at the moment. AppVeyor: yes, please update the path and disable the Integration & Specs tests at the moment. They will only fail. About additional changes because of path changes: for sure the creation of the NuGet packages. You have to adjust the props file like https://github.com/techtalk/SpecFlow/blob/master/Installer/NuGetPackages/SpecFlow/SpecFlow.nuspec.props Everything answered? |
|
@SabotageAndi Yes you answer all what I need. Probably I have enough input, so let me update code and I will comeback with changes. |
|
@kant2002 Cool! I am looking forward to the PR. |
|
@SabotageAndi Next problem is that Buildalizer is don't strongly signed. I think that if take old MSBuild API code and don't use GlobalProjects that could be solve problems with caching of files which you do mention. I see rationale with going with Buildalyzer, just provide alternatives if strong signing will be blocker. |
|
@kant2002 I opened an issue at Buildalyzer about it (phmonte/Buildalyzer#51). |
|
@SabotageAndi I implement change, that's actually straightforward, but not able to run tests, since Buildalyzer assembly could not be loaded. |
|
I would propose stick with MSBuild API, accept PR, create another issue mentioning Buildalizer, Create PR for that small change and wait until Buildalyzer respond |
|
@kant2002 I extracted the TFMs into the Directory.build.props and also added a central location to enable/disable strong name signing (PR #1133) It's now a simple MSBuild property (https://github.com/techtalk/SpecFlow/blob/DotNetCore/Directory.Build.props#L12) which is currently false. |
|
@SabotageAndi Cool, let me rebase and add Buldalyzer |
- Control usage of new implementation via SPECFLOW_USE_MSBUILDAPI to be able test both implementations side-by-side - Update TechTalk.SpecFlow.Generator to net46 to be able use MSBuild API, and propagate changes where required across projects which depends on the TechTalk.SpecFlow.Generator - Update MSBuild API to 15.6. This change may be not needed, but, I strongly suspect that it will play more nicely with all version of .NET Core up to not yet released .NET Core 2.1 - Add tests on the exclude folder - Change test a bit to take into account that paths `.\filename` and `filename` appear as two items via MSBuild API, and I dont think that duplicate items is desired behavior - Implementation of MSBuildApiProjectReader is just old resurrected implementation using MSBuild API with small changes See SpecFlowOSS#1029
|
@SabotageAndi I'm ready. Please take a look at PR |
* Rewrite MSBuild reader to use MSBuild API - Control usage of new implementation via SPECFLOW_USE_MSBUILDAPI to be able test both implementations side-by-side - Update TechTalk.SpecFlow.Generator to net46 to be able use MSBuild API, and propagate changes where required across projects which depends on the TechTalk.SpecFlow.Generator - Update MSBuild API to 15.6. This change may be not needed, but, I strongly suspect that it will play more nicely with all version of .NET Core up to not yet released .NET Core 2.1 - Add tests on the exclude folder - Change test a bit to take into account that paths `.\filename` and `filename` appear as two items via MSBuild API, and I dont think that duplicate items is desired behavior - Implementation of MSBuildApiProjectReader is just old resurrected implementation using MSBuild API with small changes See #1029 * Update Generator project to use net461 target framework * Comment out not working test projects * Use Buildalizer for loading projects * Remove XML project reader completely This is justified by fact that supporting it, means reimplementing whole MSBuild logic, which is not reasonable. Also for loading plugins would be needed read PackageReferences from the projects, and this would be problematic implement correctly. Current implementation is also flawled. See #1102, and wellknown fact that Exclude property on the MSBuild items is not supported
|
Does something else should be done on this issue ? |
|
For now, yes. I forgot to close the issue after merging the PR. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
With SpecFlow 2.2, we changed to reading the csproj via XML. This has limited support for the SDK project format.
To get back complete MSBuild functionality, we require a design time build of the project.
For that we could use https://github.com/daveaglick/Buildalyzer.
The text was updated successfully, but these errors were encountered: