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

0.9.0 Database classes missing #306

Closed
drjaydenm opened this Issue Dec 13, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@drjaydenm

drjaydenm commented Dec 13, 2017

Hi there, after upgrading to 0.9.0, it seems as if the roundhouse.dll has dropped the database specific classes e.g. the roundhouse.databases.sqlserver.SqlServerDatabase class in particular.

I tried looking for other roundhouse nuget packages in-case it had been split out, however couldn't see any. Where can I find the SqlServerDatabase class in this release?

At the moment, I will have to downgrade back to 0.8.8 to fix this issue

@BiggerNoise

This comment has been minimized.

Show comment
Hide comment
@BiggerNoise

BiggerNoise Dec 13, 2017

Member

@drjaydenm Thank you for the report. You are correct that the classes are missing. We'll figure out the issue and get this fixed as soon as we can.

@ferventcoder & @erikbra:

We have an ILMerge step for the console, but not one for the DLL. I can take a shot at adding one, it's complicated a bit by the fact that the database libs are not dependencies of the rh lib, so they are not in the build output directory.

I'm open to another suggestion, but I would expect that we want to do something in that vein.

For what it's worth, here's the first few lines of the ILMerge output from the last UppercuT driven build:

ILMerge version 2.14.1208.65535
Copyright (C) Microsoft Corporation 2004-2006. All rights reserved.
ILMerge /internalize:C:\development\Chuck\roundhouse\build.custom\..\build.custom\ilmerge.internalize.ignore.txt /target:dll /out:C:\development\Chuck\roundhouse\code_drop\merge\roundhouse.dll /log:C:\development\Chuck\roundhouse\code_drop\build_artifacts\ilmerge\ilmergeDLL.log /ndebug /zeroPeKind /allowDup roundhouse.dll /keyfile:C:\development\Chuck\roundhouse\build.custom\..\RoundhousE.snk Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling.Data.dll Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling.dll MySql.Data.dll Npgsql.dll roundhouse.databases.access.dll roundhouse.databases.mysql.dll roundhouse.databases.oracle.dll roundhouse.databases.postgresql.dll roundhouse.databases.sqlite.dll roundhouse.databases.sqlserver.dll roundhouse.databases.sqlserver2000.dll roundhouse.databases.sqlserverce.dll StructureMap.dll 
Set platform to 'v4', using directory 'C:\Windows\Microsoft.NET\Framework64\v4.0.30319\..\v4.0.30319' for mscorlib.dll
Running on Microsoft (R) .NET Framework v4.0.30319
mscorlib.dll version = 4.0.0.0
Read 4 lines from the exclusion file 'C:\development\Chuck\roundhouse\build.custom\..\build.custom\ilmerge.internalize.ignore.txt'.
The list of input assemblies is:
	roundhouse.dll
	Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling.Data.dll
	Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling.dll
	MySql.Data.dll
	Npgsql.dll
	roundhouse.databases.access.dll
	roundhouse.databases.mysql.dll
	roundhouse.databases.oracle.dll
	roundhouse.databases.postgresql.dll
	roundhouse.databases.sqlite.dll
	roundhouse.databases.sqlserver.dll
	roundhouse.databases.sqlserver2000.dll
	roundhouse.databases.sqlserverce.dll
	StructureMap.dlll
Member

BiggerNoise commented Dec 13, 2017

@drjaydenm Thank you for the report. You are correct that the classes are missing. We'll figure out the issue and get this fixed as soon as we can.

@ferventcoder & @erikbra:

We have an ILMerge step for the console, but not one for the DLL. I can take a shot at adding one, it's complicated a bit by the fact that the database libs are not dependencies of the rh lib, so they are not in the build output directory.

I'm open to another suggestion, but I would expect that we want to do something in that vein.

For what it's worth, here's the first few lines of the ILMerge output from the last UppercuT driven build:

ILMerge version 2.14.1208.65535
Copyright (C) Microsoft Corporation 2004-2006. All rights reserved.
ILMerge /internalize:C:\development\Chuck\roundhouse\build.custom\..\build.custom\ilmerge.internalize.ignore.txt /target:dll /out:C:\development\Chuck\roundhouse\code_drop\merge\roundhouse.dll /log:C:\development\Chuck\roundhouse\code_drop\build_artifacts\ilmerge\ilmergeDLL.log /ndebug /zeroPeKind /allowDup roundhouse.dll /keyfile:C:\development\Chuck\roundhouse\build.custom\..\RoundhousE.snk Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling.Data.dll Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling.dll MySql.Data.dll Npgsql.dll roundhouse.databases.access.dll roundhouse.databases.mysql.dll roundhouse.databases.oracle.dll roundhouse.databases.postgresql.dll roundhouse.databases.sqlite.dll roundhouse.databases.sqlserver.dll roundhouse.databases.sqlserver2000.dll roundhouse.databases.sqlserverce.dll StructureMap.dll 
Set platform to 'v4', using directory 'C:\Windows\Microsoft.NET\Framework64\v4.0.30319\..\v4.0.30319' for mscorlib.dll
Running on Microsoft (R) .NET Framework v4.0.30319
mscorlib.dll version = 4.0.0.0
Read 4 lines from the exclusion file 'C:\development\Chuck\roundhouse\build.custom\..\build.custom\ilmerge.internalize.ignore.txt'.
The list of input assemblies is:
	roundhouse.dll
	Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling.Data.dll
	Microsoft.Practices.EnterpriseLibrary.TransientFaultHandling.dll
	MySql.Data.dll
	Npgsql.dll
	roundhouse.databases.access.dll
	roundhouse.databases.mysql.dll
	roundhouse.databases.oracle.dll
	roundhouse.databases.postgresql.dll
	roundhouse.databases.sqlite.dll
	roundhouse.databases.sqlserver.dll
	roundhouse.databases.sqlserver2000.dll
	roundhouse.databases.sqlserverce.dll
	StructureMap.dlll
@erikbra

This comment has been minimized.

Show comment
Hide comment
@erikbra

erikbra Dec 13, 2017

Member

This has totally passed me, I didn't know we did this is all, sorry. Is there any reason we should have all this in one DLL, and not separate nuget packages for each native provider? I think this might help going forward, as we hope to migrate towards .NET Standard, and not every database provider might be migrateable. I have limited knowledge of using roundhouse as a DLL, not .exe, so I don't know how mostpeople use it.

Member

erikbra commented Dec 13, 2017

This has totally passed me, I didn't know we did this is all, sorry. Is there any reason we should have all this in one DLL, and not separate nuget packages for each native provider? I think this might help going forward, as we hope to migrate towards .NET Standard, and not every database provider might be migrateable. I have limited knowledge of using roundhouse as a DLL, not .exe, so I don't know how mostpeople use it.

@BiggerNoise

This comment has been minimized.

Show comment
Hide comment
@BiggerNoise

BiggerNoise Dec 13, 2017

Member

I have not used RH in a DLL either, so this one went right by me as well.

I suspect that the single DLL is more for our convenience than anything else. However, I think that if we can get the NuGet and the AppVeyor accounts linked, then publishing all of the packages for a particular release is going to be the same effort.

In light of this, and the concern about .NET standard, I agree that we should move to publishing separate packages for each DB library.

Member

BiggerNoise commented Dec 13, 2017

I have not used RH in a DLL either, so this one went right by me as well.

I suspect that the single DLL is more for our convenience than anything else. However, I think that if we can get the NuGet and the AppVeyor accounts linked, then publishing all of the packages for a particular release is going to be the same effort.

In light of this, and the concern about .NET standard, I agree that we should move to publishing separate packages for each DB library.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Dec 13, 2017

Member

If I may, avoid separate packages for each. The usefulness and convenience of the exe is not in having to install multiple packages. It is the same in the DLL.

Rh.dll alone is simply an engine that does almost nothing without having the database dlls.

Plus the plugin architecture needs the dlls next to the main dll

Member

ferventcoder commented Dec 13, 2017

If I may, avoid separate packages for each. The usefulness and convenience of the exe is not in having to install multiple packages. It is the same in the DLL.

Rh.dll alone is simply an engine that does almost nothing without having the database dlls.

Plus the plugin architecture needs the dlls next to the main dll

@BiggerNoise

This comment has been minimized.

Show comment
Hide comment
@BiggerNoise

BiggerNoise Dec 13, 2017

Member

Leaving aside possible issues with .NET Standard, what if the database DLLs included the rh.dll? It could be setup as a dependent package.

Does anyone use the RH library with multiple database engines from the same project? (Serious question, not rhetorical or snarky). If not, they're still installing the same number of libraries.

Member

BiggerNoise commented Dec 13, 2017

Leaving aside possible issues with .NET Standard, what if the database DLLs included the rh.dll? It could be setup as a dependent package.

Does anyone use the RH library with multiple database engines from the same project? (Serious question, not rhetorical or snarky). If not, they're still installing the same number of libraries.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Dec 13, 2017

Member

Yes, frameworks like DropkicK do.

Member

ferventcoder commented Dec 13, 2017

Yes, frameworks like DropkicK do.

@ferventcoder

This comment has been minimized.

Show comment
Hide comment
@ferventcoder

ferventcoder Dec 13, 2017

Member

Cake build being another.

Member

ferventcoder commented Dec 13, 2017

Cake build being another.

@BiggerNoise

This comment has been minimized.

Show comment
Hide comment
@BiggerNoise

BiggerNoise Dec 13, 2017

Member

OK. I think I'll have time tomorrow night to look into doing the merge operation.

If either of you want to create a P/R for a merge request, then I can merge it and cut a new release.

Member

BiggerNoise commented Dec 13, 2017

OK. I think I'll have time tomorrow night to look into doing the merge operation.

If either of you want to create a P/R for a merge request, then I can merge it and cut a new release.

@erikbra

This comment has been minimized.

Show comment
Hide comment
@erikbra

erikbra Dec 15, 2017

Member

The middle way would maybe be to create one "big" DLL Nuget, with ILMerges, and, maybe later, add slimmer ones, if we get some technology problems with migrating all to core, e.g.

To solve this particular bug, I would say maybe making a separate .csproj for the "merged" version of Roundhouse.dll would be the easiest way right now, as we don't have much "build tools" support (only Powershell.

I'm thinking, creating a new project, Roundhouse.Merged, Roundhouse.Dll.Merge, or something, and referencing all the projects we want to merge into the Roundhouse.dll there, and do the ILMerge and Nuget pack on that project. If anyone else don't have a better idea? We could of course go and collect all the files, but I think the way of using a single output folder for each project was a bit convoluted (at least to me), but I might stand corrrected.

Member

erikbra commented Dec 15, 2017

The middle way would maybe be to create one "big" DLL Nuget, with ILMerges, and, maybe later, add slimmer ones, if we get some technology problems with migrating all to core, e.g.

To solve this particular bug, I would say maybe making a separate .csproj for the "merged" version of Roundhouse.dll would be the easiest way right now, as we don't have much "build tools" support (only Powershell.

I'm thinking, creating a new project, Roundhouse.Merged, Roundhouse.Dll.Merge, or something, and referencing all the projects we want to merge into the Roundhouse.dll there, and do the ILMerge and Nuget pack on that project. If anyone else don't have a better idea? We could of course go and collect all the files, but I think the way of using a single output folder for each project was a bit convoluted (at least to me), but I might stand corrrected.

@BiggerNoise

This comment has been minimized.

Show comment
Hide comment
@BiggerNoise

BiggerNoise Dec 15, 2017

Member

Yeah. I took a stab at copying the logic for the console app and adapting it to the lib situation. It's almost working, but maintenance is going to be a pain.

I think that your suggestion to create a merge project makes a lot of sense. It will be much easier to to follow and adjust when we have to exclude libraries.

I'd use the name Roundhouse.Lib.Merged. That will produce the roundhouse.lib NuGet package.

BTW - I took a stab at this because I felt that all of the build related stuff was ending up in your lap. If you want to take a crack at the approach you describe, please go ahead.

Member

BiggerNoise commented Dec 15, 2017

Yeah. I took a stab at copying the logic for the console app and adapting it to the lib situation. It's almost working, but maintenance is going to be a pain.

I think that your suggestion to create a merge project makes a lot of sense. It will be much easier to to follow and adjust when we have to exclude libraries.

I'd use the name Roundhouse.Lib.Merged. That will produce the roundhouse.lib NuGet package.

BTW - I took a stab at this because I felt that all of the build related stuff was ending up in your lap. If you want to take a crack at the approach you describe, please go ahead.

@erikbra

This comment has been minimized.

Show comment
Hide comment
@erikbra

erikbra Dec 15, 2017

Member

Very happy that you grab on to build stuff as well. Being very dependent on single persons is a pain. I'll try to make a stab at my suggestion, I think it'll keep things simple, at least. Maybe I have some time this weekend, I'll try to manage.

Member

erikbra commented Dec 15, 2017

Very happy that you grab on to build stuff as well. Being very dependent on single persons is a pain. I'll try to make a stab at my suggestion, I think it'll keep things simple, at least. Maybe I have some time this weekend, I'll try to manage.

erikbra added a commit to erikbra/roundhouse that referenced this issue Dec 15, 2017

Fixed Issue chucknorris#306 - added roundhouse.lib.merged
Added another project, just for the merge of all the DLLs into one for
Nuget packaging. Made more of the original Nuget dependencies proper
dependencies of roundhouse.lib instead of merging some of them. I think
merging them might lead to problems if someone uses both roundhouse.lib
and one of the original dependency nuget packages.
@erikbra

This comment has been minimized.

Show comment
Hide comment
@erikbra

erikbra Dec 16, 2017

Member

@BiggerNoise, please see PR #308. @drjaydenm, could you please do me a favour, and test if this prerelease version of the NuGet package contains all classes you expect, and isn't broken in any other way? https://ci.appveyor.com/api/buildjobs/l9fpiqbmqdi7wi7s/artifacts/code_drop%2Fpackages%2Froundhouse.lib.0.9.1-PullRequest.308.nupkg

Member

erikbra commented Dec 16, 2017

@BiggerNoise, please see PR #308. @drjaydenm, could you please do me a favour, and test if this prerelease version of the NuGet package contains all classes you expect, and isn't broken in any other way? https://ci.appveyor.com/api/buildjobs/l9fpiqbmqdi7wi7s/artifacts/code_drop%2Fpackages%2Froundhouse.lib.0.9.1-PullRequest.308.nupkg

BiggerNoise added a commit that referenced this issue Dec 18, 2017

Merge pull request #308 from erikbra/issue306
Fixed Issue #306 - added roundhouse.lib.merged
@BiggerNoise

This comment has been minimized.

Show comment
Hide comment
@BiggerNoise

BiggerNoise Dec 19, 2017

Member

OK, this is fixed in 0.9.1 and available on Nuget.org. @drjaydenm - Thank you again for bringing this to our attention and helping us verify the fix.

Member

BiggerNoise commented Dec 19, 2017

OK, this is fixed in 0.9.1 and available on Nuget.org. @drjaydenm - Thank you again for bringing this to our attention and helping us verify the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment