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

Obey CMAKE_INSTALL_PREFIX #7617

Closed
wants to merge 4 commits into from

Conversation

bryanloz-xilinx
Copy link
Contributor

Problem solved by the commit

If you want to install XRT into a user specified directory, XRT's CMake infra will not obey the user.
It will always put it into CMAKE_INSTALL_PREFIX/xrt

This change would make the CMake infra more obedient to the user.

Risks (if any) associated the changes in the commit

Could potentially break existing flows.

@bryanloz-xilinx
Copy link
Contributor Author

I'm only 90% sure I want to make this happen. This PR also serves as a test to see if this change breaks anything.

@gbuildx
Copy link
Collaborator

gbuildx commented Jul 6, 2023

Build failed :(

@gbuildx
Copy link
Collaborator

gbuildx commented Jul 6, 2023

Build failed :(

Copy link
Collaborator

@stsoe stsoe left a comment

Choose a reason for hiding this comment

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

This indeed breaks many different repos that use XRT as a submodule.

Also, CMake itself prepends CMAKE_INSTALL_PREFIX to all variables referenced in install directories so now you have two levels of CMAKE_INSTALL_PREFIX.

I agree it should be possible to set CMAKE_INSTALL_PREFIX to something different from what is hardwired, but the change in this PR will not do it.

Can we close and schedule a more thorough revamp of XRT's CMake infra.? It's long been on my wish list, but no resources assigned.

@stsoe stsoe added the do not merge hold off on merging label Jul 6, 2023
@stsoe
Copy link
Collaborator

stsoe commented Jul 6, 2023

I'm only 90% sure I want to make this happen. This PR also serves as a test to see if this change breaks anything.

Please do all testing outside the build pipeline in your own environment.

@bryanloz-xilinx
Copy link
Contributor Author

@stsoe Closing in favor of CMake revamp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge hold off on merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants