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

"PackageReference" Adoption #9664

Closed
wants to merge 1 commit into from
Closed

"PackageReference" Adoption #9664

wants to merge 1 commit into from

Conversation

QilongTang
Copy link
Contributor

@QilongTang QilongTang commented Apr 17, 2019

Please Note:

  1. Before submitting the PR, please review How to Contribute to Dynamo
  2. Dynamo Team will meet 1x a month to review PRs found on Github (Issues will be handled separately)
  3. PRs will be reviewed from oldest to newest
  4. If a reviewed PR requires changes by the owner, the owner of the PR has 30 days to respond. If the PR has seen no activity by the next session, it will be either closed by the team or depending on its utility will be taken over by someone on the team
  5. PRs should use either Dynamo's default PR template or one of these other template options in order to be considered for review.
  6. PRs that do not have one of the Dynamo PR templates completely filled out with all declarations satisfied will not be reviewed by the Dynamo team.
  7. PRs made to the DynamoRevit repo will need to be cherry-picked into all the DynamoRevit Release branches that Dynamo supports. Contributors will be responsible for cherry-picking their reviewed commits to the other branches after a LGTM label is added to the PR.

Purpose

This is magic, makes the project config so much easier to read! And verified project still builds fine locally with local cache of all the dependecies. We should totally do this.

Notice there is way to migrate legacy packages.config in Visual Studio 2017 and can be opened correctly in VS2017 only according to https://docs.microsoft.com/en-us/nuget/reference/migrate-packages-config-to-package-reference#limitations.

A team decision we need to make!

image

Declarations

Check these if you believe they are true

  • The code base is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning, and are documented in the API Changes document.

Reviewers

@mjkkirschner @scottmitchell

FYIs

@DynamoDS/dynamo

@QilongTang QilongTang changed the title Magic PackageReference Adoption Magic "PackageReference" Adoption Apr 17, 2019
@QilongTang
Copy link
Contributor Author

QilongTang commented Apr 17, 2019

My only concern is how it is going to affect our CI. If all machines are updated with VS2017, we should be fine though but how about 3rd party developers?

@QilongTang QilongTang changed the title Magic "PackageReference" Adoption "PackageReference" Adoption Apr 17, 2019
@mjkkirschner
Copy link
Member

@QilongTang thanks for checking this out... this warning kind of scares me:
Some packages may not be fully compatible with PackageReference..... is there a big benefit to doing this now? It's also unsupported by c++projects it seems.

@mjkkirschner
Copy link
Member

mjkkirschner commented Apr 17, 2019

@QilongTang I believe we will also hit there in some of our repos:

https://docs.microsoft.com/en-us/nuget/reference/migrate-packages-config-to-package-reference#package-compatibility-issues - I recall some install.ps1 scripts that run after the nuget restore - but I cannot remember which repos we used it in.

Is it worth using this in a few spots? I feel if we do it we should atleast be consistent throughout entire repos at a time.

@scottmitchell
Copy link
Collaborator

@QilongTang This is cool, but I don't fully understand the benefits.

@QilongTang
Copy link
Contributor Author

@scottmitchell @mjkkirschner Dealing with packages in a single center location has huge benefit

  1. Takes much less disk space since all nuget packages will be stored in a single folder instead of different packages folders under each solution folder
  2. Benefit branch build process hugely. If you recall, switching release branches in Dynamo and DynamoRevit give people such a pain because they have to delete all the packages downloaded near the solution and rebuild again. Doing this theoretically people can switched between branches and just build because references will all be fetched from central location, off course when each branch is set up that way. But we will not get the benefit without starting from now.
  3. csproj files become much easier to read because we can only include the direct dependencies.

I actually do not agree that we should only do it when we want to align all the slns, that sounds like never to me. For internal repo which are mostly used by our team, we should strike for efficiency.

@mjkkirschner
Copy link
Member

@QilongTang seems we can close this now?

@QilongTang
Copy link
Contributor Author

@mjkkirschner Yes, finally

@QilongTang QilongTang closed this Nov 4, 2020
@QilongTang QilongTang deleted the UsePackageReference branch July 14, 2021 13:11
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.

None yet

3 participants