Skip to content
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

Move platform dependent files to respective directories. #3168

Closed
wants to merge 14 commits into from

Conversation

deAtog
Copy link

@deAtog deAtog commented Jul 2, 2020

This pull request simplifies AnyCPU builds and permits side by side installation of the CefSharp
libraries by moving the CefSharp libraries to the platform dependent folders. With this change,
CefSharpTargetDir is no longer used to indicate where to put platform dependent files, and no
project changes are needed for AnyCPU builds.

Fixes: #3161

Summary:

  • This change moves the CefSharp platform dependent libraries to their respective folders.
  • XML transforms are used to add <dependentAssembly> sections to the app.config during build.

Changes:

  • Adds targets to NuGet packages that are executed during build.
  • Simplifies NuGet .targets files for all platforms.

How Has This Been Tested?

  • Tested updated NuGet packages using a local folder.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Updated documentation

Checklist:

  • Tested the code(if applicable)
  • Commented my code
  • Changed the documentation(if applicable)
  • New files have a license disclaimer
  • The formatting is consistent with the project (project supports .editorconfig)

@AppVeyorBot
Copy link

Build CefSharp 83.4.2-CI3576 completed (commit dc3e7a3b13 by @)

@AppVeyorBot
Copy link

Build CefSharp 83.4.2-CI3577 completed (commit 981a6849e1 by @)

@AppVeyorBot
Copy link

Build CefSharp 83.4.2-CI3583 completed (commit fb8667e165 by @)

@AppVeyorBot
Copy link

Build CefSharp 83.4.2-CI3584 completed (commit bbb211fe39 by @)

<Visible>false</Visible>
</None>
</ItemGroup>
<UsingTask TaskName="TransformXml" AssemblyFile="$(MSBuildExtensionsPath)\Microsoft\VisualStudio\v$(VisualStudioVersion)\Web\Microsoft.Web.Publishing.Tasks.dll" />
Copy link
Member

Choose a reason for hiding this comment

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

Would this run on VS2013/VS2015? The folder structure changed starting with VS2017, so I'd be surprised if this actually ran on older versions.

From what I can tell Microsoft.Web.Publishing.Tasks.dll is not guaranteed to be installed in Visual Studio, looks like it's only installed as part of the web development tools. I don't think we can reasonably ask people to install this just to use our nuget package. Specially anyone upgrading for whom the packages worked previously.

Reference https://stackoverflow.com/a/44855452/4583726

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't aware of this, I think we can resolve this by using a custom task library that utilizes Microsoft.Web.Xdt.

@amaitland
Copy link
Member

It's an interesting concept and I wish the PR had been submitted some four years ago. I'm hopeful that we can use some parts of it. There are just too many changes including a number of breaking ones. I do have a plan to release a new set of packages (#2795) that will hopefully use runtimes\native option (having a battle with the VC++ dependencies unfortunately). This should support your use case (#3161)

CefSharpTargetDir is no longer used to indicate where to put platform dependent files

This is a breaking change for anyone currently setting this property.

  • This change moves the CefSharp platform dependent libraries to their respective folders.

This is a breaking change and would cause problems for those using build scripts, clickonce, other installers, etc. I expect this would be a support nightmare.

  • Simplifies NuGet .targets files for all platforms.

They were deliberately verbose, don't remember the exact details, something about msbuild performing unnecessary re-compilations when using the condition syntax, the choose syntax was preferred. Have to go back in the history for the details.

  • Adds targets to NuGet packages that are executed during build.

Not clear how this would impact anyone using AnyCPU currently, possibly those using Prefer 32bit would have problems, anyone copying the files on first run from the x86/x64 directory into the root then removing the unused dependencies might also have problems. Lots of testing would be required.

I have some other concerns which I've commented inline.

I'm happy to include the transforms and allow the user to include a reference to a special .targets file, possibly even add a configuration option for AnyCPU, a one liner that enables using the transform. Moving the x86/x64 target files would be too problematic in my opinion. I'd rather spend time finishing #2795 than dealing with all the questions/support related to this.

Basically I'm not prepared to consider any breaking changes to the current set of packages.

@deAtog
Copy link
Author

deAtog commented Jul 10, 2020

It's an interesting concept and I wish the PR had been submitted some four years ago. I'm hopeful that we can use some parts of it. There are just too many changes including a number of breaking ones. I do have a plan to release a new set of packages (#2795) that will hopefully use runtimes\native option (having a battle with the VC++ dependencies unfortunately). This should support your use case (#3161)

I haven't looked too closely at these packages, but in my opinion this is a step backwards from a usability perspective. The current packages are far more developer friendly in my opinion. E.g. to use the new packages, my project would have to depend on both the x64 and x86 packages and conditionally include the PackageReference based on the Platform.

CefSharpTargetDir is no longer used to indicate where to put platform dependent files

This is a breaking change for anyone currently setting this property.

I think this could be resolved by using a custom msbuild task that uses Microsoft.Web.Xdt to do the transformations. The task could do variable substitution as is done for web.config files.

  • This change moves the CefSharp platform dependent libraries to their respective folders.

This is a breaking change and would cause problems for those using build scripts, clickonce, other installers, etc. I expect this would be a support nightmare.

I don't really see any way around this. I can see how this could be a breaking change from a deployment perspective, but I'm not sure I'd consider that a breaking change for the package as a whole. Any time a package is upgraded, users will likely have to adjust their deployment scripts as files may have been removed or new ones may have been added. These packages depend on cef.redist.x86 and cef.redist.x64 which I suspect have not remained consistent in this manner from version to version. Why would anyone expect different from these packages or any other packages for that manner?

  • Simplifies NuGet .targets files for all platforms.

They were deliberately verbose, don't remember the exact details, something about msbuild performing unnecessary re-compilations when using the condition syntax, the choose syntax was preferred. Have to go back in the history for the details.

I can revert to the choose syntax if need be, but the AnyCPU section would be overly redundant.

  • Adds targets to NuGet packages that are executed during build.

Not clear how this would impact anyone using AnyCPU currently, possibly those using Prefer 32bit would have problems, anyone copying the files on first run from the x86/x64 directory into the root then removing the unused dependencies might also have problems. Lots of testing would be required.

The Prefer 32bit flag tells Windows to run an AnyCPU application in 32bit mode instead of 64bit mode. The flag is mainly there to support ARM processors. In other words it allows one to generate a binary that will run on any processor, but the IL will be compiled to 32bit instead of 64bit if the processor is natively 64bit. To contrast this with the x86 platform, which generates IL that is only compatible with the Intel x86 processors. CEF appears to support ARM, but there is no cef.redist.arm package so this is a non-issue at this time. Regardless if the Prefer 32bit flag is set .Net should choose the correct assembly based on the processorArchitecture attribute.

I have some other concerns which I've commented inline.

I'm happy to include the transforms and allow the user to include a reference to a special .targets file, possibly even add a configuration option for AnyCPU, a one liner that enables using the transform. Moving the x86/x64 target files would be too problematic in my opinion. I'd rather spend time finishing #2795 than dealing with all the questions/support related to this.

Basically I'm not prepared to consider any breaking changes to the current set of packages.

@AppVeyorBot
Copy link

Build CefSharp 83.4.2-CI3591 completed (commit 62d4fc2625 by @)

@amaitland
Copy link
Member

I haven't looked too closely at these packages, but in my opinion this is a step backwards from a usability perspective. The current packages are far more developer friendly in my opinion. E.g. to use the new packages, my project would have to depend on both the x64 and x86 packages and conditionally include the PackageReference based on the Platform.

No, if I can get it working properly then the meta package will select the correct runtime there is a discussion at #2796 (comment) that outlines the approach.

This is the way forward in my opinion.

@amaitland
Copy link
Member

I don't really see any way around this. I can see how this could be a breaking change from a deployment perspective, but I'm not sure I'd consider that a breaking change for the package as a whole. Any time a package is upgraded, users will likely have to adjust their deployment scripts as files may have been removed or new ones may have been added. These packages depend on cef.redist.x86 and cef.redist.x64 which I suspect have not remained consistent in this manner from version to version. Why would anyone expect different from these packages or any other packages for that manner?

Personally I think your underestimating the disruption your proposed changes would cause. From memory I've only ever had a handful of requests for having side by side x86/x64 builds in the save directory over the last 6 years. Considering you can specify CefSharpTargetDir and choose where the files are copied, the amount of effort just isn't justifiable in my opinion. As it stands currently you should be able to achieve #3161 without that much effort. In theory it shouldn't be much more than setting CefSharpTargetDir, copy the AppDomain.CurrentDomain.AssemblyResolve code from #1714.

I am happy to add some new .targets file you can optionally include support the app.config transform. This should give you exactly what you have now with only 2-3 lines added to your project file.

The Prefer 32bit flag tells Windows to run an AnyCPU application in 32bit mode instead of 64bit mode. The flag is mainly there to support ARM processors. In other words it allows one to generate a binary that will run on any processor, but the IL will be compiled to 32bit instead of 64bit if the processor is natively 64bit. To contrast this with the x86 platform, which generates IL that is only compatible with the Intel x86 processors. CEF appears to support ARM, but there is no cef.redist.arm package so this is a non-issue at this time. Regardless if the Prefer 32bit flag is set .Net should choose the correct assembly based on the processorArchitecture attribute

There are more than a few people targeting AnyCPU and have Prefer 32bit set that are using CefSharp on x86/amd64 architectures. From memory this was the default for VS2013 ( Ref 1 Ref 2 Ref 3), pretty sure it's still the default for at least a few project types.

My actual point is there are quite a number of different use cases that would need to be tested in detail.

If I cannot get runtime.json approach to work for #2795 then we can look at integrating some of this functionality into the new set of packages.

@AppVeyorBot
Copy link

Build CefSharp 83.4.2-CI3601 completed (commit 24260255b3 by @)

@AppVeyorBot
Copy link

Build CefSharp 83.4.2-CI3602 completed (commit 3ff77a0950 by @)

@deAtog
Copy link
Author

deAtog commented Jul 13, 2020

Considering you can specify CefSharpTargetDir and choose where the files are copied, the amount of effort just isn't justifiable in my opinion. As it stands currently you should be able to achieve #3161 without that much effort. In theory it shouldn't be much more than setting CefSharpTargetDir, copy the AppDomain.CurrentDomain.AssemblyResolve code from #1714.

I am happy to add some new .targets file you can optionally include support the app.config transform. This should give you exactly what you have now with only 2-3 lines added to your project file.

I know it's possible to do this with the packages in their current form, but nothing about the above is developer friendly. With the changes I've proposed one can easily switch between the x86, x64, and AnyCPU targets without making any application or project changes. I think support for CefSharpTargetDir can be added to the transforms. Regardless of what we do for the platform specific cases, I think the transforms should always be done for the AnyCPU target as it eliminates any project or code changes. If you have any ideas about how to deprecate the current functionality I'm open for suggestions.

I don't have a whole lot of experience with creating platform dependent NuGet packages, so most of this is new to me. From my limited experience I've seen a couple different ways of dealing with this issue. Most prefer to just copy all package files to the build output folder regardless of target platform. They either then use NuGet's built-in xml transforms or provide an AnyCPU interop library that forwards interfaces to the platform specific library. From my understanding, these transforms only get applied at package install and are removed during uninstall, which prevents dynamically changing the build folder structure. The transforms I've provided could be applied in this manner instead, but again users would not be able to use CefSharpTargetDir.

There are more than a few people targeting AnyCPU and have Prefer 32bit set that are using CefSharp on x86/amd64 architectures. From memory this was the default for VS2013 ( Ref 1 Ref 2 Ref 3), pretty sure it's still the default for at least a few project types.

My actual point is there are quite a number of different use cases that would need to be tested in detail.

My point here was that regardless of how Prefer 32bit is set, the correct assembly will be loaded based on the execution context of the application. If it is executing in 32 bit mode then it will load the assembly from the x86 folder. If it is executing in 64 bit mode then it will load the assembly from the x64 folder. The Prefer 32bit option forces an AnyCPU application to run in a 32 bit context instead of the default 64 bit context on 64 bit processors. An AnyCPU application with the Prefer 32bit option set will always load the library from the x86 folder.

@deAtog
Copy link
Author

deAtog commented Jul 15, 2020

@amaitland I've been thinking about this, and I think I have a partial solution:

  1. Once support for CefSharpTargetDir is added we apply the transforms whenever the it is not equal to '.' or if the platform is AnyCPU. This ensures that no application modifications are needed.
  2. Deprecate CefSharpAnyCPUSupport as it is no longer needed to support AnyCPU projects.
  3. Add CefSharpPlatformDirs boolean with a default of false to force the addition of platform specific directories on x86/x64 platforms. This option will have no effect for AnyCPU projects as platform directories are required. An informational message can be added to inform users of this option during build when the target platform is not AnyCPU and it has not been explicitly set.

With the above, side by side builds would be supported by defining CefSharpPlatformDirs as true.

@amaitland
Copy link
Member

With the changes I've proposed one can easily switch between the x86, x64, and AnyCPU targets without making any application or project changes.

That is also the hope for the newer set of packages.

I don't have a whole lot of experience with creating platform dependent NuGet packages, so most of this is new to me.

It's complicated, specially 4 years ago the options were very limited. You'd be amazed at how often people attempt things you just never even though of. What we have currently is far from perfect, it is well tested though.

From my limited experience I've seen a couple different ways of dealing with this issue. Most prefer to just copy all package files to the build output folder regardless of target platform.

Which packages are you referring to specifically? Are they using VC++? Using P/Invoke is far far simpler.

My point here was that regardless of how Prefer 32bit is set, the correct assembly will be loaded based on the execution context of the application. If it is executing in 32 bit mode then it will load the assembly from the x86 folder. If it is executing in 64 bit mode then it will load the assembly from the x64 folder.

The different use cases would still need to be tested in detail.

The Prefer 32bit option forces an AnyCPU application to run in a 32 bit context instead of the default 64 bit context on 64 bit processors. An AnyCPU application with the Prefer 32bit option set will always load the library from the x86 folder.

I'm aware of how AnyCPU works.

  1. Once support for CefSharpTargetDir is added we apply the transforms whenever the it is not equal to '.' or if the platform is AnyCPU. This ensures that no application modifications are needed.

It's an interesting idea

3. Add CefSharpPlatformDirs boolean with a default of false to force the addition of platform specific directories on x86/x64 platforms. This option will have no effect for AnyCPU projects as platform directories are required. An informational message can be added to inform users of this option during build when the target platform is not AnyCPU and it has not been explicitly set.

Seems like it would be simpler

  • Have the user set CefSharpTargetDir to PlatformTarget and only apply the transform if CefSharpTargetDir is not equal to .
  • If CefSharpAnyCPUSupport is set then do nothing.

If you wish to peruse this path then I'll have a look when I have time. I have quite the backlog so it would be at least a week (maybe more) before I have time to look at this again.

@amaitland
Copy link
Member

Moving the CefSharp/CEF dependencies into x86/x64 subfolders breaks .Net Core builds using the current packages. The assemblyIdentity in app.config does not appear to be supported (not surprised as it doesn't support probing privatePath to my knowledge).

The current implementation relying on Microsoft.Web.Publishing.Tasks.dll is also problematic (my current machine for example doesn't have it installed). I did find https://www.nuget.org/packages/MSBuild.Microsoft.VisualStudio.Web.targets/ which appears to be an unofficial package that could be an option for those without having the VS features installed. Yes I am aware creating a custom task is an option.

I have some massive restructure planned (#3311) where the managed libraries will be converted from x86/x64 to AnyCPU and have a single Mixed Mode CLI/C++ dll. At this point in time I've included the two required transform files for the x86/x64 options.

I do appreciate the contribution 👍 If it were 5+ years ago I probably would have gone done this path. Closing this now.

@amaitland amaitland closed this Dec 11, 2020
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