arrayfire / forge

High Performance Visualization
222 stars 48 forks source link

Add support for Hunter package manager (optional: default off) #155

Closed caseymcc closed 6 years ago

caseymcc commented 6 years ago

What kind of change does this PR introduce? (Bug fix, feature, documentation update, new examples etc.) Support for new packaging framework

What is the current behavior? (You can also link to an open issue here) Adds support for Hunter package manager

What is the new behavior (if this is a feature change)? If cmake is given -DUSE_HUNTER=ON then hunter will fetch the require 3rd party libraries and compile them. By default, HUNTER usage is turned off.

9prady9 commented 6 years ago

@caseymcc Thank you for the rebase 👍

Do you have more changes coming in ? I don't see Hunter package lines for fontconfig or freetype on Unix platforms ? Was that intentional to use normal find_package calls to locate and use those two upstream libraries ?

On which OSes did you test out the hunter changes ?

caseymcc commented 6 years ago

I tested on Windows, my Ubuntu machine is down so I don't have a linux platform to test on at the moment (was waiting to see if the Travis builds failed). Hunter does not currently have a FontConfig project, so one needs to be written in order to use Hunter for it,. freetype however is in there and was added.

Doxygen, FontConfig, X11, and FreeImage where all listed as Quiet (optional) and considering Hunter does not have a project for them (x11 was listed in the hunter projects but I have no way to test it at the moment) I left them out. They could be found through the standard route but appear to not be needed.

caseymcc commented 6 years ago

freetype may not be found on linux if the case sentive nature is working against us. I checked the Hunter build and freetype is lower cased. You might change

find_package(FreeType)

to

find_package(freetype)

and see if it is located then. I am not sure if that change will affect the normal use. It maybe that the FindFreeType.cmake module will need to be lower cased as well for it to work properly on linux without hunter,

9prady9 commented 6 years ago

@caseymcc I have tried this on Arch Linux with GCC 6.* and it fails with the following error irrespective of the build type(Debug/Release).

[ 17%] Built target glsl_bin_target
src/backend/opengl/CMakeFiles/forge.dir/build.make:1013: warning: overriding recipe for target 'src/backend/opengl/libforge.so'
src/backend/opengl/CMakeFiles/forge.dir/build.make:1006: warning: ignoring old recipe for target 'src/backend/opengl/libforge.so'
Scanning dependencies of target forge
src/backend/opengl/CMakeFiles/forge.dir/build.make:1013: warning: overriding recipe for target 'src/backend/opengl/libforge.so'
src/backend/opengl/CMakeFiles/forge.dir/build.make:1006: warning: ignoring old recipe for target 'src/backend/opengl/libforge.so'
make[2]: *** No rule to make target 'optimized', needed by 'src/backend/opengl/libforge.so'.  Stop.
make[1]: *** [CMakeFiles/Makefile2:230: src/backend/opengl/CMakeFiles/forge.dir/all] Error 2
make: *** [Makefile:130: all] Error 2

Also, have a question for you, shouldn't we wrap find_package calls with IF-ELSE when hunter is being used. Isn't it redundant to call find_package as well?

caseymcc commented 6 years ago

I don't even know what that error is, I'll have to get my Ubuntu machine back up before I can test.

Its not redundant, with hunter the hunter_add_package just handles the fetching, compiling and installing (installed in hunter root under hunter/toolchain/config hashes) of the dependency. find_package is still required to add the project target (from the installed configs, again under hunter root) to the current cmake setup.

9prady9 commented 6 years ago

Ah, okay. I didn't know that. I thought hunter_add_package basically replaces the find_package call in CMake. Good to know!

True, that error did seem odd. I am seeing that one for first time, so I have no clue as to what it is.

On Thu 4 Jan, 2018, 7:51 PM caseymcc, notifications@github.com wrote:

I don't even know what that error is, I'll have to get my Ubuntu machine back up before I can test.

Its not redundant, with hunter the hunter_add_package just handles the fetching, compiling and installing (installed in hunter root under hunter/toolchain/config hashes) of the dependency. find_package is still required to add the project target (from the installed configs, again under hunter root) to the current cmake setup.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/arrayfire/forge/pull/155#issuecomment-355293850, or mute the thread https://github.com/notifications/unsubscribe-auth/ADHnOnjxtcadsQ_N0_7oi34EKTHzEDS6ks5tHN5wgaJpZM4RQ-EQ .

caseymcc commented 6 years ago

Is FontConfig really optional? It's listed as QUIET although on linux it is used without checking for _FOUND in the backend/opengl CMakeLists file. It is also included without any checks in font_impl.cpp.

caseymcc commented 6 years ago

eew, I think this is an ugly issue. I got my ubuntu machine back up and hit the same error. Digging through the make files built by cmake (no expert in this at all). When cmake built the make file for libforge and included libfreetype it put optimized and debug in front of the library location. I am not sure if these are valid in make files, but I am pretty sure the ";" after optimized and debug is wrong. Not sure where the error is being generated yet. Will have to poke around some more.

src/backend/opengl/libforged.so: optimized;/home/caseymcc/.hunter/_Base/4128ac8/8d47408/56284da/Install/lib/libfreetype.a;debug;/home/caseymcc/.hunter/_Base/4128ac8/8d47408/56284da/Install/lib/libfreetyped.a
caseymcc commented 6 years ago

Fixed the issue with freetype lib. Had to fix case, seems the find_package(FreeType) was running the custom FindFreeType.cmake you have and that was putting the optimized and debug commands into the make files (not sure why though). I am told those commands are the old style of including the libs. However I have now run into PIC issues which seems to stem from glfw's inclusion of libX11. Likely glfw ot libX11 is being built as static and since libforge is forced to SHARED likely it. Need to force the rest of the libs to SHARED I think, however BUILD_SHARED_LIBS does not seem to be affecting it.

I also altered FontConfig to be required on linux as it seems integral with finding fonts on linux.

Brain is hurting at the moment so l will like have to look at it later.

9prady9 commented 6 years ago

I think changing FONTCONFIG or FreeType on Windows and Linux is not required. The existing cmake files have been tested on both Windows & Linux and they work as expected without hunter.

FONTCONFIG is deliberately kept as QUIET to avoid platform specific IF flags. On Unix platform, this basically finds and adds the package silently but still reports an error when it is not found. On Windows, it is not required and code doesn't include it, thus it avoids it.

Regarding FindFreeTpye.cmake file, as you have already observed it doesn't add any flags to the freetype library, all it is doing is to create a modern cmake target FreeType::FreeType. Using freetype caused name conflicts on some machines in the past. Hence, I chose FreeType.

I will try to look into it today. Note that Makefile is complaining about a target called optimized, we don't craete such target in our CMake, so the question is who is adding this target - looks like hunter is adding this target because we didn't see such target earlier to hunter support.

9prady9 commented 6 years ago

I think the error related to PIC is due to the fact that hunter packages builds are all static in nature.

caseymcc commented 6 years ago

It says target but it is really talking about a library. The error is coming from the linker (not cmake) so target is from that point of view. In the make files built by cmake , it tells the linker to link against the following

src/backend/opengl/libforged.so: optimized;/home/caseymcc/.hunter/_Base/4128ac8/8d47408/56284da/Install/lib/libfreetype.a;debug;/home/caseymcc/.hunter/_Base/4128ac8/8d47408/56284da/Install/lib/libfreetyped.a

Unfortunately, the linker is taking the above to mean that it is looking for a library called optimized to link against. Since it doesn't exist the error is thrown. I modified the make file directly and it fixed the issue. Then I modified the cmake to use find_package(freetype) and target freetype::freetype which also fixed cmake's build of the make files. I am still unsure as to how optimized and debug got put in the first time however I have verified that if find_package(FreeType) is used when using hunter, instead of getting the target info from the freetypeConfig cmake files it runs the FindFreeType.cmake file (in your CMakeModules folder) which finds freetype in the hunter/root (rather than the system one) and the error occurs. Changing the case fixed it on linux and your Travis test ran fine (something still not right on Appveyor, but should be easy to track down). The Travis test is without hunter implying that normal operation is working fine with the case change.

If you are not wanting the case change then in order to get it working I will have to use both case types and put them behind if/endif statements.

As for FontConfig it is required on linux, setting it to QUIET on a linux machine that does not have it will not throw an error until compile time which is less descriptive of the error that has actually happened. Having it REQUIRED (with an OS flag) in cmake lets the user know immediately that the library is missing.

caseymcc commented 6 years ago

I believe this is functional now, the error on AppVeyor appears to be an issue with the service itself. Here is a successful build right before it went haywire: https://ci.appveyor.com/project/9prady9/forge-jwb4e/build/1.0.16-master

9prady9 commented 6 years ago

@caseymcc I have restarted the build for windows: VS 2015 passed fine and VS2017 is on the go. Also, can you please clarify regarding the comments in the review. Thanks!

9prady9 commented 6 years ago

I think it is Appveyor config issue for VS2017 job. I will take care of it soon. Can you please address the feedback.

9prady9 commented 6 years ago

@caseymcc Also, one more small change. Can you please change the commit message to have one of the prefixes mentioned here.

commit message format for future references.

<prefix>: brief commit message

Additional details related to commit