Skip to content

[tests] Include private headers using relative paths #2287

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

Merged
merged 1 commit into from
Jan 22, 2019

Conversation

bertmaher
Copy link
Contributor

Description: While it's best for tests to only use the "public" headers of a library, it sometimes makes sense to use the private/internal headers directly. Instead of adding to the include path via cmake, let's refer to them using relative paths like ../../lib/Foo/Bar.h. This seems to be LLVM's approach, and I see two advantages:

  1. It makes clear you're testing internal implementation, and
  2. It plays nicely with FB's internal header layouts O:-)

Testing: ninja all

Documentation: n/a

@nadavrot
Copy link
Contributor

This seems to be LLVM's approach, and I see two advantages:

@bertmaher Where is LLVM using the relative path include? I could not find this patten using grep.

@bertmaher
Copy link
Contributor Author

A handful of unittests appear to use it, e.g. https://github.com/llvm-mirror/llvm/blob/master/unittests/CodeGen/DIEHashTest.cpp#L9. It's not very common.

Fwiw, I don't exactly love this approach. An alternative is moving these headers to include/glow (kind of awkward for the backend-specific headers). Or I could not do this, and instead add some hacks to the fb internal build system to look for headers in "unexpected" places. Or maybe there's something else I haven't thought of. This change just seemed like the least-bad approach.

Copy link
Contributor

@nadavrot nadavrot left a comment

Choose a reason for hiding this comment

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

@bertmaher Ah, okay, now I understand. Thank you.

@bertmaher bertmaher merged commit be5a4e4 into pytorch:master Jan 22, 2019
@bertmaher bertmaher deleted the rinc branch January 22, 2019 04:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants