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

fixes for compiling with PDAL/untwine with MSVC/Windows #42904

Closed
wants to merge 1 commit into from

Conversation

gillins
Copy link
Contributor

@gillins gillins commented Apr 22, 2021

Description

Currently there are some problems compiling PDAL and untwine etc on Windows. Firstly, PDAL pulls in winsock2.h and parts of untwine and QGIS pull in windows.h before including PDAL (which by default includes winsock.h). This leads to compile errors due to the differently defined symbols in winsock.h and winsock2.h (see https://stackoverflow.com/questions/9153911/is-there-a-difference-between-winsock-h-and-winsock2-h/9168850).

One solution is to always ensure that winsock2.h is included before windows.h. The other (less invasive) solution is to define the WIN32_LEAN_AND_MEAN symbol during the compile which stops windows.h including winsock.h (and a few other optional headers). This is what standalone untwine does (https://github.com/hobu/untwine/blob/main/cmake/win32_compiler_options.cmake#L9) and this is what I've gone for here.

Only problem was that qgisapp.cpp uses ShellExecute() from shellapi.h which isn't included with windows.h due to WIN32_LEAN_AND_MEAN so I've had to add this include in.

There have also been a few recent (source) fixes to untwine for windows (hobuinc/untwine@c7f94c1 hobuinc/untwine#59 hobuinc/untwine#57). Are changes to untwine pulled in automatically, or should these changes be added to this PR?

xref: conda-forge/qgis-feedstock#178
cc: @SrNetoChan @hobu

@github-actions github-actions bot added this to the 3.20.0 milestone Apr 22, 2021
@SrNetoChan
Copy link
Member

Cc: @jef-n

@@ -513,7 +513,7 @@ target_link_libraries(qgis_app
libdxfrw
)

target_compile_definitions(qgis_app PRIVATE "-DQT_NO_FOREACH")
target_compile_definitions(qgis_app PRIVATE "-DQT_NO_FOREACH -DWIN32_LEAN_AND_MEAN")
Copy link
Member

Choose a reason for hiding this comment

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

already in line 425

@@ -181,6 +181,8 @@ class QgsNetworkLoggerWidgetFactory;

#ifdef Q_OS_WIN
#include <windows.h>
// because of WIN32_LEAN_AND_MEAN we need to include this explicitly
#include <shellapi.h>
Copy link
Member

Choose a reason for hiding this comment

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

already in qgisapp.cpp:487

@@ -111,6 +111,8 @@ set_target_properties(untwine PROPERTIES
RUNTIME_OUTPUT_DIRECTORY ${QGIS_OUTPUT_DIRECTORY}/${QGIS_LIBEXEC_SUBDIR}
)

target_compile_definitions(untwine PRIVATE WIN32_LEAN_AND_MEAN)
Copy link
Member

Choose a reason for hiding this comment

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

already in line 5.

IOW c5ea4b2 should already cover this fully.

@gillins
Copy link
Contributor Author

gillins commented Apr 22, 2021

Sorry! Somehow I missed that this had gone into master...

While I have your attention, how do you feel about hobuinc/untwine#59 and hobuinc/untwine@c7f94c1 which fix compilation for VC 2017 (needed for compatibility with Python see https://wiki.python.org/moin/WindowsCompilers )

@gillins gillins closed this Apr 22, 2021
@jef-n
Copy link
Member

jef-n commented Apr 22, 2021

While I have your attention, how do you feel about hobu/untwine#59 and hobu/untwine@c7f94c1 which fix compilation for VC 2017 (needed for compatibility with Python see https://wiki.python.org/moin/WindowsCompilers )

Not sure - I use VC2019 for master and it builds fine as is - if it also builds with those changes - fine with me. I just didn't check yet.

@gillins
Copy link
Contributor Author

gillins commented Apr 25, 2021

Thanks @jef-n !

@jef-n
Copy link
Member

jef-n commented Apr 25, 2021

Thanks @jef-n !

np

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