Skip to content

VS 2019 compatibility#449

Merged
jcoupey merged 5 commits intoVROOM-Project:masterfrom
gis-ops:nn_win_compat
Feb 4, 2021
Merged

VS 2019 compatibility#449
jcoupey merged 5 commits intoVROOM-Project:masterfrom
gis-ops:nn_win_compat

Conversation

@nilsnolde
Copy link
Copy Markdown
Contributor

@nilsnolde nilsnolde commented Feb 2, 2021 •
edited by jcoupey
Loading

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

nilsnolde
nilsnolde commented Feb 2, 2021
View reviewed changes
Comment thread .gitignore
nilsnolde
nilsnolde commented Feb 2, 2021
View reviewed changes
Comment thread src/utils/getopt_win.h
@nilsnolde
Copy link
Copy Markdown
Contributor Author

nilsnolde commented Feb 2, 2021

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
Copy Markdown
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
Copy Markdown
Contributor Author

nilsnolde commented Feb 3, 2021

have it live in the include directory

Sure, better idea anyways.

@nilsnolde
Copy link
Copy Markdown
Contributor Author

nilsnolde commented Feb 3, 2021

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

jcoupey
jcoupey reviewed Feb 3, 2021
View reviewed changes
Comment thread CHANGELOG.md Outdated
Comment thread src/structures/typedefs.h Outdated
nilsnolde
nilsnolde commented Feb 3, 2021
View reviewed changes
Comment thread src/main.cpp Outdated
@nilsnolde
Copy link
Copy Markdown
Contributor Author

nilsnolde commented Feb 3, 2021

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

@jcoupey jcoupey added this to the v1.10.0 milestone Feb 3, 2021
@nilsnolde
Copy link
Copy Markdown
Contributor Author

nilsnolde commented Feb 3, 2021

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..

jcoupey
jcoupey reviewed Feb 3, 2021
View reviewed changes
Copy link
Copy Markdown
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.

Comment thread CHANGELOG.md
@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

Reviewers

@jcoupey jcoupey jcoupey left review comments

Assignees

No one assigned

Labels

compilation enhancement

Projects

None yet

Milestone

v1.10.0

Development

Successfully merging this pull request may close these issues.

Make lib and main win compatible

2 participants

@nilsnolde @jcoupey