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

VS 2019 compatibility #449

Merged
merged 5 commits into from
Feb 4, 2021
Merged

Conversation

nilsnolde
Copy link
Contributor

@nilsnolde nilsnolde commented Feb 2, 2021

fixes #448

Note, I only tested building the static vroom library and the vroom main executable with these changes. I confirmed with example_2 that it works in an offline scenario. I didn't have the time (yet) to try anything else.

Also, no idea how to manually build this with VS toolchain (and zero interest wanting to know;)), I actually used CMake. Which I'll contribute separately (gotta give that a test on linux first).

In any case, the changes are a few missing includes and some ifdefs, nothing intrusive. Oh, and getopt is not available for MSVC, so I needed to borrow one from mingw64.

Tasks

  • Update CHANGELOG.md (remove if irrelevant)
  • review

.gitignore Show resolved Hide resolved
src/utils/getopt_win.h Outdated Show resolved Hide resolved
@nilsnolde
Copy link
Contributor Author

And don't sweat it if you don't want to include VS support.. This is really easy to maintain and patch in a fork. Same with CMake support..

@nilsnolde nilsnolde changed the title compatible with VS 2019 now VS 2019 compatibility Feb 2, 2021
@jcoupey
Copy link
Collaborator

jcoupey commented Feb 3, 2021

Thanks for submitting a PR! I have no real interest for win builds myself and I guess few users have based on the low number of complaints we've had so far. ;-)

On the other hand, if this only means adjusting includes and some compilation-related stuff, I'm fine with including the changes.

What bothers me most is the need to have an unrelated getopt implementation in the codebase. Could we at least have it live in the include directory alongside the rapidjson dependency?

@nilsnolde
Copy link
Contributor Author

have it live in the include directory

Sure, better idea anyways.

@nilsnolde
Copy link
Contributor Author

Also tested ORS on windows quickly btw to make sure the HTTP stuff is working as well.

CHANGELOG.md Outdated Show resolved Hide resolved
src/structures/typedefs.h Outdated Show resolved Hide resolved
src/main.cpp Outdated Show resolved Hide resolved
@nilsnolde
Copy link
Contributor Author

Let me do the following once I'm back on win:

  • adapt changelog
  • fix the include for the new location of getopo_win.h
  • experiment with some directives to not include Win headers defining some crap macros

@nilsnolde
Copy link
Contributor Author

alright, impact on the codebase minimized:) good to go from my side

For future reference: this won't compile with MSVC unless you include the NOGDI preprocessor directive, which makes sure WinGDI.h is not pulled in causing all sorts of trouble. For good measure, include WIN32_LEAN_AND_MEAN, bringing down the compile time by a minute or two..

Copy link
Collaborator

@jcoupey jcoupey left a comment

Choose a reason for hiding this comment

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

Except for the changelog conflict, this is good to merge.

CHANGELOG.md Show resolved Hide resolved
@jcoupey jcoupey merged commit c450a93 into VROOM-Project:master Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make lib and main win compatible
2 participants