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

"Namespaced" public headers files #165

Closed
SpaceIm opened this issue Oct 26, 2020 · 11 comments
Closed

"Namespaced" public headers files #165

SpaceIm opened this issue Oct 26, 2020 · 11 comments

Comments

@SpaceIm
Copy link
Contributor

SpaceIm commented Oct 26, 2020

public headers names are very common (Point.h, Frame.h, Parameter.h etc).
To properly live in a project with others dependencies, and avoid clashes, I would advice to add an intermediate folder (ezc3d for example ;) ), and update build scripts (and some #include because some of them take absolute path).

Obviously, it would be a breaking change in ezc3d interface...

@pariterre
Copy link
Member

pariterre commented Oct 26, 2020

Hi @SpaceIm !
I've read the linked issue, but I am not sure what is better for you guys. Do you mean that the files should be included #include "ezc3d/SomeFile.h"? If so, I think it definitely makes sense and I will make the changes soon!

@madebr
Copy link

madebr commented Oct 26, 2020

Hey @pariterre
At these lines:

ezc3d/CMakeLists.txt

Lines 100 to 102 in 7d40969

target_include_directories(${PROJECT_NAME} INTERFACE
$<INSTALL_INTERFACE:${${PROJECT_NAME}_INCLUDE_FOLDER}>
)

You set the include directories of the installed target to include/ezc3d.
set(${PROJECT_NAME}_INCLUDE_FOLDER include/${PROJECT_NAME})

This means that including a subheader using #include <ezc3d/Point.h> is not possible.
I suggest also adding include:

target_include_directories(${PROJECT_NAME} INTERFACE
    $<INSTALL_INTERFACE:${${PROJECT_NAME}_INCLUDE_FOLDER}>
    $<INSTALL_INTERFACE:include>
)

@pariterre
Copy link
Member

Considering that names are quite generic would not you suggest also to remove the first line? $<INSTALL_INTERFACE:${${PROJECT_NAME}_INCLUDE_FOLDER}>

@madebr
Copy link

madebr commented Oct 26, 2020

@pariterre
That would change the interface to your users.
Run the following github search query
It shows that most of your users include the headers using a plain #include "ezc3d.h".
Removing $<INSTALL_INTERFACE:${${PROJECT_NAME}_INCLUDE_FOLDER}> would break this.

It's easy to fix, but annoying.

@pariterre
Copy link
Member

pariterre commented Oct 26, 2020

@madebr
I tried with $<INSTALL_INTERFACE:${${PROJECT_NAME}_INCLUDE_FOLDER}>/.. so it adjust to whatever the folder is.
Does this PR #166 makes sense for you guys?

@madebr
Copy link

madebr commented Oct 26, 2020

It does the trick to me.

@pariterre
Copy link
Member

I am waiting for Travis to complete (actually start.. Don't know why it is so long lately...) and then I merge!

@pariterre
Copy link
Member

It is merge, hopefully this works all fine!

@SpaceIm
Copy link
Contributor Author

SpaceIm commented Oct 28, 2020

Yes it should work, but it doesn't solve potential clashes with headers from other libraries since ${${PROJECT_NAME}_INCLUDE_FOLDER} is still added to include dirs.

@pariterre
Copy link
Member

Indeed
I feel that since I am the main programmer on most of the software that use ezc3d, I should actually fix this properly. That definitely would make sense.
In any case, thanks for your input!

@pariterre
Copy link
Member

For posterity, I was about to change that, but it makes this complicated with the global includer. I've decided that unless someone else complaints about that specific issue, I will leave it as so

Thanks again!

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

No branches or pull requests

3 participants