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

librepo.h: Change how project files are included #271

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

mcrha
Copy link
Contributor

@mcrha mcrha commented Feb 6, 2023

Use path-reference in the public header, to avoid ambiguity for generic-like header files from other projects. Otherwise it would depend on the order of the include directories in the build time, which of the header file is picked.

@evan-goode evan-goode self-requested a review February 6, 2023 14:35
@evan-goode
Copy link
Member

Thank you, I understand the rationale for this change.

librepo.h isn't the only public header; some consumers of librepo #include util.h or yum.h, for instance. It would be best to keep the behavior consistent across all the headers. Are there any drawbacks to using the path-reference style throughout all the headers in the project?

@mcrha
Copy link
Contributor Author

mcrha commented Feb 7, 2023

It would be best to keep the behavior consistent across all the headers.

I agree.

Are there any drawbacks to using the path-reference style throughout all the headers in the project?

I'm not aware of anything. The public headers should be referenced with the path. I'll update the pull request.

mcrha added 3 commits February 7, 2023 09:42
Use path-reference in the public header, to avoid ambiguity
for generic-like header files from other projects. Otherwise
it would depend on the order of the include directories
in the build time, which of the header file is picked.
It is not included by any public header. The other internal
headers are not installed as well.
This may help with rebuild on change in the header files, having them
properly tracked by the build scripts.
@mcrha
Copy link
Contributor Author

mcrha commented Feb 7, 2023

I updated the pull request and split the changes into three commits:

  • one changes the #include-s to be with the path
  • other to not install an internal header
  • the last to claim public and internal header files as the project files.

The last change might help the build scripts to properly detect changes in these files, but it's untested.

Copy link
Member

@evan-goode evan-goode left a comment

Choose a reason for hiding this comment

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

Thanks, looks good!

@jan-kolarik
Copy link
Member

Nice, thanks for the improvement!

@jan-kolarik jan-kolarik merged commit de4f123 into rpm-software-management:master Feb 16, 2023
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.

3 participants