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

Visual Studio 2015 support #75

Closed
wants to merge 1 commit into from
Closed

Conversation

statico
Copy link
Contributor

@statico statico commented Oct 7, 2015

This pull request adds project files for MSVS 2015 and updates the wxWidgets dependency.

However, likely won't compile without wxWidgets/wxWidgets#111

@GitCop
Copy link

GitCop commented Oct 7, 2015

Thanks for contributing! Unfortunately there were the following style issues with your Pull Request:

  • Commit: c9e5de8
    • Your commit message body contains a line that is longer than 72 characters
    • Your subject line is longer than 50 characters

Please see https://github.com/agis-/git-style-guide (you can use git push -f to update this PR)


This message was auto-generated by https://gitcop.com

@vslavik
Copy link
Owner

vslavik commented Oct 8, 2015

The majority of this PR is unrelated to VS2015. You misunderstand what the deal with wxMSWDisableSettingHighDPIAware() is — it is not that it "appears to no longer be available" (wx really doesn't have that loose approach to backward compatibility), it is that it is explicitly documented to be 3.0 only because wx 3.0 did unconditionally opt-in HiDPI. And the only reason you had to deal with it is that you updated wx (without any in-commit explanation of why, by the way — so: why?) to the in-development master branch rather than to the latest stable series (WX_3_0_BRANCH) that WinSparkle uses and should continue to use.

So those bits are undesirable, which leaves 647c994. The only issue I have with that is that it adds these files manually. If you look at the vcxproj files (even the ones you added!), they have a header saying they were generated. I have no particular desire to maintain some files manually and have some generated, so I'd rather not do it like this. There are two "proper" ways to add 2015 projects:

  1. Add 2015 support to Bakefile, which should be trivial; or
  2. Merge all vcxproj files into one that is multi-targeting VS versions from 2010 onwards, remove support for 2008, and continue to maintain vcxproj manually instead of relying on Bakefile. This is possible with MSBuild and I think preferred (it helps with .NET bindings #73 too).

If you'd be willing to address that, it would be appreciated, otherwise I'm OK with merging this as a temporary solution, but without the upgrade-wx-to-master bit. It looks to me like the WX_3_0_BRANCH branch has 2015 support now too.

@statico
Copy link
Contributor Author

statico commented Oct 8, 2015

Thanks @vslavik. Let me preface by saying that I have no idea what I'm doing.

Regarding the wxWidgets upgrade, oops! That was a red herring so I've reverted that. There will be Please update wx/compiler.h to recognize this VC++ version messages while building with VS2015, but they're just warnings.

I had no idea this project used a build system -- I've never seen Bakefile before. I'm afraid that I don't have the time to contribute to Bakefile, nor am I proficient enough to make a multi-targeted VS project. Feel free to merge or close at will.

@vslavik
Copy link
Owner

vslavik commented Oct 11, 2015

Let me preface by saying that I have no idea what I'm doing.

That’s OK, that’s what code reviews are for ;)

There will be Please update wx/compiler.h to recognize this VC++ version messages while building with VS2015

That’s why I wrote about that you should instead update wxWidgets to the tip of the WX_3_0_BRANCH (which recognizes VS2015), not that you shouldn’t update it at all...

@vslavik
Copy link
Owner

vslavik commented Oct 11, 2015

Merged in e68bafd + 179d410.

@vslavik vslavik closed this Oct 11, 2015
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