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

Update install-rules.cmake #74

Closed
wants to merge 8 commits into from

Conversation

anders-wind
Copy link
Contributor

The current install-rules double indent the include folder: as in installation_prefix/include/libname/libname/header.hpp. At least when running: cmake --install build_folder_path --prefix=./installation_prefix

Removing these three lines seems to be a better default behavior. When we tried to get our library on vcpkg, this was an issue which blocked us from merging.

The current install rules double indent the include folder,
 like so: `include/libname/libname/header.hpp`
@friendlyanon
Copy link
Owner

friendlyanon commented Sep 23, 2022

This is #43

Please see the conversation linked in there and reconsider.


this was an issue which blocked us from merging.

The install include dir is a user-configurable cache variable and if you (or anyone packaging the project) wish to override this behavior, you may pass -DCMAKE_INSTALL_INCLUDEDIR=include to the configure command to achieve what you want in a cross-platform way.

Should this be documented in some form in the generated BUILDING.md document?

@anders-wind
Copy link
Contributor Author

Yea maybe - add some documentation on this explicitly - or maybe set the CMAKE_INSTALL_INCLUDEDIR to not just be package but <package>-<version> , that way its clear that it is two different concepts?

@anders-wind
Copy link
Contributor Author

Edited the install-rules.cmake again to reflect my comment above - no idea if it would work as I don't know exactly how your template logic works.

Copy link
Owner

@friendlyanon friendlyanon left a comment

Choose a reason for hiding this comment

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

I'm not sure how I feel about embedding the version. I guess anything is better than an unrelated system level dependency including the whole world anyway. The path is also customizable using built-in cache variables.

I'm curious, where did this cause an issue exactly? Can I take a look? Maybe that will give me a better insight hopefully.

Could you try your hand at maybe adding a section to BUILDING.md? Something that might be obvious to me might not be for someone else, so that could also help me better understand.

cmake-init/templates/common/cmake/install-rules.cmake Outdated Show resolved Hide resolved
@anders-wind
Copy link
Contributor Author

anders-wind commented Sep 24, 2022

I'm not sure how I feel about embedding the version. I guess anything is better than an unrelated system level dependency including the whole world anyway. The path is also customizable using built-in cache variables.

:D I think it seems like a really good idea to embed the version - You of course decide what you want to do here, I'll try it out in our projects and see if we run into any issues.

Could you try your hand at maybe adding a section to BUILDING.md? Something that might be obvious to me might not be for someone else, so that could also help me better understand.

Ill try to add a few words there. Maybe also as an inline comment in install-rules.cmake

I'm curious, where did this cause an issue exactly? Can I take a look? Maybe that will give me a better insight hopefully.
I'll give it a go in adding some text in building - potentially also as a comment in the install_rules?

So the final solution is exactly as you say: set CMAKE_INSTALL_INCLUDEDIR, but one of the maintainers of vcpkg had trouble using the library and started out by patching the project by deleting the line (the one I originally removed):
microsoft/vcpkg#26740.

Co-authored-by:
    friendlyanon <1736896+friendlyanon@users.noreply.github.com>
@friendlyanon
Copy link
Owner

friendlyanon commented Sep 24, 2022

Interesting, dg0yt did see the intended way to make things work the way you wanted, but curiously the suggestion to use relative paths did not make it into the port.

Another curiosity is this:

# Allow package maintainers to freely override the path for the configs
set(
{= name =}_INSTALL_CMAKEDIR "{% if lib %}${CMAKE_INSTALL_LIBDIR}/cmake{% else %}${CMAKE_INSTALL_DATADIR}{% end %}/${package}"
CACHE PATH "CMake package config location relative to the install prefix"
)
mark_as_advanced({= name =}_INSTALL_CMAKEDIR)

exists only so projects generated by cmake-init properly follow GNU conventions, but also provide a way for vcpkg specifically to control where the CMake package goes, as they always want the it to be in share (CMAKE_INSTALL_DATADIR).

@friendlyanon
Copy link
Owner

Hey, it's been 2 weeks. What's the status of this? I would like to make a release with the new warning flags and I would like to have the documentation changes from this PR in the release as well.

@anders-wind
Copy link
Contributor Author

On vacation until tomorrow evening. I can add some documentation Wednesday :)

@anders-wind
Copy link
Contributor Author

anders-wind commented Oct 12, 2022

Ive added a comment about why we nest the install include dir, and a section on how to make a vcpkg portfile.

Comment on lines 2 to 3
# CMAKE_INSTALL_INCLUDEDIR is nested in one more ${PROJECT_VERSION} directory to prevent inclusion of all other headers in the install prefix directory.
# See https://github.com/friendlyanon/cmake-init/issues/43 for more info
Copy link
Owner

Choose a reason for hiding this comment

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

Besides not keeping the 80 column limit and the period at the end, I also don't feel okay about a link back to this project.

The idea of cmake-init is that it generates projects that - in a perfect world - everyone would write to begin with, so links back feel icky to me.

Something more generic about the purpose of avoiding indirect inclusion of other projects in a shared prefix would fit better.

@@ -79,6 +79,37 @@ target_link_libraries(
{= name =}::{= name =}
)
```

### Creating a VCPKG port
Copy link
Owner

Choose a reason for hiding this comment

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

"Officially" vcpkg is used like a regular word in its docs, so it shouldn't be all caps.

The additional directory in the include path also affects every other form of packaging, there is no need to make it vcpkg specific. Something like a "Note to packagers" section with cautioning them to review the usage of CMAKE_INSTALL_INCLUDEDIR. This should also be guarded with {% if not exe %}.

@friendlyanon
Copy link
Owner

I decided to implement this change, but I felt that the wording in this PR was excessive with the involvement of vcpkg.

I included you as a co-author, since you convinced me to add this change and opened a PR for it. Thanks!

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.

2 participants