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

Handle the case where neither public-files nor private-files are present #96

Merged
merged 1 commit into from
Jan 20, 2023

Conversation

storypku
Copy link
Contributor

@storypku storypku commented Jan 19, 2023

Hi @martis42
I found some cases w/ neither public-files nor private-files. Please help review whether or not it's OK.

@storypku storypku force-pushed the no-pub-no-private branch 2 times, most recently from aff078f to af7df0d Compare January 19, 2023 13:12
Copy link
Owner

@martis42 martis42 left a comment

Choose a reason for hiding this comment

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

@storypku
Thanks for identifying and fixing this bug!
However, please reduce the examples in test/aspect/shared_library/BUILD. Using alwayslink or dlopen is not related to DWYU or the identified bug. Essentially only //test/aspect/shared_library:libfoobar.so seems relevant, which you correctly use as integration test.

@storypku
Copy link
Contributor Author

@storypku Thanks for identifying and fixing this bug! However, please reduce the examples in test/aspect/shared_library/BUILD. Using alwayslink or dlopen is not related to DWYU or the identified bug. Essentially only //test/aspect/shared_library:libfoobar.so seems relevant, which you correctly use as integration test.

Done. Is it better to keep the alwayslink attribute? Otherwise, users may wonder why libfoo.so doesn't export any useful symbol...

What's your opinion?

Copy link
Owner

@martis42 martis42 left a comment

Choose a reason for hiding this comment

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

@storypku
I don't think it is important that the example correctly exports symbols. In the end this is only showing that the tool can handle cc_binary targets without srcs attribute.

I just discovered something which I overlooked in my first review. Please remove package(default_visibility = ["//visibility:public"]). There is no reason to make these targets available outside the example.

@storypku
Copy link
Contributor Author

storypku commented Jan 20, 2023

Done. It think you are right. alwayslink=True is only a non-essential point here.

Thanks also for pointing my carelessness in the visibility settings. According to Bazel Visibility Best Practices:

Avoid setting default_visibility to public. It may be convenient for prototyping or in small codebases, but the risk of inadvertently creating public targets increases as the codebase grows. It's better to be explicit about which targets are part of a package's public interface.

@martis42 martis42 merged commit 65f74f6 into martis42:main Jan 20, 2023
@storypku storypku deleted the no-pub-no-private branch January 20, 2023 23:31
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.

None yet

2 participants