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

initial VS2017 build support with VS2015 toolset #50

Closed
wants to merge 1 commit into from
Closed

initial VS2017 build support with VS2015 toolset #50

wants to merge 1 commit into from

Conversation

japj
Copy link

@japj japj commented Oct 27, 2017

Enable cef-binary build with VS2017 using the VS2015 toolset.

Please note that the VS2017 provided cmake is not automatically put in the path, so the detection might fail. For now I manually put it in my path.

@perlun
Copy link
Member

perlun commented Nov 8, 2017

Looks good to me. @amaitland or @jornh, do you see any objections to getting this merged?

@japj - what would be required to get it building with VS2017 using the native (VS2017) toolset? It's not doable at the moment because CEF is VS2015 only?

@amaitland
Copy link
Member

I personally would finish the work started in #46

If you are going ahead with this, which is totally up to you then the toolchain version is incorrect and comments should be added explaining what's going on.

@amaitland
Copy link
Member

I personally would finish the work started in #46

See 12dede8 my comments are inline.

@perlun I'm not actively working on the project, so just go with whatever works for you. Nuget.org says your Pending approval for the cef.redist.x64 and cef.redist.x86 packages, if you need me to remove and re-add you let me know. That should be the last piece you require in terms of permissions, then you're free to go nuts 😄

@japj
Copy link
Author

japj commented Nov 9, 2017

Unfortunately, appveyor does not have build machines with VS 2013, 2015 and 2017 installed (see https://www.appveyor.com/docs/build-environment/#pre-installed-software ). So building nuget packages with all binaries is a bit tricky I think.

VS2017 c++ toolset is abi compatible with 2015 as far as I understand.

Are people still using vs2013?

@amaitland
Copy link
Member

Unfortunately, appveyor does not have build machines with VS 2013, 2015 and 2017 installed (see https://www.appveyor.com/docs/build-environment/#pre-installed-software ). So building nuget packages with all binaries is a bit tricky I think.

Three versions is impractical size wise, I've never shipped a set of packages with more than two versions due to this. Adding VS2017 support doesn't necessarily mean it will be enabled by default.

Are people still using vs2013?

Removing VS2013 support essentially forces an upgrade to VC++2015. I would expect the number of support requests to double, maybe even triple directly after that upgrade is made. Nobody ever reads the release notes...

It's all in @perlun's capable hands now.

@perlun
Copy link
Member

perlun commented Nov 17, 2017

@perlun I'm not actively working on the project, so just go with whatever works for you. Nuget.org says your Pending approval for the cef.redist.x64 and cef.redist.x86 packages, if you need me to remove and re-add you let me know. That should be the last piece you require in terms of permissions, then you're free to go nuts 😄

Ah, thanks! I accepted these requests now.

If/when I have time to work on this, I'll let you all know. (It will probably not happen in an extremely near future, unless someone is willing to step in and pay for the work - just get in touch with me if you're reading this and want to discuss the terms.)

@perlun
Copy link
Member

perlun commented Dec 3, 2017

@japj Since we added VS2017 support in CefSharp in cefsharp/CefSharp#2179, it would be nice to have a new go at this now as well.

Would you have a chance to rebase it on top off the latest master and update the PR?

(We can still build the AppVeyor builds using VC2013 until we are ready to finally move over to VC2015++ redist, but I think we'll perhaps postpone that until CEF 63 or later.)

@perlun perlun self-requested a review December 3, 2017 19:54
@japj
Copy link
Author

japj commented Dec 3, 2017

ok, not sure if everything went ok now (bit of git uncertainty), first did the conflict resolution through the github web interface, which resulted in an additional merge commit. Redid it with a rebase upon master, hopefully it is ok now

@japj
Copy link
Author

japj commented Dec 6, 2017

@perlun is there anything that I still need to do to get this finished?

@perlun
Copy link
Member

perlun commented Dec 6, 2017

@perlun is there anything that I still need to do to get this finished?

Sorry, just hasn't had time to look at it yet. Will give it a look ASAP!

@perlun
Copy link
Member

perlun commented Dec 9, 2017

@japj Sorry, some more merge conflicts came via #61. Would I be asking for too much if you try rebasing this once more? (If you are unable, just let us know and someone else can give it a try instead.)

(In the long run, we should perhaps drop all other versions than VS2015 and VS2017, and use the VC++2015 redistributable - I filed #64 about the latter part now.)

@amaitland
Copy link
Member

Support for building with v141 added in commit 568d1f5

@amaitland amaitland closed this Dec 18, 2017
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