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

Switch to NuGet packages for references. #66

Closed
wants to merge 1 commit into from

Conversation

KirillOsenkov
Copy link

No description provided.

@Krinkle
Copy link
Member

Krinkle commented Jun 10, 2020

I have not used NuGet before, but as I understand it is is considered modern and preferred to use it, so I'd be fine with that. I'm relatively inexperienced with C# and .NET, so the way I do this currently is probably unusual

Could you share a few words about how this helped you in building CVNBot? Did running msbuild directly not pick up the checked-in DLLs?

I'd prefer for the project to not require external input to compile the code, so checking into to version control would be preferred. Is there established way of using NuGet that accomodates these preferences?

@KirillOsenkov
Copy link
Author

When you currently clone the repo it doesn't build out of the box because:

  1. Mono.Data.SQlite is referenced, but is not checked in
  2. sqlite3.dll (native SQLite binary) is needed, but is not checked in

Checking in binaries into the repo is fine I guess, the usual concern is with checking in binaries into git they stay in the history forever and increase the clone size of the repo.

The way the .NET community is doing things nowadays is by using NuGet instead, with the default nuget.org feed.

Upon cloning the repo just pass the /r flag to MSBuild when building:

msbuild /r

It ensures that all NuGet packages are restored and ready, this way you don't need to check in binaries and also makes it easier to update to newer versions of dependencies. Might just want to document the need for the /r switch (which stands for "Restore"). It will only download the packages the first time if they're not already in the local NuGet cache (/Users/YourName/.nuget/packages).

@KirillOsenkov
Copy link
Author

Also shameless plug: if you have questions about .NET, C# or MSBuild, always feel free to ping me on Twitter: https://twitter.com/KirillOsenkov. Also if you're having MSBuild problems in the future, check out https://msbuildlog.com, this should make MSBuild investigations very easy.

Thanks for working on Wikipedia, I'm a huge fan!

@Krinkle
Copy link
Member

Krinkle commented Jun 10, 2020

When you currently clone the repo it doesn't build out of the box because:

  1. Mono.Data.SQlite is referenced, but is not checked in

  2. sqlite3.dll (native SQLite binary) is needed, but is not checked in

I'm sorry to push back as I'm the noob here, but I need a bit more detail.

  • The project builds in CI without issue.
  • The project builds on a fresh macOS install with only VS2019 and its Mono/.NET modules installed.
  • The project builds on a fresh Debian Linux host with only mono and msbuild installed.

I hadn't thought about this before, but it would seem Mono.Data.SQlite is a default part of Mono.

@KirillOsenkov
Copy link
Author

No problem at all. If I were to guess I think it does take these from the GAC on Mono, so it relies on these being installed on the machine with the runtime.

By supporting self-contained, hermetic, reproducible Windows builds you're increasing the chances that someone using Windows would be able to help, and ensuring that it's easier to migrate from Mono to .NET Core (or .NET in the future) if the need arises. Basically you can switch between .NET Framework, .NET Core and Mono then.

Btw I'm not pushing strongly for this PR either, so feel free to close, no problem.

@Krinkle Krinkle self-assigned this Jun 10, 2020
@Krinkle
Copy link
Member

Krinkle commented Jun 11, 2020

@KirillOsenkov Aha, so you mean that with this one dependency added, the project works fine as-is on dotnet, without Mono? That makes sense historically, it just hadn't ocurred to me that there would not be anything else Mono-specific about CVNBot, or indeed that its built-ins would be readilly available on NuGet for use outside Mono. That's cool!

If the dependency handling is all it takes right now I'd certainly be open to supporting that directly.

@Krinkle
Copy link
Member

Krinkle commented Jun 11, 2020

I've created a branch that adds dotnet as a test target:

However this fails currently, not on absence of Sqlite, but on absence of .NET Framework:

https://travis-ci.com/github/countervandalism/CVNBot/jobs/347363011

Searching around, I get the sense that .NET Framework is only for Windows, and not available for via NuGet or something like that for dotnet on Linux or Mac. Is that correct?

I'd prefer for this to be verified/integrated without Mono, but I suppose that could be done later.

Or perhaps we could even drop .NET Framework as dependency. I'm actually not sure how to know whether we are currently using anything that isn't part of .NET Core/Standard.

<SpecificVersion>False</SpecificVersion>
<HintPath>..\log4net.dll</HintPath>
</Reference>
<Reference Include="Meebey.SmartIrc4net, Version=1.1, Culture=neutral">
Copy link
Member

Choose a reason for hiding this comment

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

When I inspect the current embedded DLL for SmartIrc4net, VS2019 says it is actually version 0.4.5.0. But I'm not sure if I'm using interpreting this correctl and/or whether that is the one msbuild is indeed using.

Copy link
Author

Choose a reason for hiding this comment

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

This is correct. If you download the version 1.1 of the NuGet package and peek inside the .dll you'll notice that the version is 0.4.5.0 and the AssemblyInformationalVersion is 1.1.
image

@Krinkle
Copy link
Member

Krinkle commented Jun 11, 2020

If I understand correctly we can make it so that msbuild /r will install new dependencies (e.g. when adding or changing which packages are used), and that in general msbuild will only consider those already in the current repository.

I've spent a while trying to figure out how to tell msbuild (or NuGet) to store things in a specific directoy (e.g. ./packages) and to also know to read from there at build time.

I got the latter to work by using the following in a src/CVNBot/NuGet.Config file:

    <packageRestore>
        <add key="enabled" value="False" />
        <add key="automatic" value="False" />
    </packageRestore>

However, I'm not sure how to make an explicit msbuild /r invocation still fetch it and put it somewhere for subsequent invocations to pick up. Should I set globalPackagesFolder to a relative directory? That seems to work, but it also stores a lot of other files. I'm not sure what the "usual" way is for csharp projects that prefer to check in their DLLs.

@KirillOsenkov
Copy link
Author

Honestly if you prefer to check in the .dlls I'd not use PackageReference at all then. Sounds like you already have it configured such that it works well when using Mono. For .NET Core I'd just also check in sqlite3.dll and Mono.Data.SQlite.dll. Trying to get a mix of two worlds is uncharted territory and I wouldn't recommend it. Either check in everything or use NuGet and its defaults.

Now to clear up some unfortunate confusion about the various flavors of .NET.

.NET Framework is Windows-only, legacy, "original" runtime, it goes up to 4.8 and 4.8 is the last major version. It installs with Windows and will be supported for a long time. It supports .NETStandard 2.0 and below.

Mono is the runtime created by Xamarin, it works on all OSs and it also supports .NETStandard 2.0 and as a bonus most libraries targeting .NET Framework also run on Mono.

.NET Core is the future evolution of .NET, it works on all OSs, is being actively funded, developed and is the safest bet for investments. Starting with .NET 5 we will drop the version "5" and just call it .NET going forward. It will include what we now know as .NET Core and it will also include the Mono runtime in case you want to run on devices where only Mono can run. They will share all the libraries.

I wouldn't recommend bothering with .NET Framework, but supporting .NET Core/.NET going forward would be a plus.

Regarding configuring NuGet to restore into a local directory such as packages is honestly a bit of uncharted territory. The strong recommendation is to just use the default conventions, PackageReferences, and while it is all extremely customizable you're on your own there, as we're trying to push the ecosystem into a common set of conventions/standards adopted by everyone.

Hope this helps!

@KirillOsenkov
Copy link
Author

Oh and I just looked at the failure here:
https://travis-ci.com/github/countervandalism/CVNBot/jobs/347363011

It complains about the reference assemblies for .NET Framework 4.7.2 not being installed.

There are two ways to deal with this (if this was Windows you could just download the reference assemblies here: https://docs.microsoft.com/en-us/dotnet/framework/install/guide-for-developers)

  1. switch from targeting net472 to netcoreapp3.1 here:
    https://github.com/countervandalism/CVNBot/blob/db7dbfd3f7237d28ee12b877230c68efcf237af2/src/CVNBot/CVNBot.csproj#L4
    By targeting net472 you're targeting .NET Framework desktop (legacy), and while Mono supports this scenario out of the box so you don't have to do anything, with .NET Core it won't work if the reference assemblies aren't installed. By switching to netcoreapp3.1 you're explicitly declaring that you're targeting .NET Core and it won't need anything else other than .NET Core on the machine that you can get from https://dotnet.microsoft.com/download/dotnet-core/3.1

  2. perhaps an easier way is to just include the reference assemblies for net472 as a NuGet package, to make the build happy. You can do this by adding this line into an ItemGroup in the .csproj:

<PackageReference Include="Microsoft.NETFramework.ReferenceAssemblies" Version="1.0.0" />

@Krinkle Krinkle mentioned this pull request Jun 27, 2020
3 tasks
@Krinkle Krinkle closed this in 0c42401 Jun 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants