Skip to content
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.

Sign up for GitHub

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

Jump to bottom

BuildIisNativeProjects -> UseIisNativeAssets #39655

Merged
merged 2 commits into from
Jan 21, 2022
Merged

BuildIisNativeProjects -> UseIisNativeAssets #39655

merged 2 commits into from
Jan 21, 2022

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Jan 20, 2022

Fixes #39654

Clarifies what this property actually does

sebastienros
sebastienros reviewed Jan 20, 2022
View reviewed changes
Directory.Build.props Outdated
@@ -220,7 220,7 @@
<MvcTestingTargets>$(MSBuildThisFileDirectory)src\Mvc\Mvc.Testing\src\Microsoft.AspNetCore.Mvc.Testing.targets</MvcTestingTargets>
<_MvcTestingTasksAssembly>$(ArtifactsBinDir)\Microsoft.AspNetCore.Mvc.Testing.Tasks\$(Configuration)\netstandard2.0\Microsoft.AspNetCore.Mvc.Testing.Tasks.dll</_MvcTestingTasksAssembly>
<!-- IIS native projects can only be built on Windows for x86/x64/ARM64. -->
<BuildIisNativeProjects Condition=" '$(TargetOsName)' == 'win' AND ('$(TargetArchitecture)' == 'x86' OR '$(TargetArchitecture)' == 'x64' OR '$(TargetArchitecture)' == 'ARM64') ">true</BuildIisNativeProjects>
<UpdateIisNativeAssets Condition=" '$(TargetOsName)' == 'win' AND ('$(TargetArchitecture)' == 'x86' OR '$(TargetArchitecture)' == 'x64' OR '$(TargetArchitecture)' == 'ARM64') ">true</UpdateIisNativeAssets>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIS native projects can only be built on Windows, but also they are always built on Windows. When can this property be false on Windows? I currently set it when I build Kestrel for instance, even from a fresh clone. Can some project build.cmd files set it to false safely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe you can just pass in -noBuildNative if you explicitly want to skip native code building as that turns off. But hence the rename here, there's a bunch of projects that need to update iis native assets which is why we are renaming this to something not called build

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically projects that want to pull in ancm will have this set, and if you don't have ancm built, when it tries to update/include it, it will fail, as it should

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-noBuildNative won't change most of what the property does. That switch only controls whether a native (desktop) msbuild happens if you use one of our build.cmd or build.ps1 scripts.
The $(BuildNative) property reflect that choice while we're building but only when building using desktop msbuild and mainly to distinguish the scenario from -buildInstallers / $(BuildInstallers) (the other case where we build using desktop msbuild).

wtgodbe
wtgodbe requested changes Jan 20, 2022
View reviewed changes
Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like you missed a couple?

@HaoK
Copy link
Member Author

HaoK commented Jan 20, 2022

@wtgodbe can you point to the ones you saw that I missed? main is failing due to a bad PR pranav merged, so the checks will be red until that's fixed

dougbu
dougbu reviewed Jan 20, 2022
View reviewed changes
Copy link
Member

@dougbu dougbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand the word "update" in this context though I agree Build isn't correct much of the time. I suggest picking another word to start the name.

The property does a bunch of stuff. Mostly it

  • controls whether managed projects use (pack, copy around, …) native assemblies built in an earlier eng/build.ps1 phase or separate -buildNative build
  • controls how Microsoft.AspNetCore.App.Runtime brings in aspnetcorev2_inprocess.dll when that project is built using desktop msbuild or dotnet msbuild (a corner case but a supported one)

Directory.Build.props Outdated
@@ -220,7 220,7 @@
<MvcTestingTargets>$(MSBuildThisFileDirectory)src\Mvc\Mvc.Testing\src\Microsoft.AspNetCore.Mvc.Testing.targets</MvcTestingTargets>
<_MvcTestingTasksAssembly>$(ArtifactsBinDir)\Microsoft.AspNetCore.Mvc.Testing.Tasks\$(Configuration)\netstandard2.0\Microsoft.AspNetCore.Mvc.Testing.Tasks.dll</_MvcTestingTasksAssembly>
<!-- IIS native projects can only be built on Windows for x86/x64/ARM64. -->
<BuildIisNativeProjects Condition=" '$(TargetOsName)' == 'win' AND ('$(TargetArchitecture)' == 'x86' OR '$(TargetArchitecture)' == 'x64' OR '$(TargetArchitecture)' == 'ARM64') ">true</BuildIisNativeProjects>
<UpdateIisNativeAssets Condition=" '$(TargetOsName)' == 'win' AND ('$(TargetArchitecture)' == 'x86' OR '$(TargetArchitecture)' == 'x64' OR '$(TargetArchitecture)' == 'ARM64') ">true</UpdateIisNativeAssets>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-noBuildNative won't change most of what the property does. That switch only controls whether a native (desktop) msbuild happens if you use one of our build.cmd or build.ps1 scripts.
The $(BuildNative) property reflect that choice while we're building but only when building using desktop msbuild and mainly to distinguish the scenario from -buildInstallers / $(BuildInstallers) (the other case where we build using desktop msbuild).

@HaoK
Copy link
Member Author

HaoK commented Jan 20, 2022

Unless you have a specific suggestion @dougbu I'm going to stick with update since the main motivation was just to move away from build since its not even really building the native bits, update conveys the notion that its trying to 'update' ancm/native bits, that's the idea anyways. Happy to switch it to something better if you have any ideas, given that this isn't something we are going to set/use, just untangling this from build seems good enough

wtgodbe
wtgodbe approved these changes Jan 20, 2022
View reviewed changes
Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wtgodbe can you point to the ones you saw that I missed? main is failing due to a bad PR pranav merged, so the checks will be red until that's fixed

Ah, in that case this looks fine to me

@dougbu
Copy link
Member

dougbu commented Jan 20, 2022

Unless you have a specific suggestion @dougbu I'm going to stick with update since the main motivation was just to move away from build since its not even really building the native bits, update conveys the notion that its trying to 'update' ancm/native bits

My suggestion would be "Use…` since it doesn't change ("update") the native bits at all. "Pack…" would also make sense but may be too specific.

@HaoK
Copy link
Member Author

HaoK commented Jan 20, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@HaoK
Copy link
Member Author

HaoK commented Jan 20, 2022

Okay so UpdateIisNativeAssets -> UseIisNativeAssets

@HaoK
Copy link
Member Author

HaoK commented Jan 21, 2022

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@HaoK HaoK changed the title BuildIisNativeProjects -> UpdateIisNativeAssets BuildIisNativeProjects -> UseIisNativeAssets Jan 21, 2022
@HaoK HaoK merged commit 8b93d9e into main Jan 21, 2022
@HaoK HaoK deleted the haok/buildnative branch January 21, 2022 18:16
@ghost ghost added this to the 7.0-preview1 milestone Jan 21, 2022
ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this pull request Jan 22, 2022
@HaoK @ShreyasJejurkar
BuildIisNativeProjects -> UseIisNativeAssets (dotnet#39655)
92a1f88
@BrennanConroy BrennanConroy mentioned this pull request Feb 4, 2022
Closed
@amcasey 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dougbu dougbu dougbu left review comments

@sebastienros sebastienros sebastienros approved these changes

@wtgodbe wtgodbe wtgodbe approved these changes

@Tratcher Tratcher Awaiting requested review from Tratcher

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@Pilchie Pilchie Awaiting requested review from Pilchie

Assignees
No one assigned
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions
Projects
None yet
Milestone
7.0-preview1
Development

Successfully merging this pull request may close these issues.

Rename BuildIisNativeProjects -> UpdateIISNativeAssets
6 participants
@HaoK @dougbu @sebastienros @wtgodbe @Pilchie @amcasey