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

Header file extensions as options for C/C++ targets #6802

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Nov 22, 2018

Problem

The file extensions we consider "header files" for C/C++ targets are hardcoded as ('.h', '.hpp'). Additionally, we will error out on header-only ctypes_compatible_cpp_library() targets (as for C targets).

Solution

  • Create the --header-file-extensions option in the CCompileSettings and CppCompileSettings subsystems.
  • Make a debug-level log if the library undergoing compile is a header-only library before early returning.
  • Add some tests.

Copy link
Contributor

@CMLivingston CMLivingston left a comment

Choose a reason for hiding this comment

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

This looks good to me, and this PR is now a blocker for our internal native diff per our offline discussion.

@cosmicexplorer
Copy link
Contributor Author

this PR is now a blocker

To clarify, this is because pants will currently raise an error when there are header-only libraries -- this PR fixes that.

@cosmicexplorer cosmicexplorer merged commit ca9405c into pantsbuild:master Nov 27, 2018

object_files_product = context.products.get(ObjectFiles)
object_files_for_target = self._retrieve_single_product_at_target_base(object_files_product, c)
# Test that no object files were produced.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Danny - this is a much better test!

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Nov 27, 2018

Choose a reason for hiding this comment

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

I think so too! I swear to you that I prevaricated for at least an hour about adding tests based on matching info logs and moved forward only to unblock myself from that particular indecision. I am planning to take your comment on the other PR #6800 and make a (concise!) followup issue to fix this in the integration tests from that PR.

cosmicexplorer added a commit that referenced this pull request Nov 28, 2018
### Problem

The file extensions we consider "header files" for C/C++ targets are hardcoded as `('.h', '.hpp')`. Additionally, we will error out on header-only `ctypes_compatible_cpp_library()` targets (as for C targets).

### Solution

- Create the `--header-file-extensions` option in the CCompileSettings and CppCompileSettings subsystems.
- Make a debug-level log if the library undergoing compile is a header-only library before early returning.
- Add some tests.
cosmicexplorer added a commit that referenced this pull request Nov 29, 2018
### Problem

The file extensions we consider "header files" for C/C++ targets are hardcoded as `('.h', '.hpp')`. Additionally, we will error out on header-only `ctypes_compatible_cpp_library()` targets (as for C targets).

### Solution

- Create the `--header-file-extensions` option in the CCompileSettings and CppCompileSettings subsystems.
- Make a debug-level log if the library undergoing compile is a header-only library before early returning.
- Add some tests.
cosmicexplorer added a commit that referenced this pull request Dec 1, 2018
### Problem

The file extensions we consider "header files" for C/C++ targets are hardcoded as `('.h', '.hpp')`. Additionally, we will error out on header-only `ctypes_compatible_cpp_library()` targets (as for C targets).

### Solution

- Create the `--header-file-extensions` option in the CCompileSettings and CppCompileSettings subsystems.
- Make a debug-level log if the library undergoing compile is a header-only library before early returning.
- Add some tests.
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