Skip to content
This repository has been archived by the owner on Dec 18, 2018. It is now read-only.

Use PackageLineup to manage versions #1996

Merged
merged 1 commit into from
Aug 16, 2017
Merged

Conversation

natemcmaster
Copy link
Contributor

@natemcmaster natemcmaster commented Aug 14, 2017

Changes:

New workflow:
When cloning this repo, you must execute build.cmd /t:Pin or build /t:Restore first before the repo can be used in Visual Studio/Code. This step generates the PackageReference versions. VS will display an error message if you don't.

To update to the latest packages, run build.cmd /t:Pin or build /t:Restore again.

cc @CesarBS @halter73 @pakrym @muratg

See aspnet/KoreBuild#239
See also https://github.com/aspnet/BuildTools/wiki/%5Bspec%5D-Package-version-lineups

@benaadams
Copy link
Contributor

New workflow: ....

Add it to Readme.md?

@natemcmaster
Copy link
Contributor Author

This uncovered at least one interesting issue: aspnet/BuildTools#369. On CI only (not local builds), the restore call to roslyn's myget feed hangs for 100s. It can't reproduce locally. It doesn't cause restore to fail, but makes CI builds slower. I'll investigate

@natemcmaster
Copy link
Contributor Author

Add it to Readme.md?

That's a good idea. FYI if this experiment succeeds in solving our issues with managing PackageRef versions, then we'll be updating the most repos to this and adding this as general guidance to our engineering wiki page. That said, I'm hoping an appropriate error will be show up in VS and is clear enough that contributors don't have to hunt for a solution. I've found people tend to ignore README files, esp. if they're regular contributors already.

@natemcmaster
Copy link
Contributor Author

This uncovered at least one interesting issue: aspnet/BuildTools#369

Nevermind. Appears to have been random network issue with myget.

@benaadams
Copy link
Contributor

people tend to ignore README files, esp. if they're regular contributors already.

They start as new contributors first 😉

@natemcmaster
Copy link
Contributor Author

I updated the README.

Copy link
Contributor

@ryanbrandenburg ryanbrandenburg left a comment

Choose a reason for hiding this comment

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

Looks good.

<PackageReference Include="Microsoft.NETCore.Compilers" Version="2.6.0-rdonly-ref-61915-01" PrivateAssets="All">
<!--
Suppresses the warning about having an explicit package version in this repo.
We are okay with this for now as the experimental version of the compiler is unique to Kestrel (for now).
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we get a link to an issue here describing when we'll no longer be ok with it "for now"?

Copy link
Contributor Author

@natemcmaster natemcmaster Aug 15, 2017

Choose a reason for hiding this comment

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

@pakrym you wrote aspnet/Announcements#268. Any idea when features/readonly-ref will be merged in roslyn? Is there a good issue we can use to track progress on this?

cc @VSadov

Copy link
Contributor

@pakrym pakrym Aug 15, 2017

Choose a reason for hiding this comment

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

I've heard no exact dates, we can track dotnet/csharplang#666

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I've added this to the comment.

@@ -17,8 +15,7 @@
</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Logging.Console" Version="$(AspNetCoreVersion)" />
<PackageReference Include="Microsoft.NETCore.Compilers" Version="$(MicrosoftNETCoreCompilersVersion)" PrivateAssets="All" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Totally unrelated to your PR, but it would be nice if SOMETHING along the line of our toolchain gave a warning when you had a reference that you don't use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pakrym built something close to this. But we haven't turned it on by default because it doesn't catch everything. i.e. some packages, like Microsoft.NETCore.Compilers, contribute build-time only assets to the build which don't show up in the final artifacts.

https://github.com/aspnet/BuildTools/blob/da73e105376f5b980593c1519e58d8bcde8fdb65/src/Internal.AspNetCore.Sdk/build/FindUnusedReferences.targets#L6

@@ -0,0 +1,7 @@
<Project>
Copy link
Contributor

Choose a reason for hiding this comment

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

So these Directory.Build.props and *.targets files let us describe per-directory behavior? Neat.

<PackageReference Include="Microsoft.Extensions.Logging.Abstractions" Version="$(AspNetCoreVersion)" />
<PackageReference Include="Microsoft.Extensions.Options" Version="$(AspNetCoreVersion)" />
<PackageReference Include="System.Threading.Tasks.Extensions" Version="$(CoreFxVersion)" />
<PackageReference Include="Microsoft.AspNetCore.Hosting.Abstractions" />
Copy link
Contributor

Choose a reason for hiding this comment

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

It's happening!

@@ -0,0 +1,23 @@
<Project>
<Import Project="..\Directory.Build.props" />
Copy link
Contributor

Choose a reason for hiding this comment

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

So Directory.Build.props files will not chain themselves by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. The Directory.Build import only finds the first file in or above the project directory. We have to manually chain beyond that.

@NickCraver
Copy link
Contributor

By convention, I almost universally expect this to work (across all the .NET Core projects):

  1. get clone
  2. build.cmd

IMO, this initial requirement or args would be a lot more palatable if it was instead inferred in some way. Is there nothing we can check for to see if this should be automatically called (inside the script), making build.cmd work as it does today, without args for the user?

It's also a little doubly confusing I think, since we just announced to all users the .NET Core 2 bits auto-restored everywhere they needed to. Yes, that's a different thing, but it's a backwards set of reality and messaging overall (especially if this spreads to other repos).

@mikeharder
Copy link
Contributor

mikeharder commented Aug 16, 2017

@NickCraver: @natemcmaster can confirm, but I'm pretty sure git clone; build.cmd should continue to work as it does today. What is changing is that solutions cannot be opened in Visual Studio before running build.cmd /t:Pin or build /t:Restore from the command line.

@natemcmaster
Copy link
Contributor Author

Correct. build.cmd work the same before and after this change. The VS workflow is the only piece that changes.

By the way, some clarity on /t:Pin vs /t:Restore.

/t:Restore depends on /t:Pin and executes a "dotnet restore".
/t:Pin only pins package reference versions but does not execute restore.

/t:Pin is most useful when you have VS open already and have automatic restore enabled. Otherwise, you can get contention on writing to project.assets.json

PackageLineup is a way to manage PackageReference versions across large projects. It removes the version information from the repository and instead pulls the information from an external "lineup" file.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants