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

Remove nmake build system #1137

Merged
merged 1 commit into from
Oct 8, 2018
Merged

Remove nmake build system #1137

merged 1 commit into from
Oct 8, 2018

Conversation

rouault
Copy link
Member

@rouault rouault commented Sep 28, 2018

This was announced as scheduled for PROJ 6.

My ongoing work on the iso19111 branch hasn't updated the makefile.vc stuff and only supports cmake builds for Windows

@kbevers
Copy link
Member

kbevers commented Sep 29, 2018

I tried to do something similar a week ago but never pushed the changes because I realized that our CMake setup can't produce a drop-in replacement for NMake. As far as I can tell from this PR it will also be the case here. I am okay with merging this but we need to make sure that we have a setup that is as close to being a drop-in replacement for users relying on NMake builds.

@rouault
Copy link
Member Author

rouault commented Sep 29, 2018

What is the issue exactly with the cmake build ?

@kbevers
Copy link
Member

kbevers commented Sep 29, 2018

What is the issue exactly with the cmake build ?

I failed to create a NMake generator that would output a DLL. I think we need the __cdecl qualifiers before functions to include in a DLL before that is possible. Or it may be that I am just a complete idiot with CMake and I have missed something obvious at the time...

@mloskot
Copy link
Member

mloskot commented Sep 29, 2018

How did you configure to build DLL? Have you set https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

@kbevers
Copy link
Member

kbevers commented Sep 29, 2018

How did you configure to build DLL?

I didn't do any changes to the CMake files, I just ran a cmake -G "NMake Makefiles" and then build with nmake. As I recall, I ran into some problems, the DLL was one of them. I am not near a windows computer right now so I can't be more specific than this. I'll look into this again on monday.

@mloskot
Copy link
Member

mloskot commented Sep 29, 2018

Typically, to build shared library it takes -DBUILD_SHARED_LIBS=ON which is a native cmake option, but some projects especially ancient cmake configs define their own custom equivalents. I don't recall what CMakeLists.txt of PROJ do, writing from mobile now but will try to check later

@rouault
Copy link
Member Author

rouault commented Sep 29, 2018

There's a dedicated -DBUILD_LIBPROJ_SHARED=ON switch which is used by appveyor.yml to test DLL builds

@kbevers
Copy link
Member

kbevers commented Sep 29, 2018

Maybe things aren't as bad as I thought. I'm probably not the only one who can't find their way around this stuff so maybe we should add something to the install/build instructions on how to do this.

@mloskot
Copy link
Member

mloskot commented Sep 29, 2018

There's a dedicated -DBUILD_LIBPROJ_SHARED=ON

Any reason to not to switch over to the official variable?
https://cmake.org/cmake/help/latest/variable/BUILD_SHARED_LIBS.html

I think less custom variables/options the better, self-descriptive, knowledge-transitive setup.

@rouault
Copy link
Member Author

rouault commented Sep 29, 2018

Any reason to not to switch over to the official variable?

I don't think so unless @hobu has another opinion (this comes from c90714e)

@kbevers
Copy link
Member

kbevers commented Oct 3, 2018

Okay, so to follow up on this, here's what I did in my previous test:

> C:\dev\build\nmakeproj>cmake -G "NMake Makefiles" ..\..\proj
> nmake

which after compilation doesn't createa DLL. Running cmake with -DBUILD_LIBPROJ_SHARED=ON produces a DLL so I guess that we can use the cmake generated nmake makefiles as a drop in replacement. At some point it would be nice to get it confirmed by someone that actually uses the DLL that it works for them if build this way.

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.

None yet

3 participants