Skip to content

Conversation

@am11
Copy link
Member

@am11 am11 commented Aug 17, 2022

FIx #74060

@ghost ghost added area-Infrastructure-libraries community-contribution Indicates that the PR has been added by a community member labels Aug 17, 2022
@ghost
Copy link

ghost commented Aug 17, 2022

Tagging subscribers to this area: @dotnet/area-infrastructure-libraries
See info in area-owners.md if you want to be subscribed.

Issue Details

FIx dotnet/dotnet-api-docs#8315

Author: am11
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@krwq

This comment was marked as outdated.

@krwq

This comment was marked as outdated.

@am11
Copy link
Member Author

am11 commented Aug 17, 2022

Yup, it was a wrong link, :)

@EgorBo, could you test this patch?

@am11 am11 force-pushed the feature/build/corehost branch from 7143b83 to b1c44b7 Compare August 17, 2022 11:37
@am11 am11 changed the title Build native.rc only in release build Build native.rc only in release configuration Aug 17, 2022
@am11 am11 force-pushed the feature/build/corehost branch from b1c44b7 to 20b75b8 Compare August 17, 2022 11:38
@ghost
Copy link

ghost commented Aug 17, 2022

Tagging subscribers to this area: @hoyosjs
See info in area-owners.md if you want to be subscribed.

Issue Details

FIx #74060

Author: am11
Assignees: -
Labels:

area-Infrastructure-coreclr, community-contribution

Milestone: -

@am11 am11 marked this pull request as ready for review August 17, 2022 11:48
@EgorBo
Copy link
Member

EgorBo commented Aug 17, 2022

@am11 awesome! just checked and it works!

@am11 am11 requested a review from jkoritzinsky August 17, 2022 12:36
@jkotas
Copy link
Member

jkotas commented Aug 17, 2022

Build break:

packages\microsoft.dotnet.sharedframework.sdk\7.0.0-beta.22411.2\targets\sharedfx.targets(510,5): error : (NETCORE_ENGINEERING_TELEMETRY=Build) Missing FileVersion in 2 shared framework files:
D:\a\_work\1\s\artifacts\bin\win-x86.Debug\corehost\/hostpolicy.dll
D:\a\_work\1\s\artifacts\bin\win-x86.Debug\corehost\/hostfxr.dll

I do not think that diverging debug and release builds this way is a good idea.

@am11
Copy link
Member Author

am11 commented Aug 17, 2022

Yes, I am looking into it.

@am11 am11 force-pushed the feature/build/corehost branch from 20b75b8 to 5ee2cdf Compare August 17, 2022 13:34
@am11 am11 changed the title Build native.rc only in release configuration Omit native.rc from checked configuration Aug 17, 2022
@am11
Copy link
Member Author

am11 commented Aug 17, 2022

Unlike Unix, UPPERCASE_CMAKE_BUILD_TYPE is never set to CHECKED on Windows because we are missing -DCMAKE_BUILD_TYPE propagation in eng/native/gen-buildsys.cmd. I think propagating original configuration to cmake as we do for Unix will fix this issue without skipping.

@am11 am11 marked this pull request as draft August 17, 2022 15:20
@elinor-fung
Copy link
Member

Is this just a discrepancy in where the version file gets generated (under checked since Configuration=Checked) versus what we add to the include path for resource files (under Debug since CMAKE_BUILD_TYPE=Debug)?

NativeVersionFile=$(IntermediateOutputRootPath)hostResourceFiles\%(HostFiles.Identity)\version_info.h"

set "__ResourcesDir=%__objDir%\%__TargetRid%.%CMAKE_BUILD_TYPE%\hostResourceFiles"

If so, I think we should reconcile that rather than skipping the native.rc

@am11
Copy link
Member Author

am11 commented Aug 17, 2022

@elinor-fung we both commented at the same time. I found the rootcause why it only breaks on Windows.

@am11 am11 changed the title Omit native.rc from checked configuration Set CMAKE_BUILD_TYPE=Checked in corehost Aug 17, 2022
@am11 am11 force-pushed the feature/build/corehost branch from 5ee2cdf to 3fc49dd Compare August 17, 2022 18:32
@am11 am11 marked this pull request as ready for review August 17, 2022 18:32
@am11
Copy link
Member Author

am11 commented Aug 17, 2022

@EgorBo, @elinor-fung, this is a different fix. I haven't centralize -DCMAKE_BUILD_TYPE for Windows in eng/native/gen-buildsys.cmd (as we have in ng/native/gen-buildsys.sh), because --subset libs+mono were running into some issues. (would be nice to work those out and get these aligned at some point to reduce disparities)

@EgorBo
Copy link
Member

EgorBo commented Aug 18, 2022

Thanks for fixing this! It would be nice if someone could review this and merge asap, thanks

@elinor-fung elinor-fung merged commit 549abac into dotnet:main Aug 18, 2022
@elinor-fung
Copy link
Member

Thanks, @am11!

@ghost ghost locked as resolved and limited conversation to collaborators Sep 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Infrastructure-coreclr community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cannot open include file 'version_info.h'

5 participants