-
Notifications
You must be signed in to change notification settings - Fork 123
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
support third party library configuration in qmake #611
Conversation
Hopefully this would resolve #608 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know a lot of this is just moving code around, but it feels like a lot of work to avoid using/duplicating such a small amount of code. Still, it's a good refactor, so go for it.
Let's do think if _has_include is our reason to move to C++17, though. (Looks like it's in GCC back to v5)
ISO Standing Document 6 has recommendations that help simplify code like this. Are you familiar with it?
If you want to commit this now as a checkpoint, that's OK with me. Marked approved.
defs.h
Outdated
#include <zlib.h> // doesn't really belong here, but is missing elsewhere. | ||
#elif !ZLIB_INHIBITED | ||
#include "zlib.h" // doesn't really belong here, but is missing elsewhere. | ||
#ifdef ZLIB_H_SYSTEM_INCLUDE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we slowly recreating C++17's _has_include? I think that's a feature that's actually been in most compilers before it was formalized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. The build system may have both the system header and a copy in our source tree, and I want to respect the choice of the user instead of automatically deciding for them.
defs.h
Outdated
# include "zlib/zlib.h" // doesn't really belong here, but is missing elsewhere. | ||
#else | ||
# if HAVE_LIBZ | ||
# include <zlib.h> // doesn't really belong here, but is missing elsewhere. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Insteead of changing all our sources to look in multiple places, have you considered using -I to add zlib and such to the list that's searched?
It's admittedly inefficient, but it may be less icky to debug the combinations in the field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally didn't do that because I felt it was too easy too find the wrong zlib.h. If we use project path when using the library in our source tree it is unlikely we could find a system header file. If we use brackets when using the system header it is unlikely we will find a header in our source. I was trying to follow https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes which says "All of a project's header files should be listed as descendants of the project's source directory".
The conditional includes could be simplified if we wanted to update configure and CMakeLists. It could also be simplified like libusb if qmake didn't have a bug creating the vc template.
instead of trying to use HAVE_*, ENABLED_* *_INHIBITED and -I options in a consistent manner. We can revisit this after the demise of configure.
It looks like there is a small bug in qmake dependency generation. It effects shapefil.h and libusb.h. It doesn't effect zlib. The reason is that we #include "shapelib/shapefil.h" and "mac/libusb/libusb.h", but for zlib we don't use a path, i.e. "zlib.h". With qmake option -d -d -d you can see Dependency Directories messages. It appears that even though we added DEFINES that conditionally omit #include "shapelib/shapefil.h" that qmake dependency generation traces shapefil.h and libusb.h through the . in the Dependency Directories message. The compile works fine, i.e. it doesn't compile our library code when it shouldn't. gcc dependency generation based on the Makefile generated compile command with -M added also works correctly. This is only an issue if you run qmake asking for system libraries, e.g. WITH_SHAPELIB=pkgconfig or WITH_LIBUSB=pkgconfig, and then delete the shapelib or mac directories before you run make. In that case qmake will have erroneously added a dependency to our local library header file. Even though that file is not used by the compile it kills the make due to not being able to resolve the dependency. Note if you remove the directories before you run qmake the build works fine. I am tempted to let this go, changing the include paths so "#include shapefil.h" and "#include libusb.h" could be resolved from the path is going to ripple into configure.ac, Makefile.in, CMakeLists. I don't want to invest any time in the autotools flow except to move it to deprecated soon! Also, I would rather put energy into hierarchical compiles of third party libraries. |
The use of system libraries by distributions is common. This is partially supported in the configure flow, but distributions usually require patches to achieve the linking they desire.
This PR gets us a step closer to deprecation/removal of the autotools base flow.