Skip to content

Conversation

@bording
Copy link
Collaborator

@bording bording commented Aug 25, 2016

This is what I've got so far to address #243. The VS solution is now fully buildable in Visual Studio again.

As mentioned in #243, I went ahead and removed the ServiceModel project instead of trying to update it.

For the WinRT project, it is still currrently a separate csproj project, because it turns out that the project.json/xproj "preview" tooling doesn't support building WinRT/UWP projects yet, even if you add a netcore45 framework target to the project.json file. Looks like making everything one project will have to wait for the final tooling they're still working on.

In the mean time, it seems like the best way to continue supporting WinRT would be to put out a separate package for it.

I noticed that the XmlUtil.cs seems to be excluded everywhere, and not referenced at all. Is there a reason to do that everywhere instead of just deleting the file?

Looking through the solution, there is a mix of paket and nuget, and several different build / packaging scripts. I'm not 100% sure on how this all interacts, so there's a chance something might need to be updated still to react to these changes.

@michaelklishin
Copy link
Contributor

Thank you!

@bording
Copy link
Collaborator Author

bording commented Aug 25, 2016

A couple other things:

  • It looks like there is still some bug with the NUnit VS Runner support, so the unit tests in the Unit project aren't actually runnable from inside of VS yet.
  • The main project.json file defines a "CORECLR" symbol, but that actually isn't necessary. Each target framework can be used directly. So "CORECLR" could be replaced by "NETSTANDARD1_5"

@bording
Copy link
Collaborator Author

bording commented Aug 25, 2016

Looks like swapping the order of the frameworks in the project.json file was enough to get the NUnit runner working in Visual Studio!

@kjnilsson
Copy link
Contributor

Thanks @bording - looks great so far. Let me know when you are ready for me to give it a proper test.

The Apigen projects haven't yet been converted to dotnet core which is why we have a mix of build tools still. Fake/paket should only be used to build and run the api gen. I tried converting them a while back but hit issues with missing apis so it may take a bit more of an effort.

I would prefer to wait until the dotnet core tooling is properly able to target WinRT rather than putting out a different package. Users of WinRT can still use 3.6.X packages which still will receive updates for some time.

If XmlUtil isn't actually used anywhere anymore and can be removed without breaking anything then go for it.

@bording
Copy link
Collaborator Author

bording commented Aug 26, 2016

Went ahead and pushed up a change to remove the XmlUtil class.

The Apigen projects haven't yet been converted to dotnet core which is why we have a mix of build tools still. Fake/paket should only be used to build and run the api gen. I tried converting them a while back but hit issues with missing apis so it may take a bit more of an effort.

It looks like CodeDom is the big blocker there, everything else appears to be available. There are a couple missin contuctor overloads in the XML stuff that would require some tweaks. The CodeDom stuff could probably be replaced with using Rosyln instead.

It looks like the WinRT unit test project is also still using paket for its NUnit dependency.

I would prefer to wait until the dotnet core tooling is properly able to target WinRT rather than putting out a different package. Users of WinRT can still use 3.6.X packages which still will receive updates for some time.

Makes sense. At least the VS solution is properly compiling those projects for now, so you can make sure something doesn't break in the mean time.

Is the tokenized AssemblyInfo.cs.in file in main Client project still used anywhere? If not, that's something else that could be removed.

If there isn't anything else you'd like me to change as part of this PR, I think it's ready to go.

@bording
Copy link
Collaborator Author

bording commented Aug 27, 2016

@kjnilsson FYI, I've got a couple more changes incoming after all. I've got all the Apigen stuff working in core now!

@bording
Copy link
Collaborator Author

bording commented Aug 27, 2016

Let me know what you think about the Apigen changes. I did end up formatting the document, but if you'd prefer I revert that, I can push up a version without it.

I replaced the need for CodeDom with a pretty simple switch statement. Turns out it was only being used to create return type names. I've confirmed that with my changes the generated file output is identical.

With these changes, I believe paket and fake are close to being able to be removed. Looks like there are still references to them in dist.sh, fake.bat, and fake.sh.

@bording
Copy link
Collaborator Author

bording commented Aug 27, 2016

Regarding the CI builds, I can see that Concourse is failing, but I can't see why. It would be nice if I could get a build log at least without having credentials!

For the AppVeyor build, it looks like appveyor.yml could be simplified a bit to remove the dotnet download and install, since they appear to include it by default: https://www.appveyor.com/updates/2016/06/27/

@bording
Copy link
Collaborator Author

bording commented Aug 27, 2016

Just had one more idea:

If we added something like

"files": {
      "mappings": {
        "lib/netcore45/": "../RabbitMQ.Client.WinRT/build/bin/RabbitMQ.Client.dll"
      }
    }

to the packOptions section of the RabbitMQ.Client project.json and then invoked MSBuild to build the RabbitMQ.Client.WinRT project beforehand, we could actually get the resulting dotnet pack call to build a package that includes the WinRT assembly as well.

It doesn't include netcore45 in the list of dependencies in the output nuspec, but trying it locally, it still seems to work fine. I was able to use the package in a Windows8.1 app and open a connection to a broker.

Copy link
Contributor

Choose a reason for hiding this comment

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

how does it use the AssemblyInformationalVersion from project.json?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If the AssemblyInformationalVersion attribute isn't hardcoded, then it uses the version value from the project.json file.

So, if you're keeping that version number up to date, then the nuget package output from dotnet pack will have the correct version, and the informational version will match as well.

Keeping the AssemblyVersion attribute as it is is desirable also, so that binding redirects won't be needed between minor/patch releases.

@kjnilsson
Copy link
Contributor

@bording regarding the CI builds we are looking at ways to publish the build logs so that they can be publicly accessible. Looking at the current failures it appears to be changes to Apigen and builds scripts that are breaking it. I am looking at finding a way to fix it for now.
Another thing to bear in mind is that we currently run all our CI on Linux so including WinRT at this stage may not be doable.

@kjnilsson
Copy link
Contributor

@bording to get ci working I've pushed a couple of changes to build.sh to master. Testing it atm but you will need to integrate those changes into this PR.

@bording
Copy link
Collaborator Author

bording commented Aug 30, 2016

@bording to get ci working I've pushed a couple of changes to build.sh to master. Testing it atm but you will need to integrate those changes into this PR.

Done!

@bording regarding the CI builds we are looking at ways to publish the build logs so that they can be publicly accessible.

👍

Another thing to bear in mind is that we currently run all our CI on Linux so including WinRT at this stage may not be doable.

Interesting. Are the actual shipping assemblies built from the Linux CI as well? Could you switch to AppVeyor to build the dotnet client, which should mean it would be possible to include WinRT?

@kjnilsson
Copy link
Contributor

Thank you @bording - this is great stuff.

Yes the shipped assemblies are built on linux. Our release process is done on concourse and there are options for Windows we just haven't looked into that yet.

@kjnilsson kjnilsson merged commit 8fb6a42 into rabbitmq:master Aug 31, 2016
@bording bording deleted the vs-support branch August 31, 2016 16:16
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.

3 participants