Improve MSBuild integration

sliekens commented 8 years ago

Roundhouse's integration with MSBuild works, but it's not great right now. Here's my bullet list of suggested enhancements:

Code reuse: the Roundhouse task class should extend Microsoft.Build.Utilities.Task
Error handling: uncaught exceptions should not be allowed to bubble up to the MSBuild runner: MSBuild has a logging engine that is also its primary error handing mechanism.
log4net: this dependency is causing a lot of issues with the NuGet package. It should be factored out as suggested in #19.

ferventcoder commented 8 years ago

log4net: this dependency is causing a lot of issues with the NuGet package. It should be factored out as suggested in #19.

Not removed, perhaps ILMerged though. Since it is not necessary for MSBuild and we are already hooking up an MSBuild logger. It does serve the purpose of actually logging back to a file, which may be why I wanted to prefer the log call versus the loggingHelper.

sliekens commented 8 years ago

... and we are already hooking up an MSBuild logger.

About that: the MSBuildLogger class doesn't currently log any information about the build script. Instead it uses meaningless defaults (0 or string.Empty) for things like SubCategory, Code, File, LineNumber, ColumnNumber, EndLineNumber, EndColumnNumber, Message, HelpKeyword, and SenderName.

Maybe the MSBuildLogger class should be converted to an adapter for the TaskLoggingHelper class?

public sealed class MSBuildLogger : Logger
{
    private readonly TaskLoggingHelper helper;
    private readonly object calling_task;

    MSBuildLogger(ConfigurationPropertyHolder configuration)
    {
        var task = configuration as ITask;
        if (task != null)
        {
            helper = new TaskLoggingHelper(task);
        }

        calling_task = configuration;
    }
}

sliekens commented 8 years ago

log4net: this dependency is causing a lot of issues with the NuGet package. It should be factored out as suggested in #19.

There is a workaround that goes like this: first install Log4net 1.2.11, then install roundhouse.msbuild 0.8.6 and ignore any warnings regarding the Log4net dependency.

Install-Package log4net -Version 2.0.0
Install-Package roundhouse.msbuild -IgnoreDependencies

This is necessary because the roundhouse.msbuild package depends on an earlier version of log4net (1.2.10.0) than is required by the roundhouse.dll assembly (1.2.11.0).

The reason why it doesn't "just" work, or why you can't use a <bindingRedirect>, is because log4net changed their PublicKeyToken

log4net, Version=1.2.10.0, Culture=neutral, PublicKeyToken=1b44e1d426115821
log4net, Version=1.2.11.0, Culture=neutral, PublicKeyToken=669e0ddf0bb1aa2a

Note that the log4net package version is 2.0.0, but the containing assembly is 1.2.11.0.

Bottom line is: I think the package needs to be updated to depend on log4net 2.0.0, and that should fix mostly everything.

sliekens commented 8 years ago

Add NuGet 2.5+ build integration features to the list of suggested enhancements. https://docs.nuget.org/release-notes/nuget-2.5#automatic-import-of-msbuild-targets-and-props-files

ferventcoder commented 8 years ago

There is a workaround that goes like this: first install Log4net 1.2.11, then install roundhouse.msbuild 0.8.6 and ignore any warnings regarding the Log4net dependency.

I guess what I'm saying that may have been missed, if log4net isn't necessary for msbuild (I don't think it is), and it gets ILMerged into the assembly, there is no NuGet dependency.

ferventcoder commented 8 years ago

Add NuGet 2.5+ build integration features to the list of suggested enhancements. https://docs.nuget.org/release-notes/nuget-2.5#automatic-import-of-msbuild-targets-and-props-files

Nice, not sure how I missed that - this is how I approached it in the past https://github.com/ferventcoder/nugetpackages/blob/master/PublishedApplications/tools/install.ps1#L13-L16

sliekens commented 8 years ago

I guess what I'm saying that may have been missed, if log4net isn't necessary for msbuild (I don't think it is), and it gets ILMerged into the assembly, there is no NuGet dependency.

That should work. The roundhouse.dll assembly depends on log4net, but the roundhouse.tasks.dll assembly does not.

sliekens commented 8 years ago

Nice, not sure how I missed that - this is how I approached it in the past

NuGet 2.5 was released on April 25, 2013 :)

ferventcoder commented 8 years ago

@StevenLiekens I don't catch all of the updates. I didn't realize there was a switch over to XDT as the preferred way of handling config transforms until more recently either. :)

sliekens commented 8 years ago

Oh, I didn't mean it like that. I was just pointing out that this feature wasn't available when the code was written back in 2011.

I'm wondering if there is a similar solution for the roundhouse nupkg that contains the executable file. Installing that package adds rh.exe somewhere to your solution folder, but building the solution doesn't copy rh.exe to the output directory. It would be more useful if Install.ps1 patches the project file with something like this:

<None Include="..\packages\roundhouse.0.8.6\bin\rh.exe">
  <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
</None>

ferventcoder commented 8 years ago

Hmm, I think that would be an opt-in nuget package that is separate than the tools one. Mostly because folks may not want that behavior

sliekens commented 8 years ago

My biggest concern is that updating the package causes the toolpath to change, because NuGet embeds the version number in the path (as with "packages\roundhouse.0.8.6\bin\rh.exe").

If this isn't a problem already, then I'd imagine it's only because of infrequent releases (biyearly).

ferventcoder commented 8 years ago

Yeah, that would also be a concern. Let's open a separate issue on that.

sliekens commented 8 years ago

I can't. I don't know enough about the problem that the package is meant to solve.

I can think of these two use cases:

Allow using rh.exe instead of the msbuild task
Don't kick the database at build-time, but instead copy rh.exe to a deployment package that can be handed off to a DBA

I'm sure that people are using the package in ways that I didn't think of.

I'm trying to do the latter, but keep getting stuck trying to resolve the toolpath.

ferventcoder commented 8 years ago

Don't kick the database at build-time

I imagine by build you mean running msbuild? The MSBuild task is not meant to be used for integration into Visual Studio or into scripts you run during build cycles. Instead it is meant to be run for scripts that you run as part of deployment cycles.

copy rh.exe to a deployment package that can be handed off to a DBA

So deployment package means as like another nuget/choco package that can be used by those folks?

In a few different companies we had ways of doing things that would be for deployment, but in short, the process you use locally should be the same process throughout the entire pipeline. So if you are using the msbuild task for deployment locally (deployment msbuild tasks, not build msbuild tasks), then that should be what is handed over to your DBAs to run as well.

Lots of reasons for this, but the biggest reason is that you are continuously testing your database scripts / deployment scripts so there are no surprises when you go to production.

ferventcoder commented 8 years ago

Biggest reasons I'm asking for clarification - rh.exe is not necessary for msbuild deployment tasks, just the dlls that come with it.

sliekens commented 8 years ago

The MSBuild task is not meant to be used for integration into Visual Studio or into scripts you run during build cycles. Instead it is meant to be run for scripts that you run as part of deployment cycles.

We don't use RH in visual studio, but we do use it as a part of our continuous integration (nightly) builds. The CI build doesn't make that difference between build and deployment cycles. It's a single MSBuild script that builds and then deploys everything to a test environment. The Roundhouse task is ran in the final build step at the end of each successful build.

So deployment package means as like another nuget/choco package that can be used by those folks?

I suppose so. This package is for handing off to our DBA guy as part of the deployment cycle at the end of each sprint. A deployment package might be a zip file that contains rh.exe, a wrapper DBDeployment.bat file and the actual migration scripts.

I can't use the msbuild task for this, because our DBA wouldn't have the slightest clue of how to run it. I have to dumb it down to a single file that can be double clicked to run.

(I also don't have permissions to run the migration myself. Security is very tight for the live environment.)

ferventcoder commented 8 years ago

I can't use the msbuild task for this, because our DBA wouldn't have the slightest clue of how to run it. I have to dumb it down to a single file that can be double clicked to run.

Not sure I like the connotation here, but msbuild can be achieved as a file that can be double-clicked to run, that's exactly what we did at one place. You have your .cmd/.bat file sit next to the deployment files that has the proper things in it. Examples - https://github.com/chucknorris/roundhouse/tree/master/deployment/templates (these just use rh.exe, but one could do whatever they want in there). Note these make use of uppercut and uppercut.settings files to create one of these for each environment.

sliekens commented 8 years ago

Yeah, you're right, it can be done. But then you have to make assumptions about which version of MSBuild (if any) is installed on the machine that will run the migration. As opposed to rh.exe, which I can include in my package.

ferventcoder commented 8 years ago

Yeah, you're right, it can be done. But then you have to make assumptions about which version of MSBuild (if any) is installed on the machine that will run the migration. As opposed to rh.exe, which I can include in my package.

I wasn't saying it was a great solution, just that it was there. I am also of the mind that you use the same migration strategy all the way up to PRODUCTION. By that I mean not just RoundhousE, I mean near exactly the same scripts with Environment differences being the only changes. MsBuild RH versus RH.exe is technically different migration strategies and might get you into trouble. The only difference I usually make is locally, for the sake of speed, using the builddatabase tool, which also has the nice effect of creating your scripts for you that will be used later up the environment chain. But that usually means there are DEV, TEST, and QA/STAGING environments along the way up to PROD to test those changes.

The other minor difference along the way up is to use a backup of PROD, even if just a structure backup with minimal/sensitive-data cleaned data to restore the other environments from. Principal of least surprise is paramount. Software development is already hard enough, no reason to make it harder.

sliekens commented 8 years ago

We decided on using the executable (rh.exe) for all manual deployments. Besides that we have a build server that runs the Roundhouse task (msbuild) as part of nightly builds. This last part is nice because when the scripts contain errors, they cause the build to fail.

On-topic: can #151 get merged in? Right now, the compiler complains when you build a solution for AnyCPU and it uses the roundhouse task.

warning MSB3270: There was a mismatch between the processor architecture of the project being built "MSIL" and the processor architecture of the reference "roundhouse, Version=0.8.6.0, Culture=neutral, PublicKeyToken=91b86fd44f1f23bc, processorArchitecture=x86", "x86". This mismatch may cause runtime failures. Please consider changing the targeted processor architecture of your project through the Configuration Manager so as to align the processor architectures between your project and references, or take a dependency on references with a processor architecture that matches the targeted processor architecture of your project.

chucknorris / roundhouse

Improve MSBuild integration #201