Move some targets into a Directory.Build.targets file #39712

dougbu · 2022-01-23T05:54:27Z

not used outside IIS testassets/ folder

nits:

remove / update outdated comments
use $(DotNetRoot) property

dougbu · 2022-01-23T22:35:37Z

@adityamandaleeka could you find a reviewer for this quick fix please❔

dougbu · 2022-01-23T22:36:08Z

/btw the bottom line here is an attempt to make the IIS projects slightly less unusual

HaoK · 2022-01-24T07:40:05Z

src/Servers/IIS/IIS/test/testassets/Directory.Build.targets

+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory)..\, Directory.Build.targets))\Directory.Build.targets" />
+
+  <PropertyGroup>
+    <!-- Work around until we get a new WebSdk -->


@Tratcher any context about what this workaround comment is talking about?

I wondered about this too❕

LOL, no, but it happened back in 3.0 so it's probably safe to say we got a fresh WebSdk update since then 😁.
dougbu@a474a05

The current default (in Microsoft.NET.Sdk.Web.ProjectSystem.targets) is inprocess and this override may make sense still. Let's use this space to answer

Should we update InProcessNewShimWebSite.csproj and InProcessWebSite.csproj instead of leaving this setting here❔

Did I break anything by effectively removing this setting from NativeIISSample.csproj and ServerComparison.TestSites.csproj❔

Does the setting have any impact in projects that import testsite.props but don't use Microsoft.NET.Sdk.Web❔

What are the best hosting models for each of the affected projects❔

On (1), I suspect (given the names), the current setting is unhelpful for both InProcess*WebSite projects. Suggest we simply delete these lines and focus on the other projects.

On (2), "break" is probably too strong a word but I did change things. Good thing tests are still working.

HaoK

Looks good assuming you tried running the tests locally (dotnet test) and in VS test explorer (note make sure you run in an administrator powershell)

dougbu · 2022-01-24T18:24:55Z

assuming you tried running the tests locally (dotnet test) and in VS test explorer (note make sure you run in an administrator powershell)

I am unable to test locally even on main and can't find instructions about which IIS components I may be missing. Suggestions❔

In the meantime, I'll see whether the IIS Express functional tests work better for me.

HaoK · 2022-01-24T20:11:39Z

You'll need regular IIS enabled, I think you just will need to make sure IIS is installed/enabled, and have the hosting bundle, https://docs.microsoft.com/en-us/aspnet/core/host-and-deploy/iis/?view=aspnetcore-6.0 might be a good verification, basically make sure you can publish to IIS in VS and that is what the tests need to work

- tests have for a while worked only after `publish` anyhow nits: - remove outdated comment - use `$(DotNetRoot)` property

- not used outside IIS testassets folder

- this reverts some of commit 4bb1920bfa4f - leave nits alone

dougbu · 2022-01-24T22:26:42Z

I reverted the <BaseOutputPath/> / <OutputPath/> part, meaning this PR mostly moves the test asset-only targets into the new Directory.Build.targets file. Main issue was the deployers make a lot of assumptions about where bin/ folders are in local tests and the output paths were still changed in src/DefaultBuilder/, src/Hosting, and other parts of src/Servers/. We might be able to remove most of those Arcade overrides, but I wasn't ready to change the deployer expectations.

dougbu · 2022-01-24T22:29:22Z

src/Servers/IIS/IIS/test/testassets/InProcessNewShimWebSite/InProcessNewShimWebSite.csproj

  <Import Project="..\..\..\..\build\testsite.props" />

  <PropertyGroup>
    <TargetFramework>$(DefaultNetCoreTargetFramework)</TargetFramework>
    <AssemblyName>InProcessWebSite</AssemblyName>
-    <TestAssetOutputName>InProcessNewShimWebSite</TestAssetOutputName>


Another nit: Setting matches the default.

dougbu · 2022-01-24T22:30:19Z

src/Servers/testassets/Directory.Build.props

@@ -2,7 +2,7 @@
  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory)..\, Directory.Build.props))\Directory.Build.props" />

  <PropertyGroup>
-    <!-- Tests do not work on Helix or when bin/ directory is not in project directory due to undeclared dependency on test content. -->
+    <!-- Local testing does not work when bin/ directory is not in project directory due to deployer expectations. -->


nit: I don't think this was about "test content". Problem seems rooted in the various deployers.

dougbu · 2022-01-24T23:33:30Z

Well, even w/ IIS Express installed and much of the previous change reverted, Microsoft.AspNetCore.Server.IIS.IISExpress.FunctionalTests.InProcess.MaxRequestBodySizeTests.SetIISLimitMaxRequestBodyLogsWarning failed and more tests were skipped (159) than passed (147).

I'd like to try IIS but obviously don't have all the components I need. Trying a complete removal of the "Internet Information Services" fix, removing the hosting bundle, and rebooting before a retry…

dougbu · 2022-01-25T00:47:20Z

I'd like to try IIS but obviously don't have all the components I need. Trying a complete removal of the "Internet Information Services" fix, removing the hosting bundle, and rebooting before a retry…

This didn't help much. Getting errors like Xunit.Sdk.XunitException: Could not find aspnetcorev2_inprocess.dll loaded in process w3wp and lots of Internal Server Errors. While many of the IIS tests appear to be Helix-only (probably because the deployers aren't consistent about handling the local testing case), IIS.FunctionalTests failed (rather than skipped) 135 tests. Is there a non-default component I need❔

My leaning is toward giving up on local testing unless using IIS Express until and unless I decide to take another crack at finally migrating these tests to understand artifacts/bin/.

Any reason not to merge this PR in the meantime❔

HaoK · 2022-01-25T08:16:53Z

I wouldn't worry too much about getting IIS working as long as some IISExpress tests were working for you locally with your change. Its fine if you merge this as is, if things are completely broken locally for me after picking up this PR, I'll fix/revert at that point

This reverts commit 209ff13.

Revert "Move some targets into a Directory.Build.targets file (#39712)" This reverts commit 209ff13. Bring back directory build Revert "Revert fx_ver rename and use aspnet namespace" This reverts commit d9c3d28. Revert "Revert "Revert fx_ver rename and use aspnet namespace"" This reverts commit e2b34b88cde041b9e4bf1137eb4c07b9788b2232. Revert "Bring back directory build" This reverts commit ad9007e3ef6b7fce1c3677aa8cfa891c23fea32e. Revert "Revert "Move some targets into a Directory.Build.targets file (#39712)"" This reverts commit e7937bc.

Minor cleanup More Cleanup/CommonLibTests Move more settings into Common.Settings More build cleanup Revert stuff Revert "Revert stuff" This reverts commit 7faa512f84631f7ef7588e498c6a70c62c8ea350. Undo -nobuild changes to iis tests to fix running tests locally Revert "Move some targets into a Directory.Build.targets file (#39712)" This reverts commit 209ff13. Bring back directory build Revert "Revert fx_ver rename and use aspnet namespace" This reverts commit d9c3d28. Revert "Revert "Revert fx_ver rename and use aspnet namespace"" This reverts commit e2b34b88cde041b9e4bf1137eb4c07b9788b2232. Revert "Bring back directory build" This reverts commit ad9007e3ef6b7fce1c3677aa8cfa891c23fea32e. Revert "Revert "Move some targets into a Directory.Build.targets file (#39712)"" This reverts commit e7937bc. Unskip some more local tests

ghost added the area-runtime label Jan 23, 2022

dougbu force-pushed the dougbu/remove.iis.override branch 2 times, most recently from c32b6ae to eada1f5 Compare January 23, 2022 22:22

dougbu changed the title ~~!trial! Try using regular artifacts/bin/ folder~~ Use regular artifacts/bin/ folder for IIS projects Jan 23, 2022

dougbu changed the title ~~Use regular artifacts/bin/ folder for IIS projects~~ Use regular artifacts/bin/ folders for IIS projects Jan 23, 2022

dougbu force-pushed the dougbu/remove.iis.override branch from eada1f5 to d1678d5 Compare January 23, 2022 22:23

dougbu requested review from a team and adityamandaleeka January 23, 2022 22:25

dougbu force-pushed the dougbu/remove.iis.override branch 4 times, most recently from ace2ddc to 9b5b80f Compare January 24, 2022 02:43

dougbu marked this pull request as ready for review January 24, 2022 05:02

dougbu requested review from BrennanConroy, halter73 and Tratcher as code owners January 24, 2022 05:02

HaoK reviewed Jan 24, 2022

View reviewed changes

HaoK approved these changes Jan 24, 2022

View reviewed changes

dougbu added 3 commits January 24, 2022 14:20

Use regular artifacts/bin/ folders for IIS projects

529c8fc

- tests have for a while worked only after `publish` anyhow nits: - remove outdated comment - use `$(DotNetRoot)` property

Move targets into a Directory.Build.targets file

9c58219

- not used outside IIS testassets folder

Do not "Use regular artifacts/bin/ folders for IIS projects"

20414d6

- this reverts some of commit 4bb1920bfa4f - leave nits alone

dougbu force-pushed the dougbu/remove.iis.override branch from 9b5b80f to 20414d6 Compare January 24, 2022 22:20

dougbu changed the title ~~Use regular artifacts/bin/ folders for IIS projects~~ Move some targets into a Directory.Build.targets file Jan 24, 2022

dougbu commented Jan 24, 2022

View reviewed changes

wtgodbe approved these changes Jan 25, 2022

View reviewed changes

dougbu merged commit 209ff13 into dotnet:main Jan 25, 2022

dougbu deleted the dougbu/remove.iis.override branch January 25, 2022 17:46

ghost added this to the 7.0-preview1 milestone Jan 25, 2022

wtgodbe modified the milestones: 7.0-preview1, 7.0-preview2 Jan 31, 2022

HaoK added a commit that referenced this pull request Feb 23, 2022

Revert "Move some targets into a Directory.Build.targets file (#39712)"

e7937bc

This reverts commit 209ff13.

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move some targets into a Directory.Build.targets file #39712

Move some targets into a Directory.Build.targets file #39712

dougbu commented Jan 23, 2022 •

edited

Loading

dougbu commented Jan 23, 2022

dougbu commented Jan 23, 2022

HaoK Jan 24, 2022

dougbu Jan 24, 2022

Tratcher Jan 25, 2022

dougbu Jan 25, 2022

dougbu Jan 25, 2022

HaoK left a comment •

edited

Loading

dougbu commented Jan 24, 2022

HaoK commented Jan 24, 2022

dougbu commented Jan 24, 2022

dougbu Jan 24, 2022

dougbu Jan 24, 2022

dougbu commented Jan 24, 2022

dougbu commented Jan 25, 2022

HaoK commented Jan 25, 2022

Move some targets into a Directory.Build.targets file #39712

Move some targets into a Directory.Build.targets file #39712

Conversation

dougbu commented Jan 23, 2022 • edited Loading

dougbu commented Jan 23, 2022

dougbu commented Jan 23, 2022

HaoK Jan 24, 2022

Choose a reason for hiding this comment

dougbu Jan 24, 2022

Choose a reason for hiding this comment

Tratcher Jan 25, 2022

Choose a reason for hiding this comment

dougbu Jan 25, 2022

Choose a reason for hiding this comment

dougbu Jan 25, 2022

Choose a reason for hiding this comment

HaoK left a comment • edited Loading

Choose a reason for hiding this comment

dougbu commented Jan 24, 2022

HaoK commented Jan 24, 2022

dougbu commented Jan 24, 2022

dougbu Jan 24, 2022

Choose a reason for hiding this comment

dougbu Jan 24, 2022

Choose a reason for hiding this comment

dougbu commented Jan 24, 2022

dougbu commented Jan 25, 2022

HaoK commented Jan 25, 2022

dougbu commented Jan 23, 2022 •

edited

Loading

HaoK left a comment •

edited

Loading