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.

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

corehost: align artifacts dir used by managed and native build. #74498

Closed
wants to merge 1 commit into from

Conversation

tmds
Copy link
Member

@tmds tmds commented Aug 24, 2022

Fixes build failure that occurs when OutputRid and __DistroRid are not the same.

cc @omajid

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Aug 24, 2022
@ghost
Copy link

ghost commented Aug 24, 2022

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes build failure that occurs when OutputRid and __DistroRid are not the same.

cc @omajid

Author: tmds
Assignees: -
Labels:

area-Host

Milestone: -

__DistroRidLower="$(echo $__DistroRid | tr '[:upper:]' '[:lower:]')"
__BinDir="$__RootBinDir/bin/$__DistroRidLower.$__BuildType"
__IntermediatesDir="$__RootBinDir/obj/$__DistroRidLower.$__BuildType"

export __BinDir __IntermediatesDir __RuntimeFlavor

__CMakeArgs="-DCLI_CMAKE_HOST_VER=\"$__host_ver\" -DCLI_CMAKE_COMMON_HOST_VER=\"$__apphost_ver\" -DCLI_CMAKE_HOST_FXR_VER=\"$__fxr_ver\" $__CMakeArgs"
__CMakeArgs="-DCLI_CMAKE_HOST_POLICY_VER=\"$__policy_ver\" -DCLI_CMAKE_PKG_RID=\"$__DistroRid\" -DCLI_CMAKE_COMMIT_HASH=\"$__commit_hash\" $__CMakeArgs"
__CMakeArgs="-DCLI_CMAKE_HOST_POLICY_VER=\"$__policy_ver\" -DCLI_CMAKE_PKG_RID=\"$__PackageRid\" -DCLI_CMAKE_COMMIT_HASH=\"$__commit_hash\" $__CMakeArgs"
Copy link
Member Author

Choose a reason for hiding this comment

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

While changing this file, I noticed this use of __DistroRid. From the name of the property, I assume we want to pass in the PackageRid.

@@ -231,7 +231,7 @@
</PropertyGroup>

<PropertyGroup>
<DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(OutputRid).$(Configuration)', 'corehost'))</DotNetHostBinDir>
<DotNetHostBinDir>$([MSBuild]::NormalizeDirectory('$(ArtifactsBinDir)', '$(__DistroRid.ToLowerInvariant()).$(Configuration)', 'corehost'))</DotNetHostBinDir>
Copy link
Member Author

Choose a reason for hiding this comment

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

This now matches with the logic in the corehost/build.sh script.

@tmds tmds mentioned this pull request Aug 24, 2022
@ViktorHofer ViktorHofer requested a review from am11 August 24, 2022 13:17
@am11
Copy link
Member

am11 commented Aug 24, 2022

Fixes build failure that occurs when OutputRid and __DistroRid are not the same.

Do you mean "Add support for .." or is it an existing scenario which was working before and broke by some other change?

@tmds
Copy link
Member Author

tmds commented Aug 24, 2022

Do you mean "Add support for .." or is it an existing scenario which was working before and broke by some other change?

I'm not aware this was working before. I think we're the first to try set TargetRid to something other than distro rid and find out the corehost artifacts path gets calculated differently in these files.

@tmds
Copy link
Member Author

tmds commented Aug 25, 2022

@am11 I've created an issue: #74577. #74504 supersedes this PR.

@tmds tmds closed this Aug 25, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Sep 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Host 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.

2 participants