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

Conan (third party) support for ctypes native libraries #5998

Merged
merged 48 commits into from Jul 18, 2018

Conversation

Projects
None yet
5 participants
@CMLivingston
Copy link
Contributor

CMLivingston commented Jun 21, 2018

Problem

The new targets introduced in #5815 have no means of declaring, fetching, and using third party native dependencies.

Solution

Integrate the Conan package manager to fetch packages from a remote package store via a new task, copy the package data to the results directory of third_party_native_library targets in play, and create a product that the compile and link tasks can consume. Plumb the directory paths provided by the product through to the command lines of the compile and link steps.

Result

Users can now depend on third party native libraries in their ctypes_compatible_cpp_library targets from either conan-center or a remote URI that they specify via an option.

Some notes

  • The conan home directory is currently under .pants.d, instead of ~ or ~/.cache/pants. I did this for the short term to make debugging cache problems and other issues as simple as a ./pants clean-all. I don't think the perf loss will be too bad (1-2 seconds).
  • Some string manipulation could be better done as regex, and I have TODOs for that
  • I am going to follow up with upstream Conan about getting a flag for cleaner client output so the parse method does not need to be so ugly.
reimplement a previous PR -- ignore this
This commit is a reimplementation of registering @rules for backends, because this PR began before
that one was split off.

add some simple examples to demonstrate how to use backend rules

...actually make the changes to consume backend rules in register.py

revert accidental change to a test target

remove extraneous log statement

fix lint errors

add native backend to release.sh

isolate native toolchain path and hope for the best

add config subdir to native backend

really just some more attempts

start trying to dispatch based on platform

extend EngineInitializer to add more rules from a backend

refactor Platform to use new methods in osutil.py

refactor the native backend to be a real backend and expose rules

register a rule in the python backend to get a setup.py environment

make python_dist() tests pass

make lint pass
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Jun 22, 2018

@CMLivingston : Thanks!

Before we begin reviewing this, would you mind squashing or rebasing it down to 1-5 commits (whatever looks reasonable rebase wise)?

@CMLivingston CMLivingston force-pushed the CMLivingston:clivingston/ctypes-test-project-third-party branch 2 times, most recently from 163063d to 50d12ba Jun 22, 2018

@CMLivingston CMLivingston force-pushed the CMLivingston:clivingston/ctypes-test-project-third-party branch from 50d12ba to 91641b7 Jun 22, 2018

Chris Livingston
@CMLivingston

This comment has been minimized.

Copy link
Contributor

CMLivingston commented Jun 22, 2018

Done @stuhood

Chris Livingston

@stuhood stuhood requested review from stuhood , benjyw and cosmicexplorer Jun 22, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@@ -35,6 +38,7 @@ def build_file_aliases():
def register_goals():
# FIXME(#5962): register these under the 'compile' goal when we eliminate the product transitive
# dependency from export -> compile.
task(name='native-third-party-prep', action=NativeThirdPartyPrep).install('native-compile')

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

In general, avoid increasing the size of existing goals... bias toward creating new goals for things that folks are not intended to run directly from the CLI.

This disagrees with some previous guidance (and it will temporarily mean a distracting number of goals listed in ./pants goals... unless we ended up with a facility for hiding them there?).

But grouping tasks together into goals (that users are not directly running) creates unnecessary dependencies between them. In this case: you can't run native-third-party-prep without also running shared-libraries.

This comment has been minimized.

@CMLivingston

CMLivingston Jun 22, 2018

Contributor

I am confused here. It is piggybacking off the new native-compile goal created by Danny, right? And you would never run native-third-party-prep without running the compile and shared library tasks.

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

Unless a goal is meant to run from the commandline, goal names (and goals themselves, really) aren't useful. The reason to collect tasks together into a goal is because you want a human to be able to use that goal name on the CLI and force all tasks in the goal to run. Maybe that's the intent with native-compile, but most likely what you'd want them to run is ./pants compile, which will have a dependency on native-compile, and transitively on dep resolution and etc.

log = logging.getLogger(__name__)


class ConanPrep(Subsystem):

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

IMO, drop Prep from the name, and from the options scope.

This comment has been minimized.

@CMLivingston

def conan_requirements(self):
return (
'conan==1.4.4',

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

Would recommend putting all of these into a --requirements list option on your Subsystem, so that someone can override the version if they'd like by doing something like --conan-requirements='["conan==1.5.0", ..]'.

This comment has been minimized.

@CMLivingston
with safe_concurrent_creation(conan_pex_path) as safe_path:
builder = PEXBuilder(safe_path, pex_info=pex_info)
reqs = [PythonRequirement(req) for req in self.conan_requirements()]
interpreter = builder.interpreter

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

Is there a good reason to use a different interpreter here? If possible it would be good to minimize the differences between these two paths, so that you don't have a difference between the python used while we're bootstrapping vs the rest of the time.

This comment has been minimized.

@CMLivingston

CMLivingston Jun 22, 2018

Contributor

So you are saying introduce a dependency on pyprep/interpreter selection?

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

No: in the other branch of this if/else, you use interpreter = PythonInterpreter.get().

This comment has been minimized.

@CMLivingston

CMLivingston Jun 22, 2018

Contributor

Ah, that is what PEXBuilder is using under the hood. Will update to make more explicit.

def _get_third_party_lib_args(self):
lib_args = []
tp_lib_tgts = self.context.targets(NativeThirdPartyPrep.native_library_constraint.satisfied_by)
if tp_lib_tgts:

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

Rather than making this conditional, you should probably update the task that produces the product to always produce the product (even if there are no targets).

This comment has been minimized.

@CMLivingston
if os.path.exists(include):
task_product['include'] = include

self.context.products.register_data(self.ThirdPartyLibraryFiles, task_product)

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

This usage of products confuses me... as far as I can tell, this method will fail if it is called for more than one target. But my understanding of the products API is somewhat limited to the JVM usecase, where we pass around an instance of an object, and mutate that object to add additional files.

IMO, making ThirdPartyLibraryFiles an actual class with an "add_include" method, etc, would be clearer. Then the only products method you ever used would be get_data.

3rdparty_files = self.context.products.get_data(self.ThirdPartyLibraryFiles, self.ThirdPartyLibraryFiles)
3rdparty_files.add_include(..)

IE, get the ThirdPartyLibraryFiles product, and initialize it with a constructor if it doesn't exist yet. Then, given the instance, add some files to it.

This comment has been minimized.

@CMLivingston
def ensure_conan_remote_configuration(self, conan_binary):
"""
Ensure that the conan registry.txt file is sanitized and loaded with
a pants-specifc remote for package fetching.

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

specific

This comment has been minimized.

@CMLivingston
Ensure that the conan registry.txt file is sanitized and loaded with
a pants-specifc remote for package fetching.
:param conan_binary: The conan client pex to use for manupulating registry.txt.

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

manipulating

This comment has been minimized.

@CMLivingston

def ensure_conan_remote_configuration(self, conan_binary):
"""
Ensure that the conan registry.txt file is sanitized and loaded with

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

There is not much information on this file... is it supposed to be configured and committed in the repo?

If it is supposed to be committed in the repo, its locally should be passed in as config. If it's not supposed to be committed, then you should explicitly make sure it is located under the workdir (...and possibly also the vt.results_dir, if it's resolve-specific).

https://docs.conan.io/en/latest/reference/config_files/registry.txt.html doesn't make it very clear.

This comment has been minimized.

@CMLivingston

CMLivingston Jun 22, 2018

Contributor

It is not intended to be committed, not resolve-specific, and is currently under the self.workdir of the task. This file registers remote storage locations and will be the same for every resolve, very similar to how we configure python-repos.


def execute(self):
native_lib_tgts = self.context.targets(self.native_library_constraint.satisfied_by)
with self.invalidated(native_lib_tgts,

This comment has been minimized.

@stuhood

stuhood Jun 22, 2018

Member

@wisechengyi: Can you work with Chris to talk through the cache key strategy here? It seems likely that this will need to do something similar to coursier.

This comment has been minimized.

@CMLivingston

@CMLivingston CMLivingston force-pushed the CMLivingston:clivingston/ctypes-test-project-third-party branch from eff38de to cb79337 Jun 22, 2018

@benjyw
Copy link
Contributor

benjyw left a comment

This is super-awesome!

I have one naming nit: I'm not in love with the "third party" verbiage. True, we tend to use it in repos, but we don't really use it in pants itself. I'd prefer more technical terminology like "external", which is neutral regarding ownership of the code. E.g., if I use code I authored and published to conan, then it's not third-party in the legal/organizational sense, but it is still external in how it was fetched.

Apart from that, see some inline comments.

def implementation_version(cls):
return super(Conan, cls).implementation_version() + [('Conan', 1)]

def bootstrap_conan(self):

This comment has been minimized.

@benjyw

benjyw Jun 23, 2018

Contributor

This is neat - one day we should generalize it in a manner similar to JVMTools.

This comment has been minimized.

@CMLivingston

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

Agreed, this is really neat!

remote_url,
'--insert'])
try:
stdout = subprocess.check_output(add_pants_conan_remote_cmdline.split())

This comment has been minimized.

@benjyw

benjyw Jun 23, 2018

Contributor

What's the split() for? Doesn't Pex.cmdline return a list already?

This comment has been minimized.

@CMLivingston

CMLivingston Jun 25, 2018

Contributor

That will return a string.


def copy_package_contents_from_conan_dir(self, results_dir, conan_requirement, pkg_sha):
"""
Copy the contents of the fetched pacakge into the results directory of the versioned

This comment has been minimized.

@benjyw

benjyw Jun 23, 2018

Contributor

Typo: s/pacakge/package/

Can we symlink instead of copying? That's what we do for ivy-resolved jars (iirc they are symlinks from the .ivy2 cache). Or possibly that's the wrong thing to do? Either way, we should be consistent?

This comment has been minimized.

@CMLivingston

CMLivingston Jun 25, 2018

Contributor

It would be consistent with Coursier and Ivy, so I will look into supporting that.

This comment has been minimized.

@CMLivingston

CMLivingston Jun 25, 2018

Contributor

After further investigation, this may not be feasible for the current iteration and would probably be better for a follow up ticket.

The reason is that the fetch task aggregate the contents of subdirectories of each conan package into two "product" folders: include/ and lib/ under the results dir of the versioned target set. AFAICT, this would require symlinking recursively to every file in the dirs of each conan package from the data dir, and relying on the symlinking command to de-dupe. Based on my attempts to do this, I could not come up with a simple and effective solution. I am happy to hear suggestions though, but it may make sense to punt on this to unblock in the short term.

# Invoke conan to pull package from remote.
try:
process = subprocess.Popen(
cmdline.split(),

This comment has been minimized.

@benjyw

benjyw Jun 23, 2018

Contributor

Same q here re split().

This comment has been minimized.

@CMLivingston

CMLivingston Jun 25, 2018

Contributor

Pex cmdline returns a string.

Chris Livingston added some commits Jun 25, 2018

Chris Livingston
Refactor task product into its own class, allow multiple conan remote…
…s by using a list with the option, always product a task product even if it is empty
Chris Livingston
@wisechengyi
Copy link
Contributor

wisechengyi left a comment

Thanks Chris! Please see comments

cwd=vts_results_dir,
stdout=subprocess.PIPE
)
except OSError as e:

This comment has been minimized.

@wisechengyi

wisechengyi Jun 26, 2018

Contributor

Any particular reason subprocess.Popen is used here instead of subprocess.check_output like other places?

@@ -93,4 +93,3 @@ def iter_target_addresses_for_sources(self, sources):
if (LegacyAddressMapper.any_is_declaring_file(legacy_address, sources_set) or
self._owns_any_source(sources_set, hydrated_target)):
yield legacy_address

This comment has been minimized.

@wisechengyi

wisechengyi Jun 26, 2018

Contributor

leave this file untouched?

)

python_dist(
name='ctypes_with_third_party',

This comment has been minimized.

@wisechengyi

wisechengyi Jun 26, 2018

Contributor

nit: rename to python_dist_with_third_party_c to be more descriptive?

This comment has been minimized.

@CMLivingston
)

python_binary(
name='bin_with_third_party',

This comment has been minimized.

@wisechengyi

wisechengyi Jun 26, 2018

Contributor

similar nit: python_binary_with_third_party_c

This comment has been minimized.

@CMLivingston
rang/3.1.0@rang/stable: Installing package\n
Requirements\n rang/3.1.0@rang/stable from 'pants-conan-remote'
\nPackages\n rang/3.1.0@rang/stable:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9\n\n
rang/3.1.0@rang/stable: Already installed!\n"

This comment has been minimized.

@wisechengyi

wisechengyi Jun 26, 2018

Contributor

non-blocking: would be worth exploring if there is more machine friendly output from conan, and possibly using conan as a library https://github.com/conan-io/conan

This comment has been minimized.

@CMLivingston

CMLivingston Jun 26, 2018

Contributor

I will make a ticket to follow up with the conan team about this, and also to explore direct library usage for a later iteration.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

If you've made a Conan ticket, could you link it in a comment next to where we parse the output?

This comment has been minimized.

@CMLivingston

Chris Livingston added some commits Jun 26, 2018

@cosmicexplorer
Copy link
Contributor

cosmicexplorer left a comment

This is really great! I'm super excited we're getting so much closer to doing real work with the native backend. Wrapping Conan in a pex was a fantastic move and like benjy said, opens this up to further extension later. There are some errors (like not requiring the third party libs product in LinkSharedLibraries) and some points where we're doing a ton of unnecessary work (copying files from where Conan downloaded them to). I left several comments on regexes, and some comments about using datatype. Overall this is great, but especially those points I'd like to be addressed. If any of the changes seem to not make sense, let me know, but I would really like most of the changes to be made before merging, especially as a lot of them allow removing a good amount of code, and that will make it significantly easier to extend in the future.

@@ -55,6 +56,7 @@ class NativeCompile(NativeTask, AbstractClass):
# `NativeCompile` will use `workunit_label` as the name of the workunit when executing the
# compiler process. `workunit_label` must be set to a string.
workunit_label = None
workunit_name = 'native-compile'

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

Is this used anywhere?

This comment has been minimized.

@CMLivingston

CMLivingston Jun 27, 2018

Contributor

Yup, on line 278 in the NativeCompileError.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

Oh, that was a mistake, it's supposed to be workunit_label. Could you make that change?

This comment has been minimized.

@CMLivingston

CMLivingston Jul 9, 2018

Contributor

yes

This comment has been minimized.

@CMLivingston

CMLivingston Jul 9, 2018

Contributor

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 10, 2018

Contributor

Thanks!


@staticmethod
def _build_conan_cmdline_args(pkg_spec, os_name=None):
os_name = os_name or get_normalized_os_name()

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

We should be using the Platform datatype from src/python/pants/backend/native/config/environment.py anywhere we do platform-specific logic -- it handles raising an exception and checking that we cover all possible platforms. Also, it looks like conan_os_opt will always be None in that error message? It seems like this method could be refactored to:

CONAN_OS_NAME = {
  'darwin': lambda: 'Macos',
  'linux': lambda: 'Linux',
}

@memoized_property
def fetch_cmdline_args(self):
  platform = Platform.create()
  conan_os_name = platform.resolve_platform_specific(cls.CONAN_OS_NAME)
  args = ['install', self.pkg_spec, '-s', 'os={}'.format(conan_os_name)]
  return args

(adding an argument for the current platform, which is provided by the call in parse())

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

It also looks like this can be made into an instance method? I refactored the above to reflect that. conan_os_name (lowercase) could also be made into a memoized property.

This comment has been minimized.

@CMLivingston

CMLivingston Jul 9, 2018

Contributor

pass

class NativeExternalLibraryFiles(object):
def __init__(self):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

Making the property names lib and include seems to really obscure what they mean (lib could mean the library name, for example) -- is there a reason these aren't just lib_dir and include_dir? Will Conan always have a single include and lib dir?

This comment has been minimized.

@CMLivingston

CMLivingston Jul 9, 2018

Contributor

No reason, and fair point. I will change it now. Yes conan will always have single include/lib dirs.

self._lib = None
self._lib_names = []

@property

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

Is there a reason we have explicit setters and getters? It seems like we could remove all of these and just do:

def __init__(self):
  self.include = None
  self.lib = None
  self.lib_names = []

This comment has been minimized.

@CMLivingston

CMLivingston Jul 9, 2018

Contributor

args.extend(['-s', 'os=' + conan_os_opt])
return args

@classmethod

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

I would recommend calling this method create or from_conan_spec, or something else -- parse could mean any of a number of things. Since this method won't throw if we use platform.resolve_platform_specific() above, I would probably remove this method entirely and (in reference to nearby comments) remove the constructor as well, and make it a datatype object with just a 'pkg_spec' member.

This comment has been minimized.

@CMLivingston

CMLivingston Jul 9, 2018

Contributor

if tp_files_product.lib_names:
for lib_name in tp_files_product.lib_names:
lib_args.append('-l{}'.format(lib_name))
lib_dir_arg = '-L{}'.format(tp_files_product.lib)

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

I think this logic (generating command-line arguments from lib and lib_names) should be moved to the NativeExternalLibraryFetch.NativeExternalLibraryFiles class. That way you could remove this method entirely.

This comment has been minimized.

@CMLivingston

CMLivingston Jul 9, 2018

Contributor

@@ -185,10 +187,20 @@ def get_compiler(self):
def _compiler(self):
return self.get_compiler()

def get_third_party_include_dirs(self):
inc_dir = []
tp_files_product = self.context.products.get_data(NativeExternalLibraryFetch.NativeExternalLibraryFiles)

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

Again, it's usually much more clear to fetch products in the body of the execute() method, and if that is done this method can be removed entirely as well (making it into a separate method actually obscures the logic here a little).

This comment has been minimized.

@CMLivingston
self.fetch_cmdline_args = fetch_cmdline_args

def parse_conan_stdout_for_pkg_sha(self, stdout):
# TODO(cmlivingston): regex this

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

This should just be:

def parse_conan_stdout_for_pkg_sha(self, stdout):
  pkg_spec_escaped = re.escape(self.pkg_spec)
  pkg_spec_pattern = re.compile('^{}:(.*)$'.format(pkg_spec_escaped), flags=re.MULTILINE)
  return pkg_spec_pattern.search(stdout).group(1)

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

Also, Stu informed me on another PR that the TODO(name) style is a remnant of when Pants used to be a smaller project, and that a TODO or FIXME with a sufficient explanation inline or a link to an issue should be used instead at this point. I tend to use e.g. TODO(#5998) when the TODO would be fixed by that issue or PR, but that's purely something I made up -- all that's important is the issue link.

This comment has been minimized.

@CMLivingston

CMLivingston Jul 9, 2018

Contributor

This is a pretty tricky output to parse and I am trying to get this merged asap so I will make a follow-up ticket to fix this.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 10, 2018

Contributor

Sounds like a plan!

from pants.build_graph.target import Target


class ExternalNativeLibrary(Target):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

This is a good name for this target at this point in time! It may be worth revisiting later, along with the C/C++ library target names.

This comment has been minimized.

@CMLivingston
if directory:
inc_dir = [directory]
return inc_dir

def _make_compile_request(self, versioned_target, dependencies):
target = versioned_target.target
include_dirs = [self._include_dirs_for_target(dep_tgt) for dep_tgt in dependencies]

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jun 27, 2018

Contributor

Generally, if I'm mutating a variable, I isolate that along with the initialization of the variable and surround it by at least one line of whitespace on both sides, a habit I learned the hard way, and I would recommend doing that here and anywhere else this occurs.

This comment has been minimized.

@CMLivingston

cosmicexplorer and others added some commits Jul 12, 2018

Chris Livingston
Chris Livingston
Chris Livingston
Chris Livingston

Chris Livingston added some commits Jul 16, 2018

@cosmicexplorer
Copy link
Contributor

cosmicexplorer left a comment

Great! We should really break out a ticket about making sure we only add the thirdparty libs we actually depend on in each target (respecting strict_deps) to compile and link command lines but once that's done this should be good to go.

if os.path.exists(conan_pex_path):
conan_binary = WrappedPEX(PEX(conan_pex_path, interpreter))
return self.ConanBinary(pex=conan_binary)
else:

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

This line has trailing whitespace.

This comment has been minimized.

@CMLivingston
}

def parse_conan_stdout_for_pkg_sha(self, stdout):
# TODO(cmlivingston): regex this

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

This should have its own ticket, see the comments here.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

This is fixed now.

return pkg_sha

@memoized_property
def directory_path(self):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

This should probably have a comment linking to either a description of the package spec or a description of the conan directory layout -- it's not clear why this works currently.

This comment has been minimized.

@CMLivingston

def _prepare_vts_results_dir(self, vts):
"""
Given a `VergetTargetSet`, prepare its results dir.

This comment has been minimized.

@cosmicexplorer
safe_mkdir(vt_set_results_dir)
return vt_set_results_dir

def _populate_task_product(self, results_dir, task_product):

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

Ah, that's kind of silly. This is great then.

longest_dir_prefix, mergetree, read_file, relative_symlink, relativize_paths,
rm_rf, safe_concurrent_creation, safe_file_dump, safe_mkdir, safe_mkdtemp,
safe_open, safe_rm_oldest_items_in_dir, safe_rmtree, touch)
absolute_symlink, check_no_overlapping_paths, fast_relpath,

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

Can this be reverted? Is travis lint erroring out on this?

This comment has been minimized.

@CMLivingston

CMLivingston Jul 17, 2018

Contributor

My local pre-commit hook bombed out due to import order.

@@ -71,39 +71,44 @@ def select_linker(platform, native_toolchain):
# 'darwin': lambda: Get(Linker, XCodeCLITools, native_toolchain._xcode_cli_tools),
# 'linux': lambda: Get(Linker, Binutils, native_toolchain._binutils),
# })
#

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

This whole file needs much better documentation. I will do that as part of #5951.

@@ -41,6 +41,16 @@ def get_compile_settings(self):
def get_compiler(self):
return self._request_single(CppCompiler, self._toolchain)

def _make_compile_argv(self, compile_request):
# FIXME: this is a temporary fix, do not do any of this kind of introspection.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

Could you please add a link to #5951 here?

This comment has been minimized.

@CMLivingston
else:
link_request = self._make_link_request(
vt, compiled_objects_product, native_target_deps_product)
vt, compiled_objects_product, native_target_deps_product, external_libs_product)

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

If I'm understanding this correctly, all thirdparty libs are exposed to all targets being compiled, which doesn't seem like it's what we want, and we should break out a ticket to only expose the thirdparty targets each C or C++ target depends on.

This comment has been minimized.

@CMLivingston
rang/3.1.0@rang/stable: Installing package\n
Requirements\n rang/3.1.0@rang/stable from 'pants-conan-remote'
\nPackages\n rang/3.1.0@rang/stable:5ab84d6acfe1f23c4fae0ab88f26e3a396351ac9\n\n
rang/3.1.0@rang/stable: Already installed!\n"

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 17, 2018

Contributor

If you've made a Conan ticket, could you link it in a comment next to where we parse the output?

Chris Livingston added some commits Jul 17, 2018

@CMLivingston

This comment has been minimized.

Copy link
Contributor

CMLivingston commented Jul 18, 2018

ping @stuhood for final review

@stuhood
Copy link
Member

stuhood left a comment

This looks great. Thanks for iterating Chris!


all_shared_libs_by_name = {}

# FIXME: convert this to a v2 engine dependency injection.

This comment has been minimized.

@stuhood

stuhood Jul 18, 2018

Member

Or just to a constant/global, frankly. It's not going to change, regardless of options or anything else.

This comment has been minimized.

@cosmicexplorer

cosmicexplorer Jul 18, 2018

Contributor

Noted!

@cosmicexplorer cosmicexplorer merged commit 4aec12c into pantsbuild:master Jul 18, 2018

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment