Skip to content
Closed
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
20 changes: 20 additions & 0 deletions src/python/pants/backend/native/subsystems/binaries/cmake.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# coding=utf-8
# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import absolute_import, division, print_function, unicode_literals

import os

from pants.binaries.binary_tool import NativeTool
from pants.util.memo import memoized_property


class CMake(NativeTool):
options_scope = 'cmake'
default_version = '3.9.5'
archive_type = 'tgz'

@memoized_property
def bin_dir(self):
return os.path.join(self.select(), 'bin')
20 changes: 20 additions & 0 deletions src/python/pants/backend/native/subsystems/binaries/make.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# coding=utf-8
# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

from __future__ import absolute_import, division, print_function, unicode_literals

import os

from pants.binaries.binary_tool import NativeTool
from pants.util.memo import memoized_property


class Make(NativeTool):
options_scope = 'make'
default_version = '4.2.1'
archive_type = 'tgz'

@memoized_property
def bin_dir(self):
return os.path.join(self.select(), 'bin')
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@


class NativeBuildStepSettingsBase(Subsystem, MirroredTargetOptionMixin):
"""Holds options which are understood by all parts of the compile and link pipeline.

NB: This is separate from NativeBuildSettings, which is used to store options controlling pants
behavior. Rather, this subsystem stores options which are typically interpreted into command-line
arguments or environment variables for a compiler or linker invocation.
"""

mirrored_option_to_kwarg_map = {
'fatal_warnings': 'fatal_warnings',
Expand Down
38 changes: 31 additions & 7 deletions src/python/pants/backend/native/tasks/link_shared_libraries.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,26 @@ class LinkSharedLibraryRequest(datatype([
'output_dir',
('external_lib_dirs', tuple),
('external_lib_names', tuple),
('external_static_archive_paths', tuple),
])):
pass

@classmethod
def with_external_libs_product(cls, external_libs_product=None, *args, **kwargs):
if external_libs_product is None:
lib_dirs = ()
lib_names = ()
static_archive_paths = ()
else:
lib_dirs = (external_libs_product.lib_dir,)
lib_names = external_libs_product.lib_names
static_archive_paths = external_libs_product.static_archive_paths

return cls(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will conflict with the change I was working on. Are you ok with waiting until that is in, and then rebasing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes!

*args,
external_lib_dirs=lib_dirs,
external_lib_names=lib_names,
external_static_archive_paths=static_archive_paths,
**kwargs)


class LinkSharedLibraries(NativeTask):
Expand Down Expand Up @@ -135,19 +153,22 @@ def _make_link_request(self, vt, compiled_objects_product, external_libs_product

external_lib_dirs = []
external_lib_names = []
external_static_archive_paths = []
if external_libs_product is not None:
for nelf in external_libs_product.get_for_targets(deps):
if nelf.lib_dir:
external_lib_dirs.append(nelf.lib_dir)
external_lib_names.extend(nelf.lib_names)
external_static_archive_paths.extend(nelf.static_archive_paths)

return LinkSharedLibraryRequest(
linker=self.linker,
object_files=tuple(all_compiled_object_files),
native_artifact=vt.target.ctypes_native_library,
output_dir=vt.results_dir,
external_lib_dirs=tuple(external_lib_dirs),
external_lib_names=tuple(external_lib_names))
external_lib_names=tuple(external_lib_names),
external_static_archive_paths=tuple(external_static_archive_paths))

_SHARED_CMDLINE_ARGS = {
'darwin': lambda: ['-Wl,-dylib'],
Expand All @@ -169,12 +190,15 @@ def _execute_link_request(self, link_request):
self.context.log.debug("resulting_shared_lib_path: {}".format(resulting_shared_lib_path))
# We are executing in the results_dir, so get absolute paths for everything.
cmd = ([linker.exe_filename] +
self.platform.resolve_platform_specific(self._SHARED_CMDLINE_ARGS) +
linker.extra_args +
['-o', os.path.abspath(resulting_shared_lib_path)] +
['-L{}'.format(lib_dir) for lib_dir in link_request.external_lib_dirs] +
['-l{}'.format(lib_name) for lib_name in link_request.external_lib_names] +
[os.path.abspath(obj) for obj in object_files])
[os.path.abspath(obj) for obj in object_files] +
# TODO: static archives should be resolvable with -L/-l too, but that's not working for
# some reason with the IrrXML package in testprojects/.
list(link_request.external_static_archive_paths) +
['-L{}'.format(d) for d in link_request.external_lib_dirs] +
['-l{}'.format(l) for l in link_request.external_lib_names] +
self.platform.resolve_platform_specific(self._SHARED_CMDLINE_ARGS) +
['-o', os.path.abspath(resulting_shared_lib_path)])
self.context.log.debug("linker command: {}".format(cmd))

env = linker.as_invocation_environment_dict
Expand Down
8 changes: 0 additions & 8 deletions src/python/pants/backend/native/tasks/native_compile.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,6 @@
from collections import defaultdict

from pants.backend.native.config.environment import Executable
from pants.backend.native.targets.external_native_library import ExternalNativeLibrary
from pants.backend.native.targets.native_library import NativeLibrary
from pants.backend.native.tasks.native_external_library_fetch import NativeExternalLibraryFiles
from pants.backend.native.tasks.native_task import NativeTask
from pants.base.build_environment import get_buildroot
Expand Down Expand Up @@ -40,12 +38,6 @@ def file_paths(self):


class NativeCompile(NativeTask, AbstractClass):
# `NativeCompile` will use the `source_target_constraint` to determine what targets have "sources"
# to compile, and the `dependent_target_constraint` to determine which dependent targets to
# operate on for `strict_deps` calculation.
# NB: `source_target_constraint` must be overridden.
source_target_constraint = None
dependent_target_constraint = SubclassesOf(ExternalNativeLibrary, NativeLibrary)

# `NativeCompile` will use `workunit_label` as the name of the workunit when executing the
# compiler process. `workunit_label` must be set to a string.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,22 +6,27 @@

import os
import re
from builtins import str
from distutils.dir_util import copy_tree

from pex.interpreter import PythonInterpreter

from pants.backend.native.config.environment import Platform
from pants.backend.native.config.environment import LLVMCppToolchain, Platform
from pants.backend.native.subsystems.binaries.cmake import CMake
from pants.backend.native.subsystems.binaries.make import Make
from pants.backend.native.subsystems.conan import Conan
from pants.backend.native.subsystems.native_toolchain import NativeToolchain
from pants.backend.native.targets.external_native_library import ExternalNativeLibrary
from pants.backend.native.tasks.native_task import NativeTask
from pants.base.build_environment import get_pants_cachedir
from pants.base.exceptions import TaskError
from pants.goal.products import UnionProducts
from pants.invalidation.cache_manager import VersionedTargetSet
from pants.task.task import Task
from pants.util.contextutil import environment_as
from pants.util.memo import memoized_property
from pants.util.objects import Exactly, datatype
from pants.util.process_handler import subprocess
from pants.util.strutil import create_path_env_var


class ConanRequirement(datatype(['pkg_spec'])):
Expand Down Expand Up @@ -59,7 +64,12 @@ def directory_path(self):
def fetch_cmdline_args(self):
platform = Platform.create()
conan_os_name = platform.resolve_platform_specific(self.CONAN_OS_NAME)
args = ['install', self.pkg_spec, '-s', 'os={}'.format(conan_os_name)]
# TODO: --build missing should be an option!
args = [
'install', self.pkg_spec,
'-s', 'os={}'.format(conan_os_name),
'--build', 'missing',
]
return args


Expand All @@ -68,25 +78,23 @@ class NativeExternalLibraryFiles(datatype([
# TODO: we shouldn't have any `lib_names` if `lib_dir` is not set!
'lib_dir',
('lib_names', tuple),
('static_archive_paths', tuple),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any particular reason things don't just work? IIRC, simply passing -l<libname> will trigger a search of supplied search dirs, right? Passing the direct path to the static archive is better than two separate options for the search dir and the library name, but since we are already doing that, why add more complexity?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It didn't work when I tried it! I don't know why, or if this is accepted in some places but not others. I'm pretty sure this is a valid command line invocation regardless, but I definitely tried -L/-l and it didn't work until I did this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

"didn't work" = "couldn't find the symbols in the archives during linking"

@cosmicexplorer cosmicexplorer Sep 11, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I 100% agree that it is more complexity, and it would probably be great to leave a TODO here to explain why static vs shared libs need to be treated differently and how that works for different linkers (if it differs at all).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah ok so it's supposed to work with -L/-l, as you describe, but it may be that clang doesn't understand that. The option -static is supposed to use static versions of libraries whenever possible, but all my attempts to make that work (which didn't use even more complex hacks) either errored at link time or at runtime when the ctypes library is loaded. This should be in a comment in the code.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

IrrXML only provides a static lib, no dynamic version, or I'd suspect the linker just got confused.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying, sounds good.

])): pass


class NativeExternalLibraryFetch(Task):
options_scope = 'native-external-library-fetch'
class NativeExternalLibraryFetch(NativeTask):
native_library_constraint = Exactly(ExternalNativeLibrary)

conan_pex_subdir = 'conan-support'
conan_pex_filename = 'conan.pex'

class NativeExternalLibraryFetchError(TaskError):
pass

@classmethod
def _parse_lib_name_from_library_filename(cls, filename):
match_group = re.match(r"^lib(.*)\.(a|so|dylib)$", filename)
if match_group:
return match_group.group(1)
return None
def subsystem_dependencies(cls):
return super(NativeExternalLibraryFetch, cls).subsystem_dependencies() + (
CMake.scoped(cls),
Conan.scoped(cls),
Make.scoped(cls),
NativeToolchain.scoped(cls),
)

@classmethod
def register_options(cls, register):
Expand All @@ -98,10 +106,6 @@ def register_options(cls, register):
def implementation_version(cls):
return super(NativeExternalLibraryFetch, cls).implementation_version() + [('NativeExternalLibraryFetch', 0)]

@classmethod
def subsystem_dependencies(cls):
return super(NativeExternalLibraryFetch, cls).subsystem_dependencies() + (Conan.scoped(cls),)

@classmethod
def product_types(cls):
return [NativeExternalLibraryFiles]
Expand All @@ -114,10 +118,44 @@ def create_target_dirs(self):
return True

@memoized_property
def _conan_python_interpreter(self):
return PythonInterpreter.get()
def _native_toolchain(self):
return NativeToolchain.scoped_instance(self)

@memoized_property
def _cpp_toolchain(self):
return self._request_single(LLVMCppToolchain, self._native_toolchain).cpp_toolchain

@memoized_property
def _cmake(self):
return CMake.scoped_instance(self)

@memoized_property
def _make(self):
return Make.scoped_instance(self)

@memoized_property
def _build_environment(self):
cpp_compiler = self._cpp_toolchain.cpp_compiler
cpp_linker = self._cpp_toolchain.cpp_linker
# Compose the invocation environments.
invocation_env_dict = cpp_compiler.as_invocation_environment_dict.copy()
invocation_env_dict.update(cpp_linker.as_invocation_environment_dict)
invocation_env_dict['PATH'] = create_path_env_var((
cpp_compiler.path_entries +
cpp_linker.path_entries +
[self._cmake.bin_dir, self._make.bin_dir]))
return invocation_env_dict

class NativeExternalLibraryFetchError(TaskError):
pass

@classmethod
def _parse_lib_name_from_library_filename(cls, filename):
match_group = re.match(r"^lib(.*)\.(a|so|dylib)$", filename)
if match_group:
return match_group.group(1)
return None

def _conan_pex_path(self):
return os.path.join(get_pants_cachedir(),
self.conan_pex_subdir,
Expand All @@ -126,8 +164,8 @@ def _conan_pex_path(self):
@memoized_property
def _conan_binary(self):
return Conan.scoped_instance(self).bootstrap(
self._conan_python_interpreter,
self._conan_pex_path)
PythonInterpreter.get(),
self._conan_pex_path())

def execute(self):
native_lib_tgts = self.context.targets(self.native_library_constraint.satisfied_by)
Expand All @@ -153,15 +191,21 @@ def _collect_external_libs(self, vts):
include_dir = os.path.join(vt.results_dir, 'include')

lib_names = []
static_archive_paths = []
if os.path.isdir(lib_dir):
for filename in os.listdir(lib_dir):
lib_name = self._parse_lib_name_from_library_filename(filename)
if lib_name:
lib_names.append(lib_name)
if filename.endswith('.a'):
archive_path = os.path.join(lib_dir, filename)
static_archive_paths.append(archive_path)
else:
shared_lib_name = self._parse_lib_name_from_library_filename(filename)
if shared_lib_name:
lib_names.append(shared_lib_name)

nelf = NativeExternalLibraryFiles(include_dir=include_dir,
lib_dir=lib_dir,
lib_names=tuple(lib_names))
lib_names=tuple(lib_names),
static_archive_paths=tuple(static_archive_paths))
product.add_for_target(vt.target, [nelf])
return product

Expand Down Expand Up @@ -253,7 +297,7 @@ def _fetch_packages(self, vt):
# iteration of this system (a 'clean-all' will nuke the conan dir). In the future,
# it would be good to migrate this under ~/.cache/pants/conan for persistence.
# Fix this per: https://github.com/pantsbuild/pants/issues/6169
with environment_as(CONAN_USER_HOME=self.workdir):
with environment_as(CONAN_USER_HOME=self.workdir, **self._build_environment):
for pkg_spec in vt.target.packages:

conan_requirement = ConanRequirement(pkg_spec=pkg_spec)
Expand Down
3 changes: 2 additions & 1 deletion src/python/pants/backend/native/tasks/native_task.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

from pants.backend.native.subsystems.native_build_settings import NativeBuildSettings
from pants.backend.native.subsystems.native_toolchain import NativeToolchain
from pants.backend.native.targets.external_native_library import ExternalNativeLibrary
from pants.backend.native.targets.native_library import NativeLibrary
from pants.build_graph.dependency_context import DependencyContext
from pants.task.task import Task
Expand Down Expand Up @@ -39,7 +40,7 @@ def dependent_target_constraint(cls):

:return: :class:`pants.util.objects.TypeConstraint`
"""
return SubclassesOf(NativeLibrary)
return SubclassesOf(ExternalNativeLibrary, NativeLibrary)

@classmethod
def subsystem_dependencies(cls):
Expand Down
14 changes: 1 addition & 13 deletions src/python/pants/backend/python/subsystems/python_native_code.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,8 @@

from pex.pex import PEX

from pants.backend.native.config.environment import CppToolchain, CToolchain, Platform
from pants.backend.native.config.environment import CppToolchain, CToolchain
from pants.backend.native.subsystems.native_toolchain import NativeToolchain
from pants.backend.native.subsystems.xcode_cli_tools import MIN_OSX_VERSION_ARG
from pants.backend.native.targets.native_library import NativeLibrary
from pants.backend.python.python_requirement import PythonRequirement
from pants.backend.python.subsystems.python_setup import PythonSetup
Expand Down Expand Up @@ -159,7 +158,6 @@ def base_requirements(self):
class SetupPyNativeTools(datatype([
('c_toolchain', CToolchain),
('cpp_toolchain', CppToolchain),
('platform', Platform),
])):
"""The native tools needed for a setup.py invocation.

Expand All @@ -175,16 +173,6 @@ class SetupPyExecutionEnvironment(datatype([
'setup_py_native_tools',
])):

_SHARED_CMDLINE_ARGS = {
'darwin': lambda: [
MIN_OSX_VERSION_ARG,
'-Wl,-dylib',
'-undefined',
'dynamic_lookup',
],
'linux': lambda: ['-shared'],
}

def as_environment(self):
ret = {}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,7 @@ def _prepare_and_create_dist(self, interpreter, shared_libs_product, versioned_t
# TODO: test this branch somehow!
native_tools = SetupPyNativeTools(
c_toolchain=self._c_toolchain,
cpp_toolchain=self._cpp_toolchain,
platform=self._platform)
cpp_toolchain=self._cpp_toolchain)
# Native code in this python_dist() target requires marking the dist as platform-specific.
is_platform_specific = True
elif len(all_native_artifacts) > 0:
Expand Down
Loading