Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Macro DISALLOW_COPY_AND_AS#clude/file.h #1406

Closed
zaki699 opened this issue May 30, 2024 · 5 comments · Fixed by #1407 or #1408
Closed

Macro DISALLOW_COPY_AND_AS#clude/file.h #1406

zaki699 opened this issue May 30, 2024 · 5 comments · Fixed by #1407 or #1408
Assignees
Labels
priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Milestone

Comments

@zaki699
Copy link
Contributor

zaki699 commented May 30, 2024

Hi Everyone,

is that normal that include/file.h contains this line:

#include <packager/macros/classes.h>

From the README
These are the public headers for libpackager. They can only reference other
public headers or standard system headers. They cannot reference internal
headers (in packager/...) or third-party dependency headers (in
packager/third_party/...).

Is that a mistake or I am missing something ?

Regards,

@zaki699 zaki699 changed the title Macro DISALLOW_COPY_AND_AS#cude/file.h Macro DISALLOW_COPY_AND_AS#clude/file.h May 30, 2024
@joeyparrish
Copy link
Member

I think you are correct. This is a violation that would break header installation for the shared library build. macros/classes.h should be moved to the public headers.

@zaki699
Copy link
Contributor Author

zaki699 commented May 30, 2024

Yes that’s a bit of a problem on my side as I am currently using libpackager. I have to manually copy the file macros/classes into include which is not great. Should I create a pull request to add macros/classes to the public include ?

@joeyparrish
Copy link
Member

Yes, please!

@joeyparrish
Copy link
Member

There is a CMake target that is meant to catch errors like these. I'm going to figure out why it didn't.

@joeyparrish joeyparrish self-assigned this May 30, 2024
@joeyparrish joeyparrish added type: bug Something isn't working correctly priority: P2 Smaller impact or easy workaround labels May 30, 2024
@joeyparrish
Copy link
Member

Commands for configuring shared libs and building the link test, which compiles a simple application with libpackager and the public headers:

cmake -S . -B build/ -DCMAKE_BUILD_TYPE=Debug -G Ninja -DBUILD_SHARED_LIBS=ON
cmake --build build/ --parallel -- packager_link_test

It works. However, the main file includes only packager.h, which includes all the other header files except file.h. If we add file.h to packager.h, the build fails.

@github-actions github-actions bot added this to the v3.3 milestone May 30, 2024
joeyparrish added a commit that referenced this issue May 31, 2024
include/file.h is breaking header installation for the shared library build.  macros/classes.h must be included to the public headers.

Closes #1406

Co-authored-by: Zaki Ahmed <zaki.ahmed.perso@gmail.com>
Co-authored-by: Joey Parrish <joeyparrish@users.noreply.github.com>
@github-actions github-actions bot added the status: archived Archived and locked; will not be updated label Jul 30, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 30, 2024
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
priority: P2 Smaller impact or easy workaround status: archived Archived and locked; will not be updated type: bug Something isn't working correctly
Projects
None yet
2 participants