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

create a compiler and/linker compatibility argument like python interpreter constraints #7122

Closed
cosmicexplorer opened this issue Jan 21, 2019 · 4 comments

Comments

@cosmicexplorer
Copy link
Contributor

We created an example of building TensorFlow custom operators in #7046. As described in the TensorFlow docs, g++ must be used to compile custom operators. We currently work around that by defaulting the the toolchain_variant to gnu, which tells pants to use g++ to compile C++ (and ld from binutils to link it).

We should probably have some analogy of python interpreter constraints for compilers and linkers to overcome this hygienically, without having incompatible build products mixed around.

bazel seems to be trying to do this with toolchains (that page also has some good reasoning on how to do this in general), but I am under the impression that CROSSTOOL is still the main/only way to actually hook into compilers and linkers, as there is a lot of implementation behind the toolchains idea that bazel hasn't gotten around to yet. If it works, we may be able to steal the idea, but we should make sure that bazel toolchains are trying to solve the same problem we are.

@cosmicexplorer
Copy link
Contributor Author

This ticket consists of:

  1. Moving --toolchain-variant from NativeBuildSettings to NativeBuildStepSettings.
  2. Adding toolchain_variant as an argument and to the payload of NativeLibrary (also add a NativeLibrary#toolchain_variant property accessing that payload field).
  3. Adding 'toolchain_variant': 'toolchain_variant' to mirrored_option_to_kwarg_map, which is from MirroredTargetOptionMixin.
  4. Making NativeTask#get_c{,pp}_toolchain_variant() accept a target as a second argument, and doing the same for C{,pp}Compile#get_compiler() and LinkSharedLibraries#linker.
  5. After doing all of the above, you should be able to edit NativeCompile#_make_compile_request() and LinkSharedLibraries#_make_link_request() to create the appropriate compiler or linker from the given target, and everything else should just work.

In terms of testing, I would be fine if we added a couple targets to the ctypes testproject which use the new argument, and extended the current test which checks the info log to see which compiler was selected (it would be good if we could extend that to the linker as well if it's not already). #7046 would be great testing, but it is now blocked on #7016, so #7046 is probably going to land after this ticket.

Some notes on mirrored_option_to_kwarg_map:

  1. The name is wrong, it should be mirrored_option_to_target_field_map, or something, since we are not accessing the keyword argument of the target when constructed, but rather the property of the target with that name, as in NativeLibrary#compiler_option_sets.
  2. Feel free to remove the dict and just make it a list of fields, since I think we will always want the key to be the same as the value here.

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Jan 22, 2019

Last thing -- I think we should eventually enforce that all of the targets that we link together in LinkSharedLibraries were compiled by the same toolchain, by:

  1. Compiling a target multiple times, once per toolchain it is compatible with.
  2. Error out if targets specify incompatible toolchains.

I don't think we should do this as part of the above, and would prefer to wait until we have ported the native backend to v2 rules. For now we can assume that compile outputs from clang and gcc are compatible, and stick to the above (it might be good to leave a comment about this, though).

cosmicexplorer added a commit that referenced this issue Jan 25, 2019
…tensions in setup.py (#7126)

### Problem

Resolves #7016 (see [mirrored `pants-devel` post](https://groups.google.com/forum/#!topic/pants-devel/Y37d0tf4bKo)). This unblocks #6855 and #7046, as well as further work on #7122.

Closes #5661, closes #6841. I also made a long, long-overdue [github project for native code](https://github.com/pantsbuild/pants/projects/11).

### Solution

- Use the host environment to invoke compilers and linkers as desired in distutils, don't try to inject our own toolchain. See #5661 and #7016 for why this is extremely difficult to maintain. 

### Result

Further native backend iteration is unblocked.
@cosmicexplorer
Copy link
Contributor Author

While #7179 addressed this, I want to keep this issue open to conserve the rest of the thoughts here. #7183 is also relevant as the precedence chain described there is what #7179 enables us to do with this "compatibility" concept as well.

@Eric-Arellano
Copy link
Contributor

Closing as the native backend was initially removed from 2.0.

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

No branches or pull requests

2 participants