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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -37,32 +37,44 @@ def get_compiler_option_sets_enabled_default_value(cls):
return {"fatal_warnings": ["-Werror"]}


class CCompileSettings(Subsystem):
options_scope = 'c-compile-settings'
class CompileSettingsBase(Subsystem):

@classmethod
def subsystem_dependencies(cls):
return super(CCompileSettings, cls).subsystem_dependencies() + (
return super(CompileSettingsBase, cls).subsystem_dependencies() + (
NativeBuildStepSettings.scoped(cls),
)

@classproperty
def header_file_extensions_default(cls):
raise NotImplementedError('header_file_extensions_default() must be overridden!')

@classmethod
def register_options(cls, register):
super(CompileSettingsBase, cls).register_options(register)
register('--header-file-extensions', advanced=True, default=cls.header_file_extensions_default,
type=list, fingerprint=True,
help="The file extensions which should not be provided to the compiler command line.")

@memoized_property
def native_build_step_settings(self):
return NativeBuildStepSettings.scoped_instance(self)

@memoized_property
def header_file_extensions(self):
return self.get_options().header_file_extensions

class CppCompileSettings(Subsystem):
options_scope = 'cpp-compile-settings'

@classmethod
def subsystem_dependencies(cls):
return super(CppCompileSettings, cls).subsystem_dependencies() + (
NativeBuildStepSettings.scoped(cls),
)
class CCompileSettings(CompileSettingsBase):
options_scope = 'c-compile-settings'

@memoized_property
def native_build_step_settings(self):
return NativeBuildStepSettings.scoped_instance(self)
header_file_extensions_default = ['.h']


class CppCompileSettings(CompileSettingsBase):
options_scope = 'cpp-compile-settings'

header_file_extensions_default = ['.h', '.hpp', '.hxx', '.tpp']


# TODO: add a fatal_warnings kwarg to NativeArtifact and make a LinkSharedLibrariesSettings subclass
Expand Down
31 changes: 20 additions & 11 deletions src/python/pants/backend/native/tasks/native_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ class NativeCompileRequest(datatype([
'sources',
'compiler_options',
'output_dir',
'header_file_extensions',
])): pass


Expand All @@ -47,8 +48,6 @@ class NativeCompile(NativeTask, AbstractClass):
source_target_constraint = None
dependent_target_constraint = SubclassesOf(ExternalNativeLibrary, NativeLibrary)

HEADER_EXTENSIONS = ('.h', '.hpp')

# `NativeCompile` will use `workunit_label` as the name of the workunit when executing the
# compiler process. `workunit_label` must be set to a string.
@classproperty
Expand Down Expand Up @@ -181,10 +180,23 @@ def _make_compile_request(self, versioned_target, dependencies, external_libs_pr
compiler_options=(self._compile_settings
.native_build_step_settings
.get_merged_args_for_compiler_option_sets(compiler_option_sets)),
output_dir=versioned_target.results_dir)
output_dir=versioned_target.results_dir,
header_file_extensions=self._compile_settings.header_file_extensions)

def _iter_sources_minus_headers(self, compile_request):
for s in compile_request.sources:
if not s.endswith(tuple(compile_request.header_file_extensions)):
yield s

class _HeaderOnlyLibrary(Exception): pass

def _make_compile_argv(self, compile_request):
"""Return a list of arguments to use to compile sources. Subclasses can override and append."""

sources_minus_headers = list(self._iter_sources_minus_headers(compile_request))
if len(sources_minus_headers) == 0:
raise self._HeaderOnlyLibrary()

compiler = compile_request.compiler
compiler_options = compile_request.compiler_options
# We are going to execute in the target output, so get absolute paths for everything.
Expand All @@ -199,7 +211,7 @@ def _make_compile_argv(self, compile_request):
'-I{}'.format(os.path.join(buildroot, inc_dir))
for inc_dir in compile_request.include_dirs
] +
[os.path.join(buildroot, src) for src in compile_request.sources])
[os.path.join(buildroot, src) for src in sources_minus_headers])

self.context.log.debug("compile argv: {}".format(argv))

Expand All @@ -211,18 +223,15 @@ def _compile(self, compile_request):
NB: This method must arrange the output files so that `collect_cached_objects()` can collect all
of the results (or vice versa)!
"""
sources = compile_request.sources

if len(sources) == 0 and not any(s for s in sources if not s.endswith(self.HEADER_EXTENSIONS)):
# TODO: do we need this log message? Should we still have it for intentionally header-only
# libraries (that might be a confusing message to see)?
self.context.log.debug("no sources in request {}, skipping".format(compile_request))
cosmicexplorer marked this conversation as resolved.
Show resolved Hide resolved
try:
argv = self._make_compile_argv(compile_request)
except self._HeaderOnlyLibrary:
self.context.log.debug('{} is a header-only library'.format(compile_request))
return

compiler = compile_request.compiler
output_dir = compile_request.output_dir

argv = self._make_compile_argv(compile_request)
env = compiler.as_invocation_environment_dict

with self.context.new_workunit(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,17 @@ def rules(cls):


class NativeCompileTestMixin(object):

def _retrieve_single_product_at_target_base(self, product_mapping, target):
product = product_mapping.get(target)
base_dirs = list(product.keys())
self.assertEqual(1, len(base_dirs))
single_base_dir = base_dirs[0]
all_products = product[single_base_dir]
self.assertEqual(1, len(all_products))
single_product = all_products[0]
return single_product

def create_simple_cpp_library(self, **kwargs):
self.create_file('src/cpp/test/test.hpp', contents=dedent("""
#ifndef __TEST_HPP__
Expand Down
35 changes: 35 additions & 0 deletions tests/python/pants_test/backend/native/tasks/test_c_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from pants.backend.native.targets.native_library import CLibrary
from pants.backend.native.tasks.c_compile import CCompile
from pants.backend.native.tasks.native_compile import ObjectFiles
from pants_test.backend.native.tasks.native_task_test_base import (NativeCompileTestMixin,
NativeTaskTestBase)

Expand All @@ -17,6 +18,21 @@ class CCompileTest(NativeTaskTestBase, NativeCompileTestMixin):
def task_type(cls):
return CCompile

def create_header_only_alternate_c_library(self, ext, **kwargs):
header_filename = 'test{}'.format(ext)
self.create_file('src/c/test/{}'.format(header_filename), contents=dedent("""
#ifndef __TEST_H__
#define __TEST_H__

int test(int);

#endif
"""))
return self.make_target(spec='src/c/test',
target_type=CLibrary,
sources=[header_filename],
**kwargs)

def create_simple_c_library(self, **kwargs):
self.create_file('src/c/test/test.h', contents=dedent("""
#ifndef __TEST_H__
Expand All @@ -38,10 +54,29 @@ def create_simple_c_library(self, **kwargs):
sources=['test.h', 'test.c'],
**kwargs)

def test_header_only_noop_with_alternate_header_extension(self):
alternate_extension = '.asdf'
c = self.create_header_only_alternate_c_library(alternate_extension)
context = self.prepare_context_for_compile(target_roots=[c], options={
'c-compile-settings': {
'header_file_extensions': [alternate_extension],
},
})

# Test that the task runs without error if provided a header-only library.
c_compile = self.create_task(context)
c_compile.execute()

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.

self.assertEqual(0, len(object_files_for_target.filenames))

def test_caching(self):
c = self.create_simple_c_library()
context = self.prepare_context_for_compile(target_roots=[c])
c_compile = self.create_task(context)

# TODO: what is this testing?
c_compile.execute()
c_compile.execute()
38 changes: 38 additions & 0 deletions tests/python/pants_test/backend/native/tasks/test_cpp_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@

from __future__ import absolute_import, division, print_function, unicode_literals

from textwrap import dedent

from pants.backend.native.targets.native_library import CppLibrary
from pants.backend.native.tasks.cpp_compile import CppCompile
from pants.backend.native.tasks.native_compile import ObjectFiles
from pants_test.backend.native.tasks.native_task_test_base import (NativeCompileTestMixin,
NativeTaskTestBase)

Expand All @@ -14,10 +18,44 @@ class CppCompileTest(NativeTaskTestBase, NativeCompileTestMixin):
def task_type(cls):
return CppCompile

def create_simple_header_only_library(self, **kwargs):
# TODO: determine if there are other features that people expect from header-only libraries that
# we could be testing here. Currently this file's contents are just C++ which doesn't define any
# non-template classes or methods.
self.create_file('src/cpp/test/test.hpp', contents=dedent("""
#ifndef __TEST_HPP__
#define __TEST_HPP__

template <typename T>
T add(T a, T b) {
return a + b;
}

#endif
"""))
return self.make_target(spec='src/cpp/test',
target_type=CppLibrary,
sources=['test.hpp'],
**kwargs)

def test_header_only_target_noop(self):
cpp = self.create_simple_header_only_library()
context = self.prepare_context_for_compile(target_roots=[cpp])

# Test that the task runs without error if provided a header-only library.
cpp_compile = self.create_task(context)
cpp_compile.execute()

object_files_product = context.products.get(ObjectFiles)
object_files_for_target = self._retrieve_single_product_at_target_base(object_files_product, cpp)
# Test that no object files were produced.
self.assertEqual(0, len(object_files_for_target.filenames))

def test_caching(self):
cpp = self.create_simple_cpp_library()
context = self.prepare_context_for_compile(target_roots=[cpp])
cpp_compile = self.create_task(context)

# TODO: what is this testing?
cpp_compile.execute()
cpp_compile.execute()