Skip to content

Remove NuGet.SolutionRestoreManager.Interop from list#4195

Merged
heng-liu merged 1 commit intodevfrom
dev-hengliu-CI-releasePipelineScriptUpdate
Aug 13, 2021
Merged

Remove NuGet.SolutionRestoreManager.Interop from list#4195
heng-liu merged 1 commit intodevfrom
dev-hengliu-CI-releasePipelineScriptUpdate

Conversation

@heng-liu
Copy link
Contributor

@heng-liu heng-liu commented Aug 11, 2021

Bug

Fixes: https://github.com/NuGet/Client.Engineering/issues/1096

Regression? No

Description

Remove NuGet.SolutionRestoreManager.Interop from the checking list of scripts/utils/EnsureAllPackagesExist.ps1, as we no longer have/pack this project.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A
  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@heng-liu heng-liu requested a review from a team as a code owner August 11, 2021 17:24
@erdembayar
Copy link
Contributor

@heng-liu
If we merge this now then does it fix one failure I have now?

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

Would it be a good idea to have the check run after build as well? Rather than at release only?

That way we don't run into this issue again.

@erdembayar
Copy link
Contributor

erdembayar commented Aug 11, 2021

@heng-liu
It looks Microsoft.Build.NuGetSdkResolver exist in non-RTM only, but not in RTM. In 5.11 it used to be in both build. It looks we need to add condition for this. You can create separate issue to track it.

@NuGet/nuget-client Is removal of Microsoft.Build.NuGetSdkResolver from RTM was intentional?

@heng-liu
Copy link
Contributor Author

heng-liu commented Aug 11, 2021

Would it be a good idea to have the check run after build as well? Rather than at release only?

That way we don't run into this issue again.

Actually, we do check packages in every build at https://github.com/NuGet/NuGet.Client/blob/dev/eng/pipelines/templates/Build_and_UnitTest.yml#L227-L232.
But it is with another approach: #3852.
That is, checking the count of projects with <PackProject>true</PackProject>, and the generated .nupkg file and .symbols.nupkg files. If there is any mismatch, it will fail the build with a message.
Since there is no hardcoded checklist in this approach, the build doesn't fail (as expected, since all packages are generated successfully).

Shall we use the hardcoded checklist in both build pipeline and release pipeline, to make them consistent?
I will use the hardcoded checklist in both build pipeline and release pipeline, to make them consistent.
I'll do this in a separate PR, so that the publishing won't be blocked.

@heng-liu
Copy link
Contributor Author

@heng-liu
If we merge this now then does it fix one failure I have now?

For now, if this PR get merged, I guess it will fix the failure in the current release build if you redeploy it.
Since it's getting the script from dev branch (https://devdiv.visualstudio.com/DevDiv/_releaseDefinition?definitionId=257&_a=definition-tasks&environmentId=9031)

But soon we have to change the release pipeline to not get the script from dev branch (in case old serve branch also gets the checklist from dev branch, which doesn't reflect the fact of packages in serve branch). And in order to get the right version of the script, I need to work to finish this PR: #3926

@heng-liu
Copy link
Contributor Author

@heng-liu
It looks Microsoft.Build.NuGetSdkResolver exist in non-RTM only, but not in RTM. In 5.11 it used to be in both build. It looks we need to add condition for this. You can create separate issue to track it.

@NuGet/nuget-client Is removal of Microsoft.Build.NuGetSdkResolver from RTM was intentional?

May I know which build you are referring to? I just checked build 6.0.0.165, both RTM and nonRTM have the package Microsoft.Build.NuGetSdkResolver . I think the only difference between RTM and nonRTM is https://github.com/NuGet/NuGet.Client/blob/dev/scripts/utils/EnsureAllPackagesExist.ps1#L37-L41

@erdembayar
Copy link
Contributor

erdembayar commented Aug 12, 2021

Microsoft.Build.NuGetSdkResolver

Could you check please this asset list please? I can see difference between RTM and non-RTM for Microsoft.Build.NuGetSdkResolver.

It looks not an issue.

@erdembayar
Copy link
Contributor

erdembayar commented Aug 12, 2021

Microsoft.Build.NuGetSdkResolver

Could you check please this asset list please? I can see difference between RTM and non-RTM for Microsoft.Build.NuGetSdkResolver.

Sorry here is correct asset in question.

It looks not an issue.

@erdembayar erdembayar self-assigned this Aug 12, 2021
@heng-liu heng-liu merged commit 7d8988d into dev Aug 13, 2021
@heng-liu heng-liu deleted the dev-hengliu-CI-releasePipelineScriptUpdate branch August 13, 2021 00:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants