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

Prefer OutputRid over PackageRID for output artifacts. #76871

Merged
merged 3 commits into from
Nov 16, 2022

Conversation

tmds
Copy link
Member

@tmds tmds commented Oct 11, 2022

This is a non-functional change that makes it more visible these are output artifacts.

@ViktorHofer @am11 ptal.

This is a non-functional change that makes it more visible
these are output artifacts.
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Oct 11, 2022
@ghost
Copy link

ghost commented Oct 11, 2022

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

Issue Details

This is a non-functional change that makes it more visible these are output artifacts.

@ViktorHofer @am11 ptal.

Author: tmds
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

@tmds
Copy link
Member Author

tmds commented Oct 11, 2022

I was triggered to make this change because I'd rather see microsoft.netcore.app.runtime.fedora.36-x64 under artifacts/bin for a non-portable build than microsoft.netcore.app.runtime.linux-x64.

@ViktorHofer @am11 ptal.

CI should take a look first to verify I didn't break anything.

@@ -63,10 +63,10 @@
Outputs="$(MicrosoftNetCoreAppRuntimePackDir)data\PlatformManifest.txt"
Condition="'$(BuildTargetFramework)' == '$(NetCoreAppCurrent)' or '$(BuildTargetFramework)' == ''">
<GenerateFileVersionProps Files="@(SharedFrameworkRuntimeFile)"
PackageId="$(MicrosoftNetCoreAppFrameworkName).Runtime.$(PackageRID)"
PackageId="$(MicrosoftNetCoreAppFrameworkName).Runtime.$(OutputRid)"
Copy link
Member

Choose a reason for hiding this comment

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

This changes the non-shipping "fake" runtime pack id. Do we also make changes to the actual shipping runtime pack sfxproj? I'm suprised that there aren't any changes to the sfxprojs.

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 wasn't aware there's a fake runtime pack id, and a shipping runtime pack id.
Do these represent different builds?

I assume the sfxprojs pick up the MicrosoftNetCoreAppRuntimePack* properties?

Copy link
Member

Choose a reason for hiding this comment

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

No, both use the same assets underneath but the composition of them is different (predates the repository consolidation).

I assume the sfxprojs pick up the MicrosoftNetCoreAppRuntimePack* properties?

Unsure about that. Please double check. You should be able to build the sfxprojs via the packs subset.

@tmds
Copy link
Member Author

tmds commented Oct 11, 2022

CI doesn't look happy. There seem to be many NuGet issues.

@tmds
Copy link
Member Author

tmds commented Oct 19, 2022

CI is looking much better. There are 2 jobs that have timed out. @ViktorHofer do you want to retrigger those, or run the whole thing?

I'm not sure what I should validate for #76871 (comment). Is green CI enough?

Can we trigger CI also for the unknown Banana rid? Azure shows it greyed out, so I think it doesn't run by default.

@tmds
Copy link
Member Author

tmds commented Oct 25, 2022

ping @ViktorHofer

@tmds
Copy link
Member Author

tmds commented Nov 8, 2022

@ViktorHofer can you take a look?

@ViktorHofer
Copy link
Member

We need to retrigger CI as the previous build isn't available anymore.

@ViktorHofer ViktorHofer closed this Nov 8, 2022
@ViktorHofer ViktorHofer reopened this Nov 8, 2022
@ViktorHofer
Copy link
Member

/azp run runtime

@ViktorHofer
Copy link
Member

/azp run runtime-dev-innerloop

@ViktorHofer
Copy link
Member

/azp run runtime-staging

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@ViktorHofer
Copy link
Member

/azp run dotnet-linker-tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tmds
Copy link
Member Author

tmds commented Nov 9, 2022

The unsuccessful jobs failed in the 'Send to Helix' step. I don't see any build or test failures related to the PR changes.

@ViktorHofer
Copy link
Member

ViktorHofer commented Nov 11, 2022

Sorry that it took me longer to reply. Looking into this comment right now:

I'm not sure what I should validate for #76871 (comment). Is green CI enough?
Can we trigger CI also for the unknown Banana rid? Azure shows it greyed out, so I think it doesn't run by default.

Need to figure out where the RID is encoded and how to trigger it.

@ViktorHofer
Copy link
Member

I couldn't find a good way to trigger the banana rid as it had a condition on it to only run for rolling builds. I just removed that condition to get CI validation. We can revert my commit before merging.

@tmds
Copy link
Member Author

tmds commented Nov 14, 2022

The banana rid build passed: https://dev.azure.com/dnceng-public/public/_build/results?buildId=80118&view=logs&j=98fdc254-13b3-5066-1dc8-532bea57cf83. Other CI jobs look good too.

I removed the commit that triggered the banana rid job.

This should be good to merge.

@ViktorHofer ViktorHofer merged commit 2a31a5b into dotnet:main Nov 16, 2022
@ViktorHofer
Copy link
Member

Thanks @tmds

@ghost ghost locked as resolved and limited conversation to collaborators Dec 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Infrastructure-libraries 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