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

.NET: Add support for .NET Core #2269

Closed
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
@qmfrederik

qmfrederik commented Jun 12, 2016

This PR makes the Selenium WebDriver project compile on .NET Core. The changes basically boil down to fixes for:

  • The new reflection API
  • The absence of Serialization
  • Standardization on .Dispose instead of .Close
  • Changes in how OS-related information is accessed
  • Changes in the HttpWebRequest and HttpWebResponse classes

Happy to work with you on any feedback you may provide,

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Jun 12, 2016

Member

Can you please squash these commits?

Member

jimevans commented Jun 12, 2016

Can you please squash these commits?

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Jun 12, 2016

@jimevans I've squashed all the "straightforward" commits into a single one (so we're down to 4), if you want me to squash them further just let me know.

PS: I see the Travis build is failing but the failure doesn't seem to be related to these changes

qmfrederik commented Jun 12, 2016

@jimevans I've squashed all the "straightforward" commits into a single one (so we're down to 4), if you want me to squash them further just let me know.

PS: I see the Travis build is failing but the failure doesn't seem to be related to these changes

@jleyba jleyba added the C-dotnet label Jul 2, 2016

@flightlevel

This comment has been minimized.

Show comment
Hide comment
@flightlevel

flightlevel Aug 27, 2016

Any chance this could be merged in, would be great to be able to use on .NET Core

flightlevel commented Aug 27, 2016

Any chance this could be merged in, would be great to be able to use on .NET Core

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Nov 4, 2016

@jimevans @jleyba I've rebased this PR against master since it became outdated; could you take a look at this & let me know whether we could get this merged?

Happy to work with you on any feedback you may provide, and looking forward to use the C# WebDriver API on Linux & macOS :)

qmfrederik commented Nov 4, 2016

@jimevans @jleyba I've rebased this PR against master since it became outdated; could you take a look at this & let me know whether we could get this merged?

Happy to work with you on any feedback you may provide, and looking forward to use the C# WebDriver API on Linux & macOS :)

@rschiefer

This comment has been minimized.

Show comment
Hide comment
@rschiefer

rschiefer Nov 18, 2016

I'm also very interested in seeing .NET Core support in Selenium. @jimevans or @jleyba can you comment on when this PR might be reviewed?

rschiefer commented Nov 18, 2016

I'm also very interested in seeing .NET Core support in Selenium. @jimevans or @jleyba can you comment on when this PR might be reviewed?

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Nov 19, 2016

Member

I have reviewed the PR as best I can, but I'm unwilling to significantly push to merge it until the development tooling catches up with developing for .NET Core. Unfortunately, that likely means waiting for Visual Studio "15." It will happen, but I will have to ask for everyone's patience for it to happen. Thanks for understanding.

Member

jimevans commented Nov 19, 2016

I have reviewed the PR as best I can, but I'm unwilling to significantly push to merge it until the development tooling catches up with developing for .NET Core. Unfortunately, that likely means waiting for Visual Studio "15." It will happen, but I will have to ask for everyone's patience for it to happen. Thanks for understanding.

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Nov 19, 2016

To add support for .NET Core, there's two major work streams:

  • Make the codebase compatible with the .NET Core API surface (in this case, I've opted for .NET Standard 1.5)
  • Update the build scripts to work on .NET Core

.NET Standard 1.5 was released in early June '16. This API surface has been stable for almost 6 months by now. The updates in the tooling (VS2017 and later) will not impact the API surface.

Most of this PR (except for the project.json file) is about the API surface. @jimevans : Since the API surface is stable, do you think we can already move forward with this one? I'd be happy to remove any tooling-specific changes from this PR so it's purely focused on the API surface.

A release candidate of Visual Studio 2017, which will have the full .NET Core tooling, has been released, and the project.json to MSBuild migration has materialized in Visual Studio 2017 RC.
I can also offer to update the build files to match what's available in Visual Studio 2017, if that helps.

In meanwhile, those who are looking for a .NET core-compatible version (that would include @Jarga, @MartinJohns, @flightlevel, @rschiefer) and take a look at the CoreCompat.Selenium.WebDriver NuGet package. I intend to remove that NuGet package as soon as .NET Core support is available in the official Selenium NuGet packages, but it should work as a stop-gap solution for now.

qmfrederik commented Nov 19, 2016

To add support for .NET Core, there's two major work streams:

  • Make the codebase compatible with the .NET Core API surface (in this case, I've opted for .NET Standard 1.5)
  • Update the build scripts to work on .NET Core

.NET Standard 1.5 was released in early June '16. This API surface has been stable for almost 6 months by now. The updates in the tooling (VS2017 and later) will not impact the API surface.

Most of this PR (except for the project.json file) is about the API surface. @jimevans : Since the API surface is stable, do you think we can already move forward with this one? I'd be happy to remove any tooling-specific changes from this PR so it's purely focused on the API surface.

A release candidate of Visual Studio 2017, which will have the full .NET Core tooling, has been released, and the project.json to MSBuild migration has materialized in Visual Studio 2017 RC.
I can also offer to update the build files to match what's available in Visual Studio 2017, if that helps.

In meanwhile, those who are looking for a .NET core-compatible version (that would include @Jarga, @MartinJohns, @flightlevel, @rschiefer) and take a look at the CoreCompat.Selenium.WebDriver NuGet package. I intend to remove that NuGet package as soon as .NET Core support is available in the official Selenium NuGet packages, but it should work as a stop-gap solution for now.

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Nov 20, 2016

Member

Please don't think I'm trying to minimize the importance of this PR. I fully recognize how important it is. Yes, I understand that the API surface has been stable. I also understand that it's frustrating that it seems like this PR hasn't been given the attention you believe it deserves.

I've just spent the day trying to get home from overseas. The PR will happen, but again, I'm going to ask for a little more patience, please.

Member

jimevans commented Nov 20, 2016

Please don't think I'm trying to minimize the importance of this PR. I fully recognize how important it is. Yes, I understand that the API surface has been stable. I also understand that it's frustrating that it seems like this PR hasn't been given the attention you believe it deserves.

I've just spent the day trying to get home from overseas. The PR will happen, but again, I'm going to ask for a little more patience, please.

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Nov 20, 2016

@jimevans I understand, no worries. Anything I can do to help - please let me know. Happy to work with you on this.

qmfrederik commented Nov 20, 2016

@jimevans I understand, no worries. Anything I can do to help - please let me know. Happy to work with you on this.

@Brar Brar referenced this pull request Dec 3, 2016

Closed

Core compat #3200

@Elufimov

This comment has been minimized.

Show comment
Hide comment
@Elufimov

Elufimov Dec 19, 2016

Any news about this?

Elufimov commented Dec 19, 2016

Any news about this?

@Jetski5822

This comment has been minimized.

Show comment
Hide comment
@Jetski5822

Jetski5822 Dec 21, 2016

Hey Chaps, whats the good word?

Jetski5822 commented Dec 21, 2016

Hey Chaps, whats the good word?

@Elufimov

This comment has been minimized.

Show comment
Hide comment
@Elufimov

Elufimov Jan 27, 2017

Can anyone from dotnet team clarify of when we can expect .netstandard support?

Elufimov commented Jan 27, 2017

Can anyone from dotnet team clarify of when we can expect .netstandard support?

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Feb 6, 2017

@jimevans So, we've got a new release candidate of Visual Studio and I don't expect the tooling to change significantly before VS2017 is released.

I can allocate some time to work on this PR, but it would be great to do so when there's also some bandwidth on your side.

Let me know when that's the case and we'll move this forward.

Thx!

qmfrederik commented Feb 6, 2017

@jimevans So, we've got a new release candidate of Visual Studio and I don't expect the tooling to change significantly before VS2017 is released.

I can allocate some time to work on this PR, but it would be great to do so when there's also some bandwidth on your side.

Let me know when that's the case and we'll move this forward.

Thx!

@MatthewLymer

This comment has been minimized.

Show comment
Hide comment
@MatthewLymer

MatthewLymer Feb 27, 2017

Contributor

hello @qmfrederik, I was playing around with your nuget binaries on Amazon EC2, it works well but I have an intermittent issue when initializing PhantomJSDriver.

"at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)",
"at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)",
"at WebDriver.Internal.WebRequestExtensions.GetResponse(WebRequest request)",
"at OpenQA.Selenium.DriverService.get_IsInitialized()"

It throws an "AggregateException", and within that aggregate exception one of them are "WebException", looking at the implementation for "IsInitialized", it doesn't take into consideration that it's using async (as it's just trying to catch "WebException").

I can try and create a pull request if I can figure out how the build the thing.

Contributor

MatthewLymer commented Feb 27, 2017

hello @qmfrederik, I was playing around with your nuget binaries on Amazon EC2, it works well but I have an intermittent issue when initializing PhantomJSDriver.

"at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)",
"at System.Threading.Tasks.Task`1.GetResultCore(Boolean waitCompletionNotification)",
"at WebDriver.Internal.WebRequestExtensions.GetResponse(WebRequest request)",
"at OpenQA.Selenium.DriverService.get_IsInitialized()"

It throws an "AggregateException", and within that aggregate exception one of them are "WebException", looking at the implementation for "IsInitialized", it doesn't take into consideration that it's using async (as it's just trying to catch "WebException").

I can try and create a pull request if I can figure out how the build the thing.

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Feb 27, 2017

@MatthewLymer Thanks! I've taken the opportunity to rebase this branch against master and fix the merge conflict, and cherry-pick your commit.

qmfrederik commented Feb 27, 2017

@MatthewLymer Thanks! I've taken the opportunity to rebase this branch against master and fix the merge conflict, and cherry-pick your commit.

@MatthewLymer

This comment has been minimized.

Show comment
Hide comment
@MatthewLymer

MatthewLymer Feb 27, 2017

Contributor

Thanks @qmfrederik, do you know when the nuget package will be updated? I'd like to do more testing.

I'm referring to the one at https://www.nuget.org/packages/CoreCompat.Selenium.WebDriver, or do you even manage that one?

Contributor

MatthewLymer commented Feb 27, 2017

Thanks @qmfrederik, do you know when the nuget package will be updated? I'd like to do more testing.

I'm referring to the one at https://www.nuget.org/packages/CoreCompat.Selenium.WebDriver, or do you even manage that one?

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Feb 27, 2017

@MatthewLymer You are right, my bad. Should be fixed now.

I've updated the NuGet package: https://www.nuget.org/packages/CoreCompat.Selenium.WebDriver/3.2.0-beta002

qmfrederik commented Feb 27, 2017

@MatthewLymer You are right, my bad. Should be fixed now.

I've updated the NuGet package: https://www.nuget.org/packages/CoreCompat.Selenium.WebDriver/3.2.0-beta002

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Feb 27, 2017

@jimevans Another three months have passed :) and VS2017 is due to RTM in about ten days.

Let me know if there's anything I can do to help move this PR forward.

qmfrederik commented Feb 27, 2017

@jimevans Another three months have passed :) and VS2017 is due to RTM in about ten days.

Let me know if there's anything I can do to help move this PR forward.

@MatthewLymer

This comment has been minimized.

Show comment
Hide comment
@MatthewLymer

MatthewLymer Feb 27, 2017

Contributor

@qmfrederik You are a king among men.

Contributor

MatthewLymer commented Feb 27, 2017

@qmfrederik You are a king among men.

@MatthewLymer

This comment has been minimized.

Show comment
Hide comment
@MatthewLymer

MatthewLymer Feb 28, 2017

Contributor

@qmfrederik Hey Fred, sorry to keep pestering you, however I'm having an issue compiling a solution with your new version on nuget.

In the nuspec file there's a line that reads:

where the R on NameResolution should be uppercase. As a result, I am getting compile errors trying to get System.Net.NameResolution from nuget.

Is it possible for you to update the nuget package with this change?

Thanks for your help.

EDIT: For clarification, changing the file manually makes it work.

Contributor

MatthewLymer commented Feb 28, 2017

@qmfrederik Hey Fred, sorry to keep pestering you, however I'm having an issue compiling a solution with your new version on nuget.

In the nuspec file there's a line that reads:

where the R on NameResolution should be uppercase. As a result, I am getting compile errors trying to get System.Net.NameResolution from nuget.

Is it possible for you to update the nuget package with this change?

Thanks for your help.

EDIT: For clarification, changing the file manually makes it work.

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Mar 1, 2017

@MatthewLymer Sure, I just fixed that and uploaded a new version to NuGet: https://www.nuget.org/packages/CoreCompat.Selenium.WebDriver/3.2.0-beta003

Hope it helps!

qmfrederik commented Mar 1, 2017

@MatthewLymer Sure, I just fixed that and uploaded a new version to NuGet: https://www.nuget.org/packages/CoreCompat.Selenium.WebDriver/3.2.0-beta003

Hope it helps!

@Elufimov

This comment has been minimized.

Show comment
Hide comment
@Elufimov

Elufimov Mar 8, 2017

VS 2017 released. It seems time is come for this pull request. Any comments from selenium team?

Elufimov commented Mar 8, 2017

VS 2017 released. It seems time is come for this pull request. Any comments from selenium team?

@disophisis

This comment has been minimized.

Show comment
Hide comment
@disophisis

disophisis Mar 10, 2017

Are there plans for including .net core support for the selenium.support library? Things like webdriverwait and expectedconditions don't appear to be available here.

disophisis commented Mar 10, 2017

Are there plans for including .net core support for the selenium.support library? Things like webdriverwait and expectedconditions don't appear to be available here.

@ipjohnson

This comment has been minimized.

Show comment
Hide comment
@ipjohnson

ipjohnson Mar 11, 2017

I'm interested as well to see this move forward. Maybe at least get something out into beta to start playing with?

ipjohnson commented Mar 11, 2017

I'm interested as well to see this move forward. Maybe at least get something out into beta to start playing with?

@Lutando

This comment has been minimized.

Show comment
Hide comment
@Lutando

Lutando Mar 29, 2017

I am interested in this. What is the status of Selenium on .NET Core?

Lutando commented Mar 29, 2017

I am interested in this. What is the status of Selenium on .NET Core?

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Apr 2, 2017

Member

There are plans to implement this, but people should get used to the idea that it will have to wait until .NET Core/.NET Standard 2.0.

Member

jimevans commented Apr 2, 2017

There are plans to implement this, but people should get used to the idea that it will have to wait until .NET Core/.NET Standard 2.0.

@MartinJohns

This comment has been minimized.

Show comment
Hide comment
@MartinJohns

MartinJohns Apr 2, 2017

@jimevans I think what some other people also bothers is the communication. Your last statement was "until the development tooling catches up with developing for .NET Core", which happened by now with the Release of VS15. November 2016 you said "a little more patience" (which of course is relative). Since then there was no statement of a member, and of course people are curious what's the state of this issue.

Now you gave another statement which post-pones this issue until at least Q3.

MartinJohns commented Apr 2, 2017

@jimevans I think what some other people also bothers is the communication. Your last statement was "until the development tooling catches up with developing for .NET Core", which happened by now with the Release of VS15. November 2016 you said "a little more patience" (which of course is relative). Since then there was no statement of a member, and of course people are curious what's the state of this issue.

Now you gave another statement which post-pones this issue until at least Q3.

@Lutando

This comment has been minimized.

Show comment
Hide comment
@Lutando

Lutando Apr 2, 2017

@MartinJohns I would have to agree with @jimevans on having to wait for net standard 2.0. Even though at the moment the target platforms are not as much of a moving target (from an API standpoint) as it was before, net standard 2.0 is going to give library builders a common baseline to make libraries across all of the other .net target frameworks. It makes sense, it's just that other vendors have jumped on board developing in lock step with the evolution of dotnet core, and others haven't. Its basically a less risky path for them to take.

Lutando commented Apr 2, 2017

@MartinJohns I would have to agree with @jimevans on having to wait for net standard 2.0. Even though at the moment the target platforms are not as much of a moving target (from an API standpoint) as it was before, net standard 2.0 is going to give library builders a common baseline to make libraries across all of the other .net target frameworks. It makes sense, it's just that other vendors have jumped on board developing in lock step with the evolution of dotnet core, and others haven't. Its basically a less risky path for them to take.

@MartinJohns

This comment has been minimized.

Show comment
Hide comment
@MartinJohns

MartinJohns Apr 2, 2017

@Lutando I agree too that it makes the most sense to wait for .NET Standard 2.0. I just pointed out the lack in communication in this regard.

MartinJohns commented Apr 2, 2017

@Lutando I agree too that it makes the most sense to wait for .NET Standard 2.0. I just pointed out the lack in communication in this regard.

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Apr 2, 2017

Member

@MartinJohns Okay, yes, I'll cop to having communicated less than effectively in this particular issue. Mea culpa. I'm sorry to have disappointed you.

Member

jimevans commented Apr 2, 2017

@MartinJohns Okay, yes, I'll cop to having communicated less than effectively in this particular issue. Mea culpa. I'm sorry to have disappointed you.

@Elufimov

This comment has been minimized.

Show comment
Hide comment
@Elufimov

Elufimov Apr 2, 2017

Can anyone explain why targeting to .net standard 1.6 is a bad thing to do? Migration to 2.0 will trigger big refactoring? There is no backward compatibility from 1.6 to 2.0?

Elufimov commented Apr 2, 2017

Can anyone explain why targeting to .net standard 1.6 is a bad thing to do? Migration to 2.0 will trigger big refactoring? There is no backward compatibility from 1.6 to 2.0?

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Apr 2, 2017

Member

@Elufimov I'd be happy to. The 2.0 API surface reduces the number of hacks required in the .NET bindings source code from a messy large amount to a manageable handful. The resources of the maintainers of the .NET bindings are incredibly limited, and anything that increases maintenance cost is a non-starter. Additionally, the current approach requires taking additional dependencies that the project cannot afford to do, because of complexities in the project build system.

And the project build system is the way it is because of the requirements of a multi-language repository, and the need to create as simple a distributable artifact as possible. Moreover, anyone dismissing the need for an incredibly simple distributable artifact to keep deployment scenarios easy for novice users is displaying an attitude incompatible with the values of the Selenium project.

Member

jimevans commented Apr 2, 2017

@Elufimov I'd be happy to. The 2.0 API surface reduces the number of hacks required in the .NET bindings source code from a messy large amount to a manageable handful. The resources of the maintainers of the .NET bindings are incredibly limited, and anything that increases maintenance cost is a non-starter. Additionally, the current approach requires taking additional dependencies that the project cannot afford to do, because of complexities in the project build system.

And the project build system is the way it is because of the requirements of a multi-language repository, and the need to create as simple a distributable artifact as possible. Moreover, anyone dismissing the need for an incredibly simple distributable artifact to keep deployment scenarios easy for novice users is displaying an attitude incompatible with the values of the Selenium project.

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Apr 2, 2017

@jimevans Based on your last message, it seems you are rejecting this PR.

Can you just elaborate on a couple of the reasons you cite?

  • You refer to this PR as "messy large amount of hacks". That's not how I think of it. It's true that about 50% of the changes in this PR boil down to #ifdef'ing out the [Serializable] attribute since .NET Remoting is being deprecated. That said, [Serializable] is still available in one of the .NET Core packages, so we could add it back - reducing the size of this PR.
  • You mention this approach takes additional dependencies. I'm at loss here - which dependencies did I add? If anything, the number of dependencies went down because you're no longer depending on the entire .NET API surface (NETStandard.Library), but a targeted subset of it. Again, if this is an issue this can be reverted.

I understand time is limited. At the same time there are a couple of people on this thread and on the other .NET Core-related PRs who have expressed willingness to help out.

But we need to know whether what we're trying achieve (.NET Core 1.x support) will ever be accepted as a PR, and whether you're willing to have non-maintainers contribute towards this (both for .NET Core 1.x and 2.x).

qmfrederik commented Apr 2, 2017

@jimevans Based on your last message, it seems you are rejecting this PR.

Can you just elaborate on a couple of the reasons you cite?

  • You refer to this PR as "messy large amount of hacks". That's not how I think of it. It's true that about 50% of the changes in this PR boil down to #ifdef'ing out the [Serializable] attribute since .NET Remoting is being deprecated. That said, [Serializable] is still available in one of the .NET Core packages, so we could add it back - reducing the size of this PR.
  • You mention this approach takes additional dependencies. I'm at loss here - which dependencies did I add? If anything, the number of dependencies went down because you're no longer depending on the entire .NET API surface (NETStandard.Library), but a targeted subset of it. Again, if this is an issue this can be reverted.

I understand time is limited. At the same time there are a couple of people on this thread and on the other .NET Core-related PRs who have expressed willingness to help out.

But we need to know whether what we're trying achieve (.NET Core 1.x support) will ever be accepted as a PR, and whether you're willing to have non-maintainers contribute towards this (both for .NET Core 1.x and 2.x).

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Apr 3, 2017

Member

If you're asking if this feature is desired, the answer is yes. I will not be discussing a timeframe for its availability, at least not for the next few weeks. I have many other obligations, some relating to Selenium (like a concentrated push for W3C spec compliance), some not, that are pressing during that period. These obligations prevent me from, among other things, exploring whether the enhanced tooling options of VS17 will allow the project build process to produce a consumable artifact using .NET Core (Release Candidate versions of that tooling indicated that it will be more difficult with the new build tools to build the .NET bindings under that system than before, implying a build system overhaul will be required).

The API surface isn't, and cannot be, my only concern. Integrating into the project as a whole must be considered. I can understand reasonable people disagreeing on this point. This is nevertheless my current view.

This will be my last comment on this PR for the foreseeable future. I mention this to set expectation that a lack of communication does not indicate a lack of desire. This feature will happen, but I'm unwilling to discuss a timetable for implementation.

Member

jimevans commented Apr 3, 2017

If you're asking if this feature is desired, the answer is yes. I will not be discussing a timeframe for its availability, at least not for the next few weeks. I have many other obligations, some relating to Selenium (like a concentrated push for W3C spec compliance), some not, that are pressing during that period. These obligations prevent me from, among other things, exploring whether the enhanced tooling options of VS17 will allow the project build process to produce a consumable artifact using .NET Core (Release Candidate versions of that tooling indicated that it will be more difficult with the new build tools to build the .NET bindings under that system than before, implying a build system overhaul will be required).

The API surface isn't, and cannot be, my only concern. Integrating into the project as a whole must be considered. I can understand reasonable people disagreeing on this point. This is nevertheless my current view.

This will be my last comment on this PR for the foreseeable future. I mention this to set expectation that a lack of communication does not indicate a lack of desire. This feature will happen, but I'm unwilling to discuss a timetable for implementation.

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Apr 3, 2017

Build tooling and API surface are orthogonal. I guess I just fail to understand - what prevents you from accepting changes that make the C# code compatible with .NET Core 1.x or 2.x so that others who want to, can compile Selenium for .NET Core from source?

If you find this PR is a hack, just let me know which parts you disagree with and we'll get them moved out.

As to the build process, it looks like Buck has very rudimentary support for .NET; for example, it only supports a couple of versions of the .NET Framework (these versions are hard-coded). So yes, either Buck needs better support for .NET or you use the CLI features of Buck to invoke dotnet.exe. But it can be done independently of the API surface changes.

qmfrederik commented Apr 3, 2017

Build tooling and API surface are orthogonal. I guess I just fail to understand - what prevents you from accepting changes that make the C# code compatible with .NET Core 1.x or 2.x so that others who want to, can compile Selenium for .NET Core from source?

If you find this PR is a hack, just let me know which parts you disagree with and we'll get them moved out.

As to the build process, it looks like Buck has very rudimentary support for .NET; for example, it only supports a couple of versions of the .NET Framework (these versions are hard-coded). So yes, either Buck needs better support for .NET or you use the CLI features of Buck to invoke dotnet.exe. But it can be done independently of the API surface changes.

@Jetski5822

This comment has been minimized.

Show comment
Hide comment
@Jetski5822

Jetski5822 Apr 25, 2017

Hey @qmfrederik - any chance you would get time to update this branch to the latest selenium version and throw a new nuget package out there?

I have a lot of it working on Coypu, Firefox however is throwing me issues and I need 3.4.0 to get it working.

Jetski5822 commented Apr 25, 2017

Hey @qmfrederik - any chance you would get time to update this branch to the latest selenium version and throw a new nuget package out there?

I have a lot of it working on Coypu, Firefox however is throwing me issues and I need 3.4.0 to get it working.

@YevgeniyShunevych YevgeniyShunevych referenced this pull request May 3, 2017

Closed

.NET Core Support #22

@jfoshee

This comment has been minimized.

Show comment
Hide comment
@jfoshee

jfoshee May 17, 2017

Thanks for all the work here. I'm trying out Selenium for the first time w/ the temporary CoreCompat package. It seems this package may have the issue of missing .js files described here: #3137

jfoshee commented May 17, 2017

Thanks for all the work here. I'm trying out Selenium for the first time w/ the temporary CoreCompat package. It seems this package may have the issue of missing .js files described here: #3137

@Elufimov Elufimov referenced this pull request Jun 2, 2017

Closed

Support .NET Standard 2.0 #4106

@ramon-garcia

This comment has been minimized.

Show comment
Hide comment
@ramon-garcia

ramon-garcia Jun 2, 2017

I am also using the Corecompat packages like @jfoshee . And I am having problems with missing resources. If one tries to create a Firefox profile

var profile = new FirefoxProfile();

errors appear about missing resources:

Cannot find a file named '[nuget package dir]\packages\corecompat.sel
enium.webdriver\3.4.0-beta001\lib\netstandard1.5\webdriver.json' or an embedded resource with the id 'WebDriver.FirefoxPreferences'.

After placing a file webdriver.json in this directory:

OpenQA.Selenium.WebDriverException : Cannot find a file named '[nuget package dir]\corecompat.selenium.webdriver\3.4.0-beta001\lib\netstandard1.5\webdriver.xpi' or an embedd
ed resource with the id 'WebDriver.FirefoxExt.zip'.

ramon-garcia commented Jun 2, 2017

I am also using the Corecompat packages like @jfoshee . And I am having problems with missing resources. If one tries to create a Firefox profile

var profile = new FirefoxProfile();

errors appear about missing resources:

Cannot find a file named '[nuget package dir]\packages\corecompat.sel
enium.webdriver\3.4.0-beta001\lib\netstandard1.5\webdriver.json' or an embedded resource with the id 'WebDriver.FirefoxPreferences'.

After placing a file webdriver.json in this directory:

OpenQA.Selenium.WebDriverException : Cannot find a file named '[nuget package dir]\corecompat.selenium.webdriver\3.4.0-beta001\lib\netstandard1.5\webdriver.xpi' or an embedd
ed resource with the id 'WebDriver.FirefoxExt.zip'.

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jun 5, 2017

I am also having the same problem as @ramon-garcia. Looking at 3.4.0-beta001 in .NET Reflector, it appears that the DLL has no embedded resources. Is this correct? If not, it would explain the error messages stating that embedded resources are missing.

image

martincostello commented Jun 5, 2017

I am also having the same problem as @ramon-garcia. Looking at 3.4.0-beta001 in .NET Reflector, it appears that the DLL has no embedded resources. Is this correct? If not, it would explain the error messages stating that embedded resources are missing.

image

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jun 24, 2017

So it looks like all the missing files that prevent FirefoxProfile from working in the .NET Core version are referenced in this post-build step in the .csproj. How to do they end up in the compiled assembly? Presumably some custom build step rather than just being a normal <EmbeddedResource> MSBuild item defined in the .csproj file?

martincostello commented Jun 24, 2017

So it looks like all the missing files that prevent FirefoxProfile from working in the .NET Core version are referenced in this post-build step in the .csproj. How to do they end up in the compiled assembly? Presumably some custom build step rather than just being a normal <EmbeddedResource> MSBuild item defined in the .csproj file?

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jun 24, 2017

A second issue is that when running tests in Visual Studio with Selenium referenced via NuGet, is that NuGet packages get used directly from the cache, rather than being bin-copied into the output directory, so the implementation for getting the current directory to find driver files in the FileUtilities class fails in the .NET Core version.

I've worked around this in a custom build by doing this to remove the need to pass Directory.GetCurrentDirectory() as the driver path into every driver that is created so that the default constructor can just be used:

#if !NETSTANDARD1_5
// If we're shadow copying, get the directory from the codebase instead
if (AppDomain.CurrentDomain.ShadowCopyFiles)
 {
    Uri uri = new Uri(executingAssembly.CodeBase);
    currentDirectory = Path.GetDirectoryName(uri.LocalPath);
}
#else
//// HACK Visual Studio design-time references live in the NuGet cache
//// and aren't local, so current directory based on this file is wrong
if (currentDirectory.Contains(".nuget") || Assembly.GetEntryAssembly().Location.Contains(".nuget"))
{
    currentDirectory = Directory.GetCurrentDirectory();
}
#endif

The NETSTANDARD1_5 version needs its own proper solution to the Full Framework version's code that
for handles the Shadow Copy scenario for use with design-time debugging.

martincostello commented Jun 24, 2017

A second issue is that when running tests in Visual Studio with Selenium referenced via NuGet, is that NuGet packages get used directly from the cache, rather than being bin-copied into the output directory, so the implementation for getting the current directory to find driver files in the FileUtilities class fails in the .NET Core version.

I've worked around this in a custom build by doing this to remove the need to pass Directory.GetCurrentDirectory() as the driver path into every driver that is created so that the default constructor can just be used:

#if !NETSTANDARD1_5
// If we're shadow copying, get the directory from the codebase instead
if (AppDomain.CurrentDomain.ShadowCopyFiles)
 {
    Uri uri = new Uri(executingAssembly.CodeBase);
    currentDirectory = Path.GetDirectoryName(uri.LocalPath);
}
#else
//// HACK Visual Studio design-time references live in the NuGet cache
//// and aren't local, so current directory based on this file is wrong
if (currentDirectory.Contains(".nuget") || Assembly.GetEntryAssembly().Location.Contains(".nuget"))
{
    currentDirectory = Directory.GetCurrentDirectory();
}
#endif

The NETSTANDARD1_5 version needs its own proper solution to the Full Framework version's code that
for handles the Shadow Copy scenario for use with design-time debugging.

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jun 24, 2017

Another fun compatibility bug, as described here, a dependency needs to be taken on System.Text.Encoding.Codepages and then additional codepages registered using code in ZipStorer so that custom Firefox profiles can be used, otherwise an exception is thrown when the embedded ZIP resource is accessed:

<PackageReference Include="System.Text.Encoding.Codepages" Version="4.3.0" />
static ZipStorer()
{
#if NETSTANDARD1_5
    // See https://github.com/dotnet/corefx/issues/10054#issuecomment-254877319
    // and https://github.com/dotnet/corefx/issues/11715.
    Encoding.RegisterProvider(CodePagesEncodingProvider.Instance);
#endif

    defaultEncoding = Encoding.GetEncoding(437);
}

To reproduce without the fix:

// Create a custom profile
var profile = new FirefoxProfile();
profile.SetPreference("intl.accept_languages", "fr-CA");

// Create options that use the profile
var options = new FirefoxOptions() { Profile = profile };

// Try and create the driver - this will throw an exception
var driver = new FirefoxDriver(options);

martincostello commented Jun 24, 2017

Another fun compatibility bug, as described here, a dependency needs to be taken on System.Text.Encoding.Codepages and then additional codepages registered using code in ZipStorer so that custom Firefox profiles can be used, otherwise an exception is thrown when the embedded ZIP resource is accessed:

<PackageReference Include="System.Text.Encoding.Codepages" Version="4.3.0" />
static ZipStorer()
{
#if NETSTANDARD1_5
    // See https://github.com/dotnet/corefx/issues/10054#issuecomment-254877319
    // and https://github.com/dotnet/corefx/issues/11715.
    Encoding.RegisterProvider(CodePagesEncodingProvider.Instance);
#endif

    defaultEncoding = Encoding.GetEncoding(437);
}

To reproduce without the fix:

// Create a custom profile
var profile = new FirefoxProfile();
profile.SetPreference("intl.accept_languages", "fr-CA");

// Create options that use the profile
var options = new FirefoxOptions() { Profile = profile };

// Try and create the driver - this will throw an exception
var driver = new FirefoxDriver(options);
@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Jun 24, 2017

@martincostello Yes, the buck build process used by Selenium takes care of building those artifacts and then embedding them in the .NET assembly. Buck doesn't have very good tooling support for .NET Core (or the new .NET project system) that I know of.

If you want to have them embedded, for now, I would acquire those artifacts from another source - either the Selenium NuGet packages for full .NET Core, the selenium npm packages, or any other source.

Regarding the other fixes you've made for .NET Core, the code page fix is as far as I know required.

To get the path to the NuGet folder, there are two alternatives you may want to consider:

  • Because native assemblies are never copied over, yet you can P/Invoke them even if they are in the NuGet folder. So you can have them report back their location. This way you are always sure to get the path to the NuGet folder. You can find sample code here, let me know if this would work.
  • Alternatively, you can forcefully copy files to the output folder by embedding a .targets file in the NuGet package. Here's an example of how that could work.

If you want to have these changes integrated in the CoreCompat.Selenium package, send me a PR over in the CoreCompat repository and I'll update the NuGet packages. As an added benefit, they will also become part of this PR.

qmfrederik commented Jun 24, 2017

@martincostello Yes, the buck build process used by Selenium takes care of building those artifacts and then embedding them in the .NET assembly. Buck doesn't have very good tooling support for .NET Core (or the new .NET project system) that I know of.

If you want to have them embedded, for now, I would acquire those artifacts from another source - either the Selenium NuGet packages for full .NET Core, the selenium npm packages, or any other source.

Regarding the other fixes you've made for .NET Core, the code page fix is as far as I know required.

To get the path to the NuGet folder, there are two alternatives you may want to consider:

  • Because native assemblies are never copied over, yet you can P/Invoke them even if they are in the NuGet folder. So you can have them report back their location. This way you are always sure to get the path to the NuGet folder. You can find sample code here, let me know if this would work.
  • Alternatively, you can forcefully copy files to the output folder by embedding a .targets file in the NuGet package. Here's an example of how that could work.

If you want to have these changes integrated in the CoreCompat.Selenium package, send me a PR over in the CoreCompat repository and I'll update the NuGet packages. As an added benefit, they will also become part of this PR.

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jun 24, 2017

@qmfrederik I'll see about cherry-picking the encoding fixes over to your branch later today. I'll have a ponder over the NuGet thing. I think the best approach would be one that doesn't undercut the intended benefits of loading assemblies from the NuGet cache.

For the embedded resources, I have a fork where I've just embedded them in directly with a hack to fix the resource name and committed them into git. That isn't a proper fix, so I'll leave that for someone else to find a solution that's more idiomatic for the Selenium build process.

martincostello commented Jun 24, 2017

@qmfrederik I'll see about cherry-picking the encoding fixes over to your branch later today. I'll have a ponder over the NuGet thing. I think the best approach would be one that doesn't undercut the intended benefits of loading assemblies from the NuGet cache.

For the embedded resources, I have a fork where I've just embedded them in directly with a hack to fix the resource name and committed them into git. That isn't a proper fix, so I'll leave that for someone else to find a solution that's more idiomatic for the Selenium build process.

@martincostello

This comment has been minimized.

Show comment
Hide comment
@martincostello

martincostello Jun 24, 2017

@qmfrederik Have done a PR into your fork for the encoding fix: CoreCompat#3

martincostello commented Jun 24, 2017

@qmfrederik Have done a PR into your fork for the encoding fix: CoreCompat#3

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Jun 24, 2017

@martincostello Regarding the embedded resources, could another option be to kick of a Buck build and then fetch the resources from whatever location Buck drops them?

So you'd first run a "normal" Buck build and then dotnet pack to get the .NET Core stuff.

qmfrederik commented Jun 24, 2017

@martincostello Regarding the embedded resources, could another option be to kick of a Buck build and then fetch the resources from whatever location Buck drops them?

So you'd first run a "normal" Buck build and then dotnet pack to get the .NET Core stuff.

@qmfrederik

This comment has been minimized.

Show comment
Hide comment
@qmfrederik

qmfrederik Sep 22, 2017

For those on the CoreCompat package, I've published a new version. Here's what changed:

  • It has been rebased off Selenium 3.5.2.
  • On netcoreapp2.0, the package now uses .NET Core's version of System.Drawing, System.Drawing.Common.

qmfrederik commented Sep 22, 2017

For those on the CoreCompat package, I've published a new version. Here's what changed:

  • It has been rebased off Selenium 3.5.2.
  • On netcoreapp2.0, the package now uses .NET Core's version of System.Drawing, System.Drawing.Common.
@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Sep 22, 2017

Member

Just to provide some update, the current sources provide support for .NET Core 2.0 without additional dependencies. Using the go build script will delegate to the dotnet build command, and will produce artifacts (with the proper embedded resources) that are compatible with that platform. These will be released later today as 3.6

Member

jimevans commented Sep 22, 2017

Just to provide some update, the current sources provide support for .NET Core 2.0 without additional dependencies. Using the go build script will delegate to the dotnet build command, and will produce artifacts (with the proper embedded resources) that are compatible with that platform. These will be released later today as 3.6

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Sep 22, 2017

Member

Sorry, one additional update: there are known issues with this support. Please check the .NET CHANFELOG once version 3.6.0 of the bindings are released for known caveats.

Member

jimevans commented Sep 22, 2017

Sorry, one additional update: there are known issues with this support. Please check the .NET CHANFELOG once version 3.6.0 of the bindings are released for known caveats.

@jimevans

This comment has been minimized.

Show comment
Hide comment
@jimevans

jimevans Mar 12, 2018

Member

Given that .NET Core 2.0 support has been added for some time now, I'm going to close this PR. If there are other .NET Core-specific changes that need to be made, please submit a new PR.

Member

jimevans commented Mar 12, 2018

Given that .NET Core 2.0 support has been added for some time now, I'm going to close this PR. If there are other .NET Core-specific changes that need to be made, please submit a new PR.

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