Fix PackageConfig and Create the test

[yhmtsai]

This PR create the test and fix for PackageConfig
The error is mentioned in #1104.
close #1104

I add the existed lib into the list, and remove the duplication in the reversed to keep the last not first library in the list.
To keep the ordering of the libraries, this pr can not support circular dependence.
Also, pkgconfig of ginkgo isn't supported in MSVC.

From CMake PkgConfig TARGET, they only put the -framework, -L, -l to link_libraries but others in link_options.
Thus, the linking library with absolute path will be in link_option and cmake will put it before the compiled target such that it destroy the lib order. g++ <link_options> *.o <link_libraries> Ref: https://gitlab.kitware.com/cmake/cmake/-/blob/master/Modules/FindPkgConfig.cmake#L251

[upsj]

Partial review, didn't look at the algorithm yet

[upsj]

the pkgconfig support already provides CMake target support. We should probably also add the -std=c++14 flag to cflags

Suggested change

	target_link_libraries(test_exportbuild_pkgconfig PRIVATE ${GINKGO_LIBRARIES})
	target_link_libraries(test_exportbuild_pkgconfig PRIVATE ${GINKGO_LDFLAGS})
	target_include_directories(test_exportbuild_pkgconfig PRIVATE ${GINKGO_INCLUDE_DIRS})
	target_compile_features(test_exportbuild_pkgconfig PUBLIC cxx_std_14)
	target_link_libraries(test_exportbuild_pkgconfig PRIVATE ${GINKGO_CFLAGS})
	target_link_libraries(test_exportbuild_pkgconfig PRIVATE PkgConfig::GINKGO)

[yhmtsai]

I think using package config is more close to the original way not the CMake way

[upsj]

I would prefer the CMake approach, since that way we can't accidentally miss any part of the pkg-config settings.

[yhmtsai]

Although using the target approach make make adds -pthread to compile flag, it changes the linking order to lead failure.

/usr/bin/c++ /usr/lib/libhwloc.so /usr/lib/libpapi.so -Wl,-rpath -Wl,/usr/lib -Wl,--enable-new-dtags -pthread \
/usr/lib/libmpi.so CMakeFiles/test_exportbuild_pkgconfig.dir/root/ginkgo/test/test_install/test_install.cpp.o -o \
test_exportbuild_pkgconfig  -Wl,-rpath,/usr/local/lib /usr/local/lib/libginkgo.so /usr/local/lib/libginkgo_omp.so \
/usr/local/lib/libginkgo_cuda.so /usr/local/lib/libginkgo_reference.so /usr/local/lib/libginkgo_hip.so \
/usr/local/lib/libginkgo_dpcpp.so /usr/local/lib/libginkgo_device.so 

leads /usr/bin/ld: CMakeFiles/test_exportbuild_pkgconfig.dir/root/ginkgo/test/test_install/test_install.cpp.o: undefined reference to symbol 'papi_sde_init'

ginkgo.pc

-L/usr/local/lib -lginkgo /usr/lib/libhwloc.so -lginkgo_omp -lginkgo_cuda -lginkgo_reference -lginkgo_hip \
-lginkgo_dpcpp -lginkgo_device /usr/lib/libpapi.so -Wl,-rpath -Wl,/usr/lib -Wl,--enable-new-dtags -pthread \
/usr/lib/libmpi.so

it seems to put the lib with absolute path like hwloc, papi, mpi before the object file.
However, they should be after object file.
Do you know any documentation or keyword for this behavior?

[upsj]

I think the correct way to do things would be to output -L/usr/lib -lhwloc -lpapi -lmpi. Since pkg-config is Linux-only, we can rely on libraries being prefixed lib with a .so or .a suffix. I can't find any pkg-config file on my system that uses absolute paths

[yhmtsai]

I do not think it is the requirement from the png-config. cmake also change -L -l to the absolute path in the end. pkg-config may prefer we have hwloc.pc, papi.pc, mpi.pc, and any pkg-config file from the outside or third-party package. pkg-config is also supported on Windows, but I can not make it work in this pr

[upsj]

pkg-config has few explicit limitations, but on my entire system, I couldn't find a pkg-config file that uses absolute library paths directly, so we are definitely doing something unusual here. CMake's pkg-config support relies on that implicit assumption, treating every -l and -L parameter as library information and all other parameters as link options. This causes the order to be lost.
On the Windows side: We can use CMAKE_SHARED_LIBRARY_SUFFIX and friends to make it portable.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[thoasm]

LGTM!

[upsj]

I'm trying to figure out how to deal correctly with pkg-config, but it might be best not to use the IMPORTED_TARGET approach, otherwise things might get reordered arbitrarily, as you said.

[yhmtsai]

Using IMPORTED_TARGET can handle the pthread in compile and linking.
if using the original way, I can not put CFLAGS into compile_options directly.
that's why I still change the property of target not use the original plain link/compile

[upsj]

Makes sense, then let's leave it how it is

[upsj]

I attempted to get this right, since there are at least two bugs in the current implementation:

1. the link line should be -Lpath -lname
2. compile definitions do not need to have a leading -D, so we may need to add it.

Finally, more recent CMake versions add a $<HOST_LINK:...> generator expression to the MPI link flags, which honestly just makes me want to give up on this entirely.

The cleanest thing I could come up with is a59cc01, which includes a bit of cleanup

[yhmtsai]

we can not give it up because at least one user is using it.
If the absolute link path is not correct, we should fix it in our own target. If the target is made from the package, we should keep it.
When is the compile definition without -D? Is it from cmake?

[tcojean]

LGTM. The new logic here seems good to me. I think getting a generic solution is out of scope for this so it's OK that it cannot deal with circular deps or MSVC. We only want to provide something reasonable that the user can work from if they really have to.

This at least seems to be working with OpenCARP which relies on pkg-config.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[codecov]

Codecov Report

Base: 91.13% // Head: 91.63% // Increases project coverage by +0.49% 🎉

Coverage data is based on head (2c9e4ac) compared to base (bb21691).
Patch has no changes to coverable lines.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1109      +/-   ##
===========================================
+ Coverage    91.13%   91.63%   +0.49%     
===========================================
  Files          555      555              
  Lines        47614    47554      -60     
===========================================
+ Hits         43395    43576     +181     
+ Misses        4219     3978     -241     

Impacted Files	Coverage Δ
omp/reorder/rcm_kernels.cpp	94.44% <0.00%> (-3.09%)	⬇️
reference/base/index_set_kernels.cpp	94.20% <0.00%> (+0.08%)	⬆️
core/test/base/iterator_factory.cpp	100.00% <0.00%> (+32.43%)	⬆️
omp/factorization/factorization_kernels.cpp	79.45% <0.00%> (+79.45%)	⬆️
test/factorization/par_ilu_kernels.cpp	89.65% <0.00%> (+89.65%)	⬆️
omp/factorization/par_ilu_kernels.cpp	100.00% <0.00%> (+100.00%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.