Skip to content

Add Xamarin and WinStore/Phone support with PCL Profile 259 (.NET 4.5) #73

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

Closed
wants to merge 11 commits into from
Closed

Add Xamarin and WinStore/Phone support with PCL Profile 259 (.NET 4.5) #73

wants to merge 11 commits into from

Conversation

angularsen
Copy link

Adds .NET 4.5 portable profile 259.
This profile is widely used and well supported by Xamarin for iOS and Android, as well as all the Windows targets for PC and mobile.

Target: portable-net45+netcore45+wpa81+wp8+MonoAndroid1+MonoTouch1

The biggest challenge was fixing all the reflection code to support the new reflection API around TypeInfo. This API is not supported in the current portable profile 136 for .NET 4.0, since it is only available in .NET 4.5 and later.

Portable nuget now has the following targets: .NET 3.5, .NET 4.0 (pcl) and .NET 4.5 (pcl)

Some cleanup:

  • Rename PORTABLE compile condition to PORTABLE40, and add PORTABLE45
  • Rename portable projects to Common.Logging.Portable.2010-netxx for consistency
  • Change output paths to /build/portable/
  • Use separate intermediate output paths for portable projects to avoid "already in use" build errors in Visual Studio

I particularly want review on the following changes:

.NET 4.5 portable profile 259.
Compatible with Xamarin for iOS and Android.

PCL profile 259 description:
portable-net45+netcore45+wpa81+wp8+MonoAndroid1+MonoTouch1
To avoid "already in use" errors when building from Visual Studio
since they have the same name of the assembly file.

Msbuild does not seem to have this problem.
Mostly related to reflection. Solved using compile conditions on
PORTABLE45.

WinStore introduced a new reflection API with TypeInfo and newer PCL
profiles use this instead, but the existing Portable project is the
older profile 136 that does not support this, so we need to
distinguish between PORTABLE (profile 136) and PORTABLE45 (profile 259).

The new reflection API is only available on .NET 4.5 and later, so as
long as .NET 4.0 support is desired we will need to distinguish these
two. An improvement would be to rename the PORTABLE condition to
PORTABLE40.

Breaking change in ExceptionFormatter.cs:
Thread information is not available in profile 259 as
far as I can see. I've set the string to "N/A" on the thread ID for
now. Either we must inject a function to retrieve the thread ID using
a platform specific API or find a way to access it from PCL profile 259.
It currently represents PCL profile 136 for .NET 4.0.
Helps disinguish it from PORTABLE45 (profile 259).
For consistency among -net20, -net40 and -net45 variants.
Consistent paths for .net 2.0, 4.0 and 4.5.
Fix paths nuspec file.
This is already covered by the portable target for .NET 4.0.
Also update checked in file to match contents after running a build.

Note: I'm not entirely sure about the impact of these changes,
specifically changing "portable" string to "portable40" and
"portable45". I'd like some review feedback on it.
Generated by running build-package.bat
This was referenced Nov 30, 2014
@angularsen
Copy link
Author

One issue I've found so far is that DebugOutLogger does not output anything to my Output window in Visual Studio when running the application in Debug configuration.

During development of this pull request I built my own portable assembly in the app to test things out, and that worked. I built the assembly in Debug mode, and looking through the implementation it makes sense since DebugOutLogger simply calls Debug.WriteLine(), but that method call is removed by the compiler when compiling to Release, isn't it?

If so, aren't the nugets always compiled in Release effectively rendering DebugOutLogger a NoOpLogger? A similar concern is discussed here: http://stackoverflow.com/a/9263339/134761

Hope you can provide some insight, maybe I've misunderstood something. Thanks!

@angularsen
Copy link
Author

Sorry for the rant, but I've spent way too much time on this already and getting a little bit frustrated.

So after I continued to hook this up to NLog in my WPF app, I installed the Common.Logging.NLog31 nuget, which brought in the non-portable Common.Logging library as a dependency. That gave me ConsoleOutLogger in the WPF app, nice, but I can't have both the portable and the non-portable logging nuget in my WPF app, and if I remove the portable one I get compile errors since one of my PCL libraries takes the ILog interface as a parameter, and it only knows about the portable one. I might be able to solve this with some binding redirects, but this is starting to smell bad.

Let me know if I have misunderstood something, but here is my take on how it should have worked:

  • I have a WPF app and a PCL library I want to reuse for other platforms like Xamarin for Android and iOS.
  • I want to provide a logger for my PCL library, and basically all it needs is the ILog interface. That's it.
  • The WPF app, on the other hand, passes in the logger implementation, such as ConsoleOutLogger, or something tied up to NLog/Log4Net.

My proposed structure:

  • Common.Logging.Interface: Portable + .net 3.5 targets. ILog and any other interfaces needed, meant for log producers.
  • Common.Logging.Core: Requires the full .NET platform. Has all the config reader, formatting, reflection and simple logger stuff. Meant for apps and setup/initializers. Depends on the Interface nuget.
  • Common.Logging.NLog/Log4Net++. Depends on the Interface nuget, possibly the core nuget as well, I don't know that part very well.

At least to me, and from a portable app standpoint, this makes a whole lot more sense. Adding support for other portable profiles in the future would also be a lot less pain if they didn't have to implement so much code, if any at all. Having just recently implemented this pull request, I know what part very well.

@angularsen
Copy link
Author

My current workaround is creating my own PortableNLogLogger and PortableNLogLoggerAdapter, copied and modified from the Common.Logging.NLog nuget. This works with the portable common.logging nuget.

@sbohlen
Copy link
Member

sbohlen commented Jan 10, 2015

Thanks for the comments/PR. From my read of this, you have come up against the blocker outlined here #71. Your proposed solution in re: "Common.Logging.Interfaces" sounds (nearly) identical to the outlined changes in Common.Logging.Core in that issue/thread, so I'm presuming that this set of changes in the PCL-EXPERIMENTATION branch (https://github.com/net-commons/common-logging/tree/pcl-experimentation) is likely to address your blockers as well (?)

Can you comment on this --?

@angularsen
Copy link
Author

Sorry, I've written my own log interface and adapter since then. It was just easier in the end. PortableLog. I have shamelessly copied quite a bit of the common-logging code, but added my own flavor to it. I hope you don't mind. You can close this pull request if you want.

@sbohlen
Copy link
Member

sbohlen commented Jan 10, 2015

I don't mind at all; sorry you weren't able to wait for us to get the PCL story properly sorted on our end.

Cheers,

-Steve B.

@sbohlen sbohlen closed this Jan 10, 2015
@angularsen
Copy link
Author

Thanks, cheers!

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.

2 participants