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

add a --toolchain-variant option to select the compiler for C/C++ #6800

Merged
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
e78a483
add a --toolchain-variant option to select the compiler for C/C++
cosmicexplorer Nov 21, 2018
a799016
add some logging around the compiler selection to validate toolchain …
cosmicexplorer Nov 22, 2018
71ca371
extend the exe checking to the link task
cosmicexplorer Nov 22, 2018
bf9e9a7
remove completed TODO
cosmicexplorer Nov 22, 2018
363c940
add magic linking lib dirs args to gcc and make the other test work f…
cosmicexplorer Nov 26, 2018
1de3799
add crti.o back to the command line
cosmicexplorer Nov 27, 2018
6ed4110
refactor libc resolution and make tests pass on osx
cosmicexplorer Nov 27, 2018
c4b4f28
enable libc search in the link testing
cosmicexplorer Nov 27, 2018
ebeb2fa
use repr for debug logs
cosmicexplorer Nov 27, 2018
282e9db
if this makes travis work finally we need to add a linter for this im…
cosmicexplorer Nov 27, 2018
b53b5c0
fix error introduced by incorrect git rebase operation
cosmicexplorer Nov 28, 2018
7ee1dd9
try resetting the toolchain_variant to llvm for now and fix some tests
cosmicexplorer Nov 29, 2018
1b9804e
move --tooldchain-variant to NativeToolchain to remove boilerplate in…
cosmicexplorer Nov 29, 2018
8375a62
make toolchain variant selection more declarative by moving the logic…
cosmicexplorer Nov 29, 2018
b32d643
separate extra object files from extra_args on the linker
cosmicexplorer Nov 29, 2018
1a2108d
pipe ToolchainVariantRequest into BuildLocalPythonDistributions
cosmicexplorer Nov 29, 2018
15632fe
move from explicit crti.o file paths to adding it to the LIBRARY_PATH
cosmicexplorer Nov 29, 2018
0e3c3e8
gcc on osx needs a bit more help to work at runtime
cosmicexplorer Nov 29, 2018
0703beb
enable search for libc so gcc can pass these tests
cosmicexplorer Nov 29, 2018
6af5aca
fix the last couple tests
cosmicexplorer Nov 29, 2018
9a25d32
fill out ???s and link to issues for TODOs
cosmicexplorer Nov 30, 2018
64e3e54
expand docstrings and add types to Executable properties
cosmicexplorer Dec 5, 2018
5473093
remove some of the include dir dark magic by standardizing options
cosmicexplorer Dec 5, 2018
55e6908
add TODO links
cosmicexplorer Dec 5, 2018
3cd803a
don't add buggy include search flags to the clang invoke, and test bo…
cosmicexplorer Dec 7, 2018
408f4cd
remove notes to self
cosmicexplorer Dec 7, 2018
a3b4339
remove remaining -nostdinc args and add a more concise TODO
cosmicexplorer Dec 7, 2018
45e4c7a
fix issue link
cosmicexplorer Dec 7, 2018
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
1 change: 1 addition & 0 deletions src/python/pants/backend/native/config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ python_library(
dependencies=[
'3rdparty/python:future',
'src/python/pants/engine:rules',
'src/python/pants/util:meta',
'src/python/pants/util:objects',
'src/python/pants/util:osutil',
],
Expand Down
44 changes: 36 additions & 8 deletions src/python/pants/backend/native/config/environment.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,9 @@

import os
from abc import abstractproperty
from builtins import object

from pants.engine.rules import SingletonRule
from pants.util.meta import AbstractClass
from pants.util.objects import datatype
from pants.util.osutil import all_normalized_os_names, get_normalized_os_name
from pants.util.strutil import create_path_env_var
Expand Down Expand Up @@ -42,34 +42,54 @@ def resolve_platform_specific(self, platform_specific_funs):
return fun_for_platform()


class Executable(object):
# NB: @abstractproperty requires inheriting from AbstractClass to work!
Copy link
Contributor

Choose a reason for hiding this comment

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

This is both untrue and doesn't stand the novelty test. Using AbstractClass is a convenient way to properly support @abstractproperty but not the only way, and we do this in many places in the codebase, so the revelation - though personally novel is not so generally and so probably not worth a comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a note to myself which I was planning to remove, and this was a note that I was going to word as "inheriting from AbstractClass, or an equivalent method to set the metaclass", then decided I probably didn't need to go that in depth for now. I am aware this is being presented as code for review, and I agree that this shouldn't be in there, but as mentioned in the other comment, since I "need" travis to run tests, this all gets flattened.

I suppose in the future I could keep TODOs in another file, but that's not ideal and I would prefer to make the local testing situation better to avoid pushing code to a PR that I intend to remove.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This note has been removed.

class Executable(AbstractClass):

@abstractproperty
def path_entries(self):
"""A list of directory paths containing this executable, to be used in a subprocess's PATH.

This may be multiple directories, e.g. if the main executable program invokes any subprocesses.

:rtype: list of str
"""

@abstractproperty
def exe_filename(self):
"""The "entry point" -- which file to invoke when PATH is set to `path_entries()`.

:rtype: str
"""

# TODO(#???): rename this to 'runtime_library_dirs'!
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fill in the issue link or else kill the unfinished #??? thought. A todo that itself is in a half-done state is just too much. Maybe fine when you're hacking locally, but you really should clean things up like this before posting for review. You may be making an effort at this though and just missed this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it was possible to run testing through the travis shards before pushing a commit, I would never have any such comments. I'll grant that magit in emacs makes it easy to stage hunks of files and not the whole diff, but since bootstrapping pants when running in a docker image takes a very long time, the only method of iteration I can see is to abuse travis runs on each commit. I feel that this has continually led to the assumption that I think TODO(#???) is an acceptable thing to have in code for review, which I don't, at all -- I left this particular comment as is because I was trying to test the new code against travis instead of context switching. This also applies to the NB I left for myself about @abstractproperty, which was mentioned above. I have been working on trying to fix the local testing situation but it is difficult to do all of these things at the same time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the (#???)!

@abstractproperty
def library_dirs(self):
"""Directories containing shared libraries that must be on the runtime library search path.

Note: this is for libraries needed for the current Executable to run -- see LinkerMixin below
for libraries that are needed at link time."""
for libraries that are needed at link time.

@abstractproperty
def exe_filename(self):
"""The "entry point" -- which file to invoke when PATH is set to `path_entries()`."""
:rtype: list of str
"""

@property
def extra_args(self):
"""Additional arguments used when invoking this Executable.

These are typically placed before the invocation-specific command line arguments.

:rtype: list of str
"""
return []

_platform = Platform.create()

@property
def as_invocation_environment_dict(self):
"""A dict to use as this Executable's execution environment.

:rtype: dict of string -> string
"""
lib_env_var = self._platform.resolve_platform_specific({
'darwin': lambda: 'DYLD_LIBRARY_PATH',
'linux': lambda: 'LD_LIBRARY_PATH',
Expand All @@ -92,13 +112,18 @@ class LinkerMixin(Executable):

@abstractproperty
def linking_library_dirs(self):
"""Directories to search for libraries needed at link time."""
"""Directories to search for libraries needed at link time.

:rtype: list of str
"""

@abstractproperty
jsirois marked this conversation as resolved.
Show resolved Hide resolved
def extra_object_files(self):
"""A list of object files required to perform a successful link.

This includes crti.o from libc for gcc on Linux, for example.

:rtype: list of str
"""
jsirois marked this conversation as resolved.
Show resolved Hide resolved

@property
Expand Down Expand Up @@ -131,7 +156,10 @@ class CompilerMixin(Executable):

@abstractproperty
def include_dirs(self):
"""Directories to search for header files to #include during compilation."""
"""Directories to search for header files to #include during compilation.

:rtype: list of str
"""

@property
def as_invocation_environment_dict(self):
Expand Down
2 changes: 1 addition & 1 deletion src/python/pants/backend/native/subsystems/binaries/gcc.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def _cpp_include_dirs(self):
('include/c++', self.version()),
])

# TODO: determine whether there is any manual explaining when any of these file paths are
# TODO(#6143): determine whether there is any manual explaining when any of these file paths are
# necessary.
# This file is needed for C++ compilation.
cpp_config_header_path = self._file_mapper.assert_single_path_by_glob(
Expand Down
4 changes: 1 addition & 3 deletions src/python/pants/backend/native/subsystems/libc_dev.py
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,4 @@ def _host_libc(self):

@memoized_method
def get_libc_objects(self):
if not self.get_options().enable_libc_search:
return []
return [self._host_libc.crti_object]
return [self._host_libc.crti_object] if self.get_options().enable_libc_search else []
20 changes: 14 additions & 6 deletions src/python/pants/backend/native/subsystems/native_toolchain.py
Original file line number Diff line number Diff line change
Expand Up @@ -141,10 +141,12 @@ class GCCInstallLocationForLLVM(datatype(['toolchain_dir'])):
"""This class is convertible into a list of command line arguments for clang and clang++.

This is only used on Linux. The option --gcc-toolchain stops clang from searching for another gcc
on the host system. The option appears to only exist on Linux clang and clang++."""
on the host system. The option appears to only exist on Linux clang and clang++.
"""

@property
def as_clang_argv(self):
# TODO(#6143): describe exactly what this argument does to the clang/clang++ invocation!
return ['--gcc-toolchain={}'.format(self.toolchain_dir)]


Expand All @@ -161,6 +163,7 @@ def select_llvm_c_toolchain(platform, native_toolchain):
llvm_c_compiler_args = [
'-x', 'c', '-std=c11',
'-nobuiltininc',
'-nostdinc',
Copy link
Contributor

Choose a reason for hiding this comment

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

You've added this in four places and it seems related to the failing unit tests:

  • TestNativeToolchain.test_hello_c_clang
  • TestNativeToolchain.test_hello_c_gcc
  • TestNativeToolchain.test_hello_cpp_clangpp
  • TestNativeToolchain.test_hello_cpp_gpp

As well as in the TestProjectsIntegrationTest.test_shard_44 integration test which shows:

...
                     E   	03:49:57 00:01   [native-compile]
                     E   	03:49:57 00:01     [native-third-party-fetch]
                     E   	                   Invalidated 2 targets.
                     E   	03:50:11 00:15     [c-for-ctypes]
                     E   	03:50:11 00:15     [cpp-for-ctypes]
                     E   	                   Invalidated 2 targets.
                     E   	                   selected compiler exe name: 'clang++'
                     E   	03:50:11 00:15       [cpp-compile]
                     E   	                     clang-6.0: warning: argument unused during compilation: '-nobuiltininc' [-Wunused-command-line-argument]
                     E   	                     /home/travis/build/pantsbuild/pants/testprojects/src/python/python_distribution/ctypes_with_extra_compiler_flags/some_more_math.cpp:2:10: fatal error: 'assert.h' file not found
                     E   	                     #include <assert.h>
                     E   	                              ^~~~~~~~~~
                     E   	                     1 error generated.
...

And CTypesIntegrationTest.test_ctypes_third_party_integration which shows:

...
                     E   	03:21:58 00:01   [native-compile]
                     E   	03:21:58 00:01     [native-third-party-fetch]
                     E   	                   Invalidated 2 targets.
                     E   	03:22:11 00:14     [c-for-ctypes]
                     E   	03:22:12 00:15     [cpp-for-ctypes]
                     E   	                   Invalidated 1 target.
                     E   	                   selected compiler exe name: 'clang++'
                     E   	03:22:12 00:15       [cpp-compile]
                     E   	                     clang-6.0: warning: argument unused during compilation: '-nobuiltininc' [-Wunused-command-line-argument]
                     E   	                     In file included from /home/travis/build/pantsbuild/pants/testprojects/src/python/python_distribution/ctypes_with_third_party/some_more_math_with_third_party.cpp:4:
                     E   	                     /home/travis/build/pantsbuild/pants/.pants.d/tmp/tmpQX3l3R.pants.d/native-compile/native-third-party-fetch/a33349f38f6e/testprojects.src.python.python_distribution.ctypes_with_third_party.rang/current/include/rang.hpp:15:10: fatal error: 'unistd.h' file not found
                     E   	                     #include <unistd.h>
                     E   	                              ^~~~~~~~~~
                     E   	                     1 error generated.
...

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Dec 7, 2018

Choose a reason for hiding this comment

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

-nobuiltininc (which clang recognizes, but doesn't use), as well as -nostdinc (which both clang and gcc recognize) have been removed, with a more concise TODO link to #6143 to look into precisely what these flags do (and whether we need to start making pull requests to llvm...).

]

if platform.normalized_os_name == 'darwin':
Expand All @@ -182,7 +185,7 @@ def select_llvm_c_toolchain(platform, native_toolchain):
llvm_linker_wrapper = yield Get(LLVMLinker, NativeToolchain, native_toolchain)
llvm_linker = llvm_linker_wrapper.linker

# TODO: create an algebra so these changes are more clear!
# TODO(#6855): introduce a more concise way to express these compositions of executables.
working_linker = llvm_linker.copy(
path_entries=(llvm_linker.path_entries + working_c_compiler.path_entries),
exe_filename=working_c_compiler.exe_filename,
Expand All @@ -203,6 +206,7 @@ def select_llvm_cpp_toolchain(platform, native_toolchain):
# implementation, or any from the host system. Instead, we use include dirs from the
# XCodeCLITools or GCC.
'-nobuiltininc',
'-nostdinc',
'-nostdinc++',
]

Expand All @@ -226,7 +230,7 @@ def select_llvm_cpp_toolchain(platform, native_toolchain):
# NB: we use g++'s headers on Linux, and therefore their C++ standard library.
include_dirs=provided_gpp.include_dirs,
extra_args=(llvm_cpp_compiler_args + provided_clangpp.extra_args + gcc_install.as_clang_argv))
# TODO: why are these necessary? this is very mysterious.
# TODO(#6855): why are these necessary? this is very mysterious.
extra_linking_library_dirs = provided_gpp.library_dirs + provided_clangpp.library_dirs
# Ensure we use libstdc++, provided by g++, during the linking stage.
linker_extra_args=['-stdlib=libstdc++']
Expand All @@ -253,12 +257,15 @@ def select_gcc_c_toolchain(platform, native_toolchain):
# GCC needs an assembler, so we provide that (platform-specific) tool here.
assembler = yield Get(Assembler, NativeToolchain, native_toolchain)

gcc_c_compiler_args = [
'-x', 'c', '-std=c11',
'-nostdinc',
]

if platform.normalized_os_name == 'darwin':
# GCC needs access to some headers that are only provided by the XCode toolchain
# currently (e.g. "_stdio.h"). These headers are unlikely to change across versions, so this is
# probably safe.
# TODO: we should be providing all of these (so we can eventually phase out XCodeCLITools
# entirely).
xcode_clang = yield Get(CCompiler, XCodeCLITools, native_toolchain._xcode_cli_tools)
new_include_dirs = provided_gcc.include_dirs + xcode_clang.include_dirs
Copy link
Contributor

@CMLivingston CMLivingston Nov 30, 2018

Choose a reason for hiding this comment

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

Why was this swap necessary? (it might have implications for internal use cases, will verify)

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Nov 30, 2018

Choose a reason for hiding this comment

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

This is actually a correction which is necessary for tests to pass (this is due to the gcc headers' use of #include_next). The same switch is done for the GCCCppToolchain. The reason this bug existed before was because there was no concise way to say "this executable is composed of these other executables' resources", so we lost a lot of readability by requiring repeated code.

Copy link
Contributor

Choose a reason for hiding this comment

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

...and so this probably deserves a comment in the code. Your cohort which is probably the closest to this code found it mysterious - excellent indicator that it truly is and should get note not just in review.

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Dec 5, 2018

Choose a reason for hiding this comment

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

I agree. It has historically been difficult to justify spending time on fixing tech debt for this backend as I wasn't able to get a lot of help understanding the time cost of different approaches, so I attempted to muddle through solutions and leave millions of TODOs because I was extremely concerned about wasting time on unnecessary abstractions. @CMLivingston has been extremely helpful in developing the larger picture.

The first longstanding issue is this PR, which addresses the lack of toolchain choice, which you have mentioned multiple times previously (which was appreciated). The second is #6855, which I have listed in a couple comments in this file, and which is intended to remove the mystery from repeated but slightly varying Executable compositions, and to remove the chance of making ordering mistakes like the above by combining Executables at a higher level of abstraction. The last is #5970, which needs to be redone entirely but may be much simpler after the previous are complete.

else:
Expand All @@ -267,7 +274,7 @@ def select_gcc_c_toolchain(platform, native_toolchain):
working_c_compiler = provided_gcc.copy(
path_entries=(provided_gcc.path_entries + assembler.path_entries),
include_dirs=new_include_dirs,
extra_args=['-x', 'c', '-std=c11'])
extra_args=gcc_c_compiler_args)

gcc_linker_wrapper = yield Get(GCCLinker, NativeToolchain, native_toolchain)
gcc_linker = gcc_linker_wrapper.linker
Expand All @@ -290,6 +297,7 @@ def select_gcc_cpp_toolchain(platform, native_toolchain):

gcc_cpp_compiler_args = [
'-x', 'c++', '-std=c++11',
'-nostdinc',
'-nostdinc++',
]

Expand Down
21 changes: 4 additions & 17 deletions src/python/pants/backend/native/subsystems/xcode_cli_tools.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,9 @@ def _all_existing_install_prefixes(self):
# NB: We use @memoized_method in this file for methods which may raise.
@memoized_method
def _get_existing_subdirs(self, subdir_name):
# TODO: consume ArchiveFileMapper instead of doing all that logic over again in this file.
# TODO(#6143): We should attempt to use ParseSearchDirs here or find some documentation on which
# directories we should be adding to the include path, and why. If we do need to manually
# specify paths, use ArchiveFileMapper instead of doing all that logic over again in this file.
maybe_subdirs = [os.path.join(pfx, subdir_name) for pfx in self._all_existing_install_prefixes]
existing_dirs = [existing_dir for existing_dir in maybe_subdirs if is_readable_dir(existing_dir)]

Expand Down Expand Up @@ -125,22 +127,7 @@ def lib_dirs(self):

@memoized_method
def include_dirs(self, include_cpp_inc=False):
base_inc_dirs = self._get_existing_subdirs('include')

all_inc_dirs = base_inc_dirs
for d in base_inc_dirs:
# TODO: figure out what this directory does and why it's not already found by this compiler.
secure_inc_dir = os.path.join(d, 'secure')
if is_readable_dir(secure_inc_dir):
all_inc_dirs.append(secure_inc_dir)

# TODO: figure out why we need to specify this directory separately.
if include_cpp_inc:
cpp_inc_dir = os.path.join(d, 'c++/v1')
if is_readable_dir(cpp_inc_dir):
all_inc_dirs.append(cpp_inc_dir)

return all_inc_dirs
return self._get_existing_subdirs('include')

@memoized_method
def assembler(self):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ def _assert_ctypes_binary(self, toolchain_variant):

# Check that we have selected the appropriate compilers for our selected toolchain variant,
# for both C and C++ compilation.
# TODO(#6866): don't parse info logs for testing!
for compiler_name in self._compiler_names_for_variant[toolchain_variant]:
self.assertIn("selected compiler exe name: '{}'".format(compiler_name),
Copy link
Contributor

Choose a reason for hiding this comment

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

I am definitely not a fan of scraping logs to implement tests. This was cheap since there was already an integration test but expensive since it adds production logging of questionable value (it effectively parrots the very well tested option system) and that will surprisingly break tests when altered. Consider ~never doing this when possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will make an effort to remember your comment as the concerns you mention are quite bothersome. I would love to implement any alternative -- the info logs here were pretty much exactly for testing purposes and that conflation makes me sick at heart.

I suppose the point of an integration test is not to try to inspect internal state but to examine some properties of the result of a pants run. I was wondering whether it was possible to tag an object file or shared library with metadata that includes e.g. which compiler produced it. The rationale for testing both compilers here is to avoid introducing a breakage in one toolchain, and just changing the option value doesn't seem like it does enough to verify that we are choosing the right compiler.

It's probably possible to propagate this metadata (which compiler/linker was used) into the produced python_dist() (by making some edits to BuildLocalPythonDistributions -- not by tagging any native code), which we could then inspect in this test. I will look up whether this is feasible -- adding a "compiler" tag (or something) to a dist seems like a very interesting and concise way to begin the foray into classifying compatibility between e.g. native code cache artifacts. I'm also seeing the implementation of this requiring a relatively small amount of code, which would be feasible to add in this PR or another. I'm working on this approach now to avoid the addition of these info-level logs. I'm thinking of potentially storing another file in data_files which contains info about which compiler was used.

Internally we have had to address the situation several times where e.g. tensorflow and related wheels assume quite a lot of things about the native code execution environment (mostly relating to glibc and stdlibc++ version), so if there is some way to tag wheels with this type of more in-depth native code compatibility (e.g. which compiler is used), we could potentially apply that method to 3rdparty wheels as well in the future.

Bazel might have an analogue of a far more general form of compatibility with e.g. compatible_with (with application to bazel toolchains) -- I'm not familiar enough with any of that to say whether it's applicable here. Bazel's compatibility also handles e.g. ensuring "secret" code doesn't get mixed with non-"secret" code, so it's a far more general formulation than I care about right now, but was interesting to look at just now.

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Nov 22, 2018

Choose a reason for hiding this comment

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

I've linked the above to #native so the content isn't lost. In essence, I'm just looking for a simple method for now to propagate the identity (even just the compiler name) of the toolchain we use to the python_dist() we build, in a way we can check later in this integration test. Right now this is the only such check we make so it can probably be hacked on to the specific dist we build, now that I think of it.

Copy link
Contributor

Choose a reason for hiding this comment

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

That would certainly be preferable. That said, just keep scope in mind. All this to test NativeTask.get_c_toolchain_variant and NativeTask.get_cpp_toolchain_variant work - which just tests subsystems / options. I think that can be done simply in a unit test (although you do request engine products - not sure how easy that is but I'm pretty sure unit test infra has support for this already). You need not test every bit of program logic through an integration test or unit test or both. Pick your battles based on risk or complexity and this is a tiny skirmish your whacking with a howitzer.

Copy link
Contributor

Choose a reason for hiding this comment

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

So, what's the status of this? The test still log scrapes. Do you want to follow-up with an issue tracking fixing this test and removing the production log.info it requires?

Copy link
Contributor Author

@cosmicexplorer cosmicexplorer Dec 5, 2018

Choose a reason for hiding this comment

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

Added a link to #6866 to cover my interpretation of this discussion!

pants_run.stdout_data)
Expand Down