Remove extraneous Bcl .targets reference

[shana]

Looks like 38ef630 introduced a nuget warning/import to the Bcl targets. Pretty sure that should only exist on Portable projects?

[shiftkey]

It's weird, because in 38ef630 I've now installed Microsoft.Net.Http which depends on Microsoft.Bcl.Build and Microsoft.Bcl - but they're not referenced. But no references are updated in the csproj.

I think I need Microsoft.Net.Http here but perhaps I'm missing something...

[shana]

Maybe it only adds the references if it's a portable project? (I have no clue)
I merged this into my GHfVS branch and it's building/working happily, fyi.

[shiftkey]

So I'm pretty sure this is just the side-effect of me being lazy with updating packages in #1018. References have been a bit manual in the past so I updated the packages in 38ef630 - perhaps I didn't build-to-flush-the-changes-to-disk for that project (other projects have the same change).

But even though no assemblies are referenced in the main project, it's still adding in the EnsureNuGetPackageBuildImports here - which is technically not necessary here.

[shiftkey]

Maybe it only adds the references if it's a portable project? (I have no clue)

There's at least two assemblies in net45 which aren't referenced by the project:

shiftkey@BRENDANFORS36E0 ~/Documents/GitHub/octokit.net/packages/Microsoft.Net.Http.2.2.29/lib/net45 (master)
$ ls
ensureRedirect.xml System.Net.Http.Primitives.dll*
System.Net.Http.Extensions.dll* System.Net.Http.Primitives.xml
System.Net.Http.Extensions.XML

But they're not referenced explicitly, so perhaps I'm just lucky here.

[ryangribble]

Yeah I went on a rampage trying to fix this but wasn't sure about what this bcl thing is, and given the number of reference updates etc I ended up making, I didn't bother submitting the PR haha. Would be nice to get rid of the annoying errors

[shiftkey]

@ryangribble which errors are we talking about? The Microsoft.Bcl and Microsoft.Bcl.Build packages are there to help smooth over issues when targeting certain platforms - net45 is rather modern, so it's mostly acting as a no-op here. Happy to share more based on past experiences, I'm just not sure what we're looking for here...

[shana]

@shiftkey He probably bumped into the same problem I had - every single project in the solution that references Octokit suddenly wanted to have the Bcl, Bcl.Build and Http packages added to them, which amounts to a ton of build errors and quite a bit of changes. OTOH, removing this extraneous .targets reference was so much simpler...

[haacked]

@shiftkey He probably bumped into the same problem I had - every single project in the solution that references Octokit suddenly wanted to have the Bcl, Bcl.Build and Http packages added to them, which amounts to a ton of build errors and quite a bit of changes. OTOH, removing this extraneous .targets reference was so much simpler...

Yeah, and I don't think we need those anymore, do we? We can choose to not support the platforms that required them. 😄

[shiftkey]

@shana

every single project in the solution that references Octokit suddenly wanted to have the Bcl, Bcl.Build and Http packages added to them

Ah, that issue. If that's the case then I'm 👍 for this change. In fact, we could probably do away with the new packages specified here - I think that's also to blame here, and the nuspec should reflect the correct dependencies needed when installing the package...

[shana]

In fact, we could probably do away with the new packages specified here

Not sure which ones you mean? (it doesn't actually link to a specific point in the diff probably because it's at the bottom of the page and can't scroll down more haha we should probably fix that at some point sigh anyways...)

[shiftkey]

@shana it's meant to link to Octokit/packages.config but the browser can't scroll far enough 😢

[ryangribble]

@ryangribble which errors are we talking about?

Do you mean you dont see the below errors @shiftkey ? They dont actually stop the build, but they do show up as errors (Im using VS2015 Update 1 incase that's related)

Projects that reference Octokit.csproj say they want to have the Bcl.Build reference due to referencing the Octokit project that has that.

Then other errors complain that Octokit.Tests project doesnt have a packages config file (it's named packages.Octokit.Tests.config rather than packages.config). No doubt if it could find the right file the test projects would also complain about missing references that the Octokit.Tests project has...

Often I seem to have the errors from all of the sub projects (eg Reactive, Portable, NetCore45, Mono) but then today when I went to grab this screenshot, it's only Octokit.Reactive that's doing it. Very odd!

[shana]

@shiftkey Ah, indeed. I can certainly 🔥 those as well!

[shiftkey]

@ryangribble those errors are interesting.

The first one is totally legit, and something we should be able to resolve here, but the second and third ones seem new, because they're not resolving the valid configuration files in that folder. Guess we should try and log that upstream at least while we're in here dealing with this.

[ryangribble]

so do you not get those errors? What IDE are you using?

And is upstream in this case Visual Studio itself or nuget or this Build.Bcl package?

[shiftkey]

@ryangribble oh no, I get those errors. I just wasn't paying attention after updating.

Anyway, the issue seems to be inside Microsoft.Bcl.Build:

    <ValidatePackageReferences Packages="@(ValidatePackages)"
                               ReferencingProject="$(BclBuildReferencingProject)"
                               ReferencingProjectPackagesConfig="$(BclBuildReferencingProjectConfig)"
                               ReferencedProject="$(MSBuildProjectFullPath)"
                               ReferencedProjectPackagesConfig="$(MSBuildProjectDirectory)\packages.config"
                               TreatWarningsAsErrors="$(TreatWarningsAsErrors)" />

It seems like this doesn't care that packages.$(MSBuildProjectName).config is also a valid name - I can't find the official docs on this, which concerns me a bit...

[ryangribble]

So far the best I've found is somebody with the exact same problem on SO, but they've just hacked/patched the Bcl.Build.targets file, to include the $(MSBuildProjectName) in the path of the ReferencedProjectPackagesConfig. Which obviously isnt going to dynamically handle either packages.config or packages.$(MSBuildProjectName).config

http://stackoverflow.com/questions/32254228/installed-project-specific-nuget-packages-not-being-recognized

[shana]

@shiftkey I've killed the nuget references too. FWIW, I don't get the Octokit.Tests errors because I'm only building Octokit and Octokit.Reactive on VS (I trust you to run the tests 😉 )

[shiftkey]

@shana that's fine

[shiftkey]

@shana thanks for this! @ryangribble unfortunately this doesn't resolve the issues we're seeing.