Conversation
ghost
added
the
|
Tagging subscribers to this area: @Anipik, @safern, @ViktorHofer |
pgovind
added
linkable-framework
|
Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @tannergooding, @sbomer |
safern
left a comment
safern
left a comment
There was a problem hiding this comment.
Can all these infra be helpful in the future?
Caveat that I don't completely understand how these changes work. These changes could be helpful in the future, but are not needed right now. IIUC, these changes will preserve mscorlib.dll in a trimmed app. However, mscorlib.dll is just a facade, so the linker will try to eliminate the type forwards completely (and replace them with the real references?) such that mscorlib.dll is not present in the final output. So, if another scenario pops up where we need mscorlib.dll to be present in the final output, we'll need these changes, but we can delete them now. |
|
Hello @pgovind! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
| <Project DefaultTargets="Build"> | ||
| <Import Project="$([MSBuild]::GetPathOfFileAbove(Directory.Build.props))" /> | ||
|
|
||
| <ItemGroup> |
There was a problem hiding this comment.
Can you leave these in place? The issue with the current behavior is that as soon as someone adds one TestConsoleAppSourceFiles item, the implicitly defined ones go away and the tests don't run.
I wish we had one model in these tests.
cc @joperezr
There was a problem hiding this comment.
Right, I remember that. That's why I deleted all the TestConsoleAppSourceFiles Include=... lines. That would mean every file gets included and tested right? Anyway, this is just my curiosity. I'll add those lines back.
There was a problem hiding this comment.
If you remove all of the TestConsoleAppSourceFiles items, then the infra searches for all .cs files in the current directory. As soon as you add 1 TestConsoleAppSourceFiles item, it stops doing that. I want us to stop relying on the implicit behavior, so people don't make mistakes in the future.
There was a problem hiding this comment.
While @eerhardt point is correct in that the model is not ideal so one inclusion overrides the default, I think you are fine to keep it the way you had it (with them removed) since it would effectively be the same thing here.
There was a problem hiding this comment.
If you remove all of the TestConsoleAppSourceFiles items, then the infra searches for all .cs files in the current directory. As soon as you add 1 TestConsoleAppSourceFiles item, it stops doing that. I want us to stop relying on the implicit behavior, so people don't make mistakes in the future.
I see, We could easily just remove the automatic globbing if you think that would be better. I'm happy to take care of that and find all Trimming tests depending on it and moving them to the manual specification instead.
There was a problem hiding this comment.
The other option would be to make it work like all other globbing behavior in projects (for example, the *.cs Compile globbing). If I don't want a .cs file to be a top level test, I write
<TestConsoleAppSourceFiles Remove="SharedHelperFile.cs" />.
And if I need to set some metadata items for a specific file I write:
<TestConsoleAppSourceFiles Update="MyTest.cs" SomeSwitch="true" />.
And then by default the globbing just works like people normally expect - it is always there. And adding/updating/removing the TestConsoleAppSourceFiles item doesn't affect other tests.
There was a problem hiding this comment.
I'd need to check if the Update part will work in this case since globbing will set the itemspec as the full path and I'm not sure if just filename and extension would match and appropriately set the switch, but I'll take a look to see if this could be done.
7 participants
Clean up some infra changes that went into #48527. These changes are now unnecessary with #50593