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

Updates PowerShell scripts #4006

Merged
merged 6 commits into from
Apr 21, 2021
Merged

Updates PowerShell scripts #4006

merged 6 commits into from
Apr 21, 2021

Conversation

dominoFire
Copy link
Contributor

@dominoFire dominoFire commented Apr 20, 2021 •
edited
Loading

Bug

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

Regression? No Last working version:

Description

  • Updates validation details
  • Adds a new environment variable in .\artifacts\configure.json
  • Removes fixed References and substitutes with PackageReferences
  • Moved 1 function from configure.ps1 to common.ps1

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • [ ] Automated tests added
    • [ ] Test exception
    • N/A: Changed local scripts. Validated it works on local machine.

  • Documentation

    • [ ] Documentation PR or issue filled
    • N/A: Engineering change

@dominoFire dominoFire requested a review from a team as a code owner April 20, 2021 00:17
erdembayar
erdembayar reviewed Apr 20, 2021
View reviewed changes
Copy link
Contributor

@erdembayar erdembayar left a comment

Choose a reason for hiding this comment

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

Thank you for fixing our build script.
I have 1 comment.

build/packages.targets
@@ -70,15 +70,17 @@
However, our MSBuild integration tests use Microsoft.Build 16.8.0, which requires System.Threading.Tasks.Dataflow 4.9.0 (assembly version 4.9.3.0).
To resolve runtime assembly binding failures, we'll downgrade the package from 4.11.1 to 4.9.0.
-->
<PackageReference Update="Microsoft.VisualStudio.VCProjectEngine" Version="16.9.31025.104" />
Copy link
Contributor

Choose a reason for hiding this comment

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

By removing condition for VS15, is build script stop supporting VS15?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory, yes. I tested building for VS15 with this change, and, it didn't work. Failures indicate a .NET Core SDK issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I leave the VS15 condition, it could work, but, we still need to address the .NET Core SDK Issue

Copy link
Member

Choose a reason for hiding this comment

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

We depend on changes to CPS & the .NET project system that are available only in d16. I think we're a long time past "not supporting" d15 from our dev branch. Our build scripts just haven't kept up.

Copy link
Member

@nkolev92 nkolev92 Apr 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It's potentially a change with some tail, but we should change our output location to VS only or something else.
VS15 referred to dev15. When we moved to dev16, we didn't change it because we didn't want to chase the tail.
I think we have an opportunity to change it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

So then do we need 15 in [ValidateSet(15, 16, 17)] now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since the script does not build for dev15, I will remove 15 in the set.

nkolev92
nkolev92 approved these changes Apr 20, 2021
View reviewed changes
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.

LGTM.

@dominoFire dominoFire requested review from erdembayar and zivkan April 21, 2021 01:54
@dominoFire dominoFire merged commit 1328b1d into dev Apr 21, 2021
@dominoFire dominoFire deleted the dev-dominofire-vs branch April 21, 2021 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkolev92 nkolev92 nkolev92 approved these changes

@zivkan zivkan zivkan approved these changes

@erdembayar erdembayar Awaiting requested review from erdembayar

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dominoFire @nkolev92 @zivkan @erdembayar