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

fix_third_party: set install libdir and fix spdlog includes #570

Merged
merged 2 commits into from
May 22, 2023

Conversation

acastanedam
Copy link
Contributor

With the update of the third-party libraries, qcor's compilation
crashes when try to link to the gtest and spdlog installed by
xacc. This PR fix that and allows for compilation of qcor without
touching qcor's configuration files.

@1tnguyen
Copy link
Member

1tnguyen commented Mar 9, 2023

Hi @acastanedam,

Thanks for the contribution. This all looks good.
Could you please create an Eclipse account and sign the CLA? I can merge this after the ECA validation passes.

Also, please make sure you sign off all the commits with the same email as the email in your Eclipse account. Your git commit didn't have a signature yet.

Thanks!

@acastanedam
Copy link
Contributor Author

Hi @1tnguyen ,

thanks for checking it. I did the registration process with ECA and signed the ECLA. About the signature I am not sure, what is happening. On the other, I am not sure if the changes are breaking some of the CI tasks, or this is coming from #569.

@1tnguyen
Copy link
Member

The CI check failures above should be fixed in master now. Would you mind merging main into your branch since I don't have write permission? Thanks!

With the update of the third-party libraries, qcor's compilation
crashes when try to link to the gtest and spdlog installed by
xacc. This PR fix that and allows for compilation of qcor without
touching qcor's configuration files.
@acastanedam
Copy link
Contributor Author

Hi @1tnguyen , excuse me for the delay. I've just re-based on the last master and push.

CMakeLists.txt Outdated
@@ -60,6 +60,7 @@ set(CMAKE_SKIP_INSTALL_RPATH OFF)
set(CMAKE_SKIP_RPATH OFF)
set(CMAKE_INSTALL_RPATH_USE_LINK_PATH ON)
set(CMAKE_EXPORT_COMPILE_COMMANDS TRUE)
set(CMAKE_INSTALL_LIBDIR "${CMAKE_INSTALL_PREFIX}/lib")
Copy link
Member

Choose a reason for hiding this comment

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

Now I see a problem with this line :)

It should be placed after this block: https://github.com/eclipse/xacc/blob/master/CMakeLists.txt#L102-L106
(CMAKE_INSTALL_PREFIX is not set to a proper default value yet at this point. Our CI scripts all use this default hence didn't set it explicitly)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see. I've just moved the line. Thanks!

@1tnguyen 1tnguyen merged commit f84b609 into eclipse:master May 22, 2023
17 checks passed
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

2 participants