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

fix: Break header installation for the shared library #1407

Merged

Conversation

zaki699
Copy link
Contributor

@zaki699 zaki699 commented May 30, 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

Copy link

google-cla bot commented May 30, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@zaki699 zaki699 closed this May 30, 2024
@joeyparrish
Copy link
Member

I started the tests for this... why did you close it?

@zaki699 zaki699 reopened this May 30, 2024
@zaki699
Copy link
Contributor Author

zaki699 commented May 30, 2024 via email

@joeyparrish
Copy link
Member

I edited your PR and added the necessary line (file.h) to packager.h to make sure the packager_link_test target can catch macros/classes.h being missing.

@zaki699
Copy link
Contributor Author

zaki699 commented May 30, 2024 via email

@joeyparrish
Copy link
Member

Happy to help. That check really should have caught this, and I created it, so it was the least I could do.

@zaki699
Copy link
Contributor Author

zaki699 commented May 30, 2024 via email

@joeyparrish
Copy link
Member

Sure. I'm going to the "Details" link next to the CLA check at the bottom of the PR. There, I see a list of the contributors from the git commits:

The following contributors were found for this pull request:

1ef5981 PR Opener: @zaki699
6fb1ad6 Author: <o***n​@Zakis-MacBook-Pro.local>
1ef5981 Author: @joeyparrish <joeyparrish​@users.noreply.github.com>

So it's the commit from ...​@Zakis-MacBook-Pro.local that doesn't match the CLA. You just need to run git config to set your name and email address, then amend the commit in question and force-push the PR branch.

@zaki699 zaki699 force-pushed the Fix-macros-classes-include-file branch from 1ef5981 to 042e60f Compare May 30, 2024 21:12
@zaki699
Copy link
Contributor Author

zaki699 commented May 30, 2024

Ok Now the CLA is validated.

Thx

@zaki699
Copy link
Contributor Author

zaki699 commented May 30, 2024

It seems that I have somehow managed to remove your previous commit. Do you mind pushing it again ?

Thanks you

This allows our link test to catch missing header dependencies for file.h.  An audit of all public headers was conducted, and file.h was the only one not referenced in packager.h.
@joeyparrish
Copy link
Member

Done

@joeyparrish
Copy link
Member

I will merge this if the tests pass, which I fully expect

@joeyparrish joeyparrish merged commit b5c2cb8 into shaka-project:main May 31, 2024
35 checks passed
@zaki699 zaki699 deleted the Fix-macros-classes-include-file branch May 31, 2024 05:10
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.

Macro DISALLOW_COPY_AND_ASSIGN include/file.h
3 participants