Skip to content

cmake: Respect --prefix for headers during cmake --install #1060

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Apr 14, 2023
Merged

cmake: Respect --prefix for headers during cmake --install #1060

merged 1 commit into from
Apr 14, 2023

Conversation

ericvw
Copy link

@ericvw ericvw commented Apr 10, 2023

Removing CMAKE_INSTALL_PREFIX allows for cmake --install <build-dir> --prefix=<installation-prefix> to install headers at <installation-prefix>/include at install time.

When CMAKE_INSTALL_PREFIX is present in INSTALL(), it is hard-coded during configuration (e.g., cmake -S .), which disallows it from being set at install time.

Closes: #782

@codecov-commenter
Copy link

codecov-commenter commented Apr 10, 2023 •
edited
Loading

Codecov Report

Merging #1060 (5ce1f4d) into c_master (8160ede) will not change coverage.
The diff coverage is n/a.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files 
@@            Coverage Diff            @@
##           c_master    #1060   +/-   ##
=========================================
  Coverage     55.45%   55.45%           
=========================================
  Files             8        8           
  Lines          1044     1044           
=========================================
  Hits            579      579           
  Misses          465      465

@redboltz
Copy link
Contributor

It seems that the CI (Linux) doesn't start. I think that it is caused by ubuntu18.04 end of support. And your PR is the first one after that.

I guess that the following line should be updated to ubuntu-latest or ubuntu-20.04. Could you update it in your PR ?

msgpack-c/.github/workflows/gha.yml

Line 69 in 8b57d2c

runs-on: ubuntu-18.04

@ericvw
Copy link
Author

ericvw commented Apr 11, 2023

I guess that the following line should be updated to ubuntu-latest or ubuntu-20.04. Could you update it in your PR ?

Sure. Which one do you prefer?

@ericvw ericvw force-pushed the cmake-respect-prefx-at-install branch from 5ce1f4d to 1d87bd8 Compare April 11, 2023 15:38
@ericvw
Copy link
Author

ericvw commented Apr 11, 2023

I went with ubuntu-latest to be aligned with macos-latest.

@ericvw ericvw force-pushed the cmake-respect-prefx-at-install branch 2 times, most recently from f966327 to 1e5aa18 Compare April 11, 2023 15:48
@ericvw
Copy link
Author

ericvw commented Apr 11, 2023

There is more work involved in updating Linux CI. I will work on that in a separate PR as it is becoming more involved.

@ericvw
Copy link
Author

ericvw commented Apr 11, 2023

I created #1061 to get Linux CI updated…

@ericvw ericvw mentioned this pull request Apr 11, 2023
Merged
@ericvw
cmake: Respect --prefix for headers during cmake --install
80f6000 
Removing CMAKE_INSTALL_PREFIX allows for `cmake --install <build-dir>
--prefix=<installation-prefix>` to install headers at
`<installation-prefix>/include` at install time.

When CMAKE_INSTALL_PREFIX is present in `INSTALL()`, it is hard-coded
during configuration (e.g., `cmake -S .`), which disallows it from being
set at install time.
@ericvw ericvw force-pushed the cmake-respect-prefx-at-install branch from 1e5aa18 to 80f6000 Compare April 12, 2023 13:22
@ericvw
Copy link
Author

ericvw commented Apr 12, 2023

@redboltz, I rebased this PR against the latest c_master branch.

@redboltz
Copy link
Contributor

Thank you! merged.

@redboltz redboltz merged commit cd67366 into msgpack:c_master Apr 14, 2023
@dundargoc dundargoc mentioned this pull request Oct 17, 2023
Merged
@redboltz redboltz added the C label Apr 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ericvw @codecov-commenter @redboltz