we can now depend on non-header-only libraries from conan by building them ourselves #6486

cosmicexplorer · 2018-09-11T04:25:24Z

Problem

As @stuhood was valiantly making headway on #6178, we realized that we didn't have any examples of 3rdparty native libraries from Conan that weren't header-only (so we weren't testing the native linking at all for 3rdparty libs). Also, we weren't passing static archives to the linker correctly at all -- we need to refer to them with the full .a file path, and after the object files that depend on them.

Solution

Separate static archives from shared libraries when creating NativeExternalLibraryFiles so we can correctly use them in the link phase.
Add --build missing to the Conan command line, and wrap its invocation environment in our native toolchain. This allows us to depend on native libraries which do not have precompiled binaries (most of them).
Add a python_binary which uses the IrrXML library from conan-central (provided as a static archive) in an existing python_dist to parse an xml file and print the node names.
Add an integration test for the above.

Result

Now we can depend on native libraries like IrrXML in our ctypes_compatible_c{,pp}_library targets, which is pretty neat.

CMLivingston · 2018-09-11T17:08:10Z

src/python/pants/backend/native/tasks/native_external_library_fetch.py

+      include_dir=include_dir,
+      lib_dir=lib_dir,
+      lib_names=tuple(lib_names),
+      static_archive_paths=tuple(static_archive_paths))


We cast as tuple because we want these kwargs to be immutable, correct?

Tuples are hashable -- while this particular datatype doesn't go through the v2 engine, it makes it easier if you can assume your input is a tuple for other datatypes which do get produced or consumed by rules.

Hm. Immutability is a much better reason than the one I just gave.

CMLivingston · 2018-09-11T17:47:46Z

src/python/pants/backend/native/tasks/native_external_library_fetch.py

@@ -69,36 +77,28 @@ 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),


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?

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.

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

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).

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.

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

Thanks for clarifying, sounds good.

CMLivingston

This looks great! Thanks for taking care of this. Just added a couple questions. We might be able to omit the direct paths to the archives?

cosmicexplorer · 2018-09-11T22:58:18Z

We might be able to do that -- or if we can't, there should at least be a detailed explanation of why. I will add this.

stuhood

Looks fine to me, but will conflict quite a bit with #6178 / #6492.

stuhood · 2018-09-12T20:47:06Z

src/python/pants/backend/native/tasks/link_shared_libraries.py


-    return cls(*args, external_lib_dirs=lib_dirs, external_lib_names=lib_names, **kwargs)
+    return cls(


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

cosmicexplorer · 2018-09-13T18:03:21Z

I'm waiting on #6492 to get merged now. I've added a TODO to figure out why using theIrrXML static library isn't working with the usual -L/-l arguments.

cosmicexplorer · 2018-10-14T20:28:18Z

Very legit CI failures, still working on this one.

…6630) Takeover of #6492 (which has completely passed review) as it was blocked by progress on two other PRs I have up (#6486, #6628) due to potential merge conflicts, which I can resolve when they come up for each of these PRs to unblock landing them in parallel. The body of #6492 was: ### Problem As described in #6178, the `NativeExternalLibraryFiles` products of a `conan` resolve are not currently partitioned by target, which means it isn't possible to expose individual 3rdparty deps to only their declared dependents. ### Solution Partition the `NativeExternalLibraryFiles` product using `UnionProduct` while producing it in `NativeExternalLibraryFetch` (and switch to using isolated `vt.results_dir` directories per `external_native_library` target), and consume the split product in `NativeCompile` and `LinkSharedLibraries`. ### Result Only declared dependents have access to 3rdparty libraries. Fixes #6178.

@CMLivingston

… design mistake (#6628) ### Problem *This probably blocks #6486 and #6492.* As @CMLivingston and I realized sometime this week (to my *immense* dismay), we had no tests of native library targets depending on each other. When making these examples, I realized two things: 1. `--strict-deps` doesn't really make sense as two separate options for C and C++ targets which can depend on each other. 2. The implementation of `--strict-deps` in the native backend ("caching" native target interdependencies in a product from the compiler tasks) was absurd and unnecessary. I think I vaguely recall that the fact that `LinkSharedLibraries` previously did not have a subsystem dependency on the `CCompileSettings` or `CppCompileSettings` subsystems may have led to a subtle caching bug in the link task when options in those subsystems were modified, but I don't remember all the details right now. Regardless, that bug would have been fixed with this PR -- see the below: ### Solution - Move dependency calculation into a single `NativeBuildSettings` subsystem which is consumed by compile and link tasks, and use it to calculate the dependencies in both tasks. - Drop the very unnecessary `NativeTargetDependencies` product. - Move a ton of logic into `NativeTask` that really should have been there instead of `NativeCompile`, but couldn't because of the hacked together dependency calculation. - Add a testproject with C and C++ targets depending on each other, and an integration test showing the above all works. ### Result We have a working example of C and C++ targets depending on each other, resulting in idiomatic enough C and C++, which also work extremely effectively in a `python_dist()` target.

cosmicexplorer · 2018-10-18T23:13:10Z

I'm pretty sure the remaining CI failure is because travis doesn't support the newer gcc libstdc++ ABI by default ("default" meaning "the default compiler installation has an old shared lib"), and we don't have any way currently of pointing it to our own libstdc++.so.6 from our gcc binary during a python run. This can probably be fixed for now with a bool compile option which toggles the use of -D_GLIBCXX_USE_CXX11_ABI=0 when compiling, which we would switch on for this integration test (along with a TODO after we can figure out #5970).

cosmicexplorer · 2018-10-19T20:39:50Z

Some things were fixed by isolating the PATH to just contain the C++ toolchain, and exposing our CMake binary as a NativeTool, but now there are further errors (it requires make to be able to build the library we want to use -- see https://travis-ci.org/pantsbuild/pants/jobs/443860202). I strongly suspect that providing make as a NativeTool and adding it to the PATH will resolve the issue, but will repro the issue locally first with a docker image.

also separate static archives from shared libs

…args to pass travis

…t when building conan deps

cosmicexplorer · 2018-10-22T16:37:04Z

Blocking on pantsbuild/binaries#84 for the remaining CI failures (all of them are integration tests failing when building the native lib we add in this PR because they don't have gnu make on the PATH (and the one on travis is pulling in other binaries we don't want, so we can't not isolate the PATH here (it seems))).

Eric-Arellano · 2020-05-18T00:15:37Z

Closing due to this being stale. Of course, please feel free to reopen this.

cosmicexplorer requested review from stuhood and CMLivingston September 11, 2018 04:25

CMLivingston reviewed Sep 11, 2018

View reviewed changes

CMLivingston approved these changes Sep 11, 2018

View reviewed changes

stuhood approved these changes Sep 12, 2018

View reviewed changes

cosmicexplorer mentioned this pull request Sep 13, 2018

Split conan resolve product by target to enforce declared dependencies #6492

Closed

cosmicexplorer mentioned this pull request Oct 14, 2018

Add testproject with C/C++ interdependent targets, fix native backend design mistake #6628

Merged

cosmicexplorer force-pushed the third-party-conan-dep-with-lib branch from 732de06 to 6effd19 Compare October 14, 2018 02:55

cosmicexplorer mentioned this pull request Oct 14, 2018

Split conan resolve by native_external_library targets (takeover). #6630

Merged

cosmicexplorer force-pushed the third-party-conan-dep-with-lib branch from ca6cf61 to dc70ebc Compare October 18, 2018 02:54

cosmicexplorer force-pushed the third-party-conan-dep-with-lib branch from 93edd4d to 53a8013 Compare October 19, 2018 19:43

cosmicexplorer added 11 commits October 20, 2018 21:41

add support for / example of a C++ 3rdparty non-header dependency

8619150

also separate static archives from shared libs

add integration test for our xml parsing that we build ourselves

0d89282

fix the conan expected command line

784ddba

python 3 compat fix and expand TODO

b70c9bc

add clearer todo about static libs on the command line

b8e8858

fix bug with use of native target deps product and try adding -rpath …

c192e98

…args to pass travis

remove unused rpath args

e4dbe2f

fix some blunders (but not all)

8ece080

fix another blunder

27f93dc

update a docstring and add a str import for py3

b268d6c

introduce CMake BinaryTool and use it to fully isolate the environmen…

4d4689d

…t when building conan deps

cosmicexplorer force-pushed the third-party-conan-dep-with-lib branch from 53a8013 to 4d4689d Compare October 22, 2018 01:58

cosmicexplorer added 2 commits October 21, 2018 19:37

remove unnecessary intermediate PexInfo

d5a25b7

add gnu make NativeTool (waiting on corresponding binaries)

5e320b7

This was referenced Oct 22, 2018

add gnu make 4.2.1 pantsbuild/binaries#84

Open

Add make 4.2.1 pantsbuild/binaries#85

Closed

cosmicexplorer mentioned this pull request Oct 26, 2018

Compiler option sets for Native targets #6665

Merged

Eric-Arellano closed this May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

we can now depend on non-header-only libraries from conan by building them ourselves #6486

we can now depend on non-header-only libraries from conan by building them ourselves #6486

cosmicexplorer commented Sep 11, 2018

CMLivingston Sep 11, 2018

cosmicexplorer Sep 11, 2018

cosmicexplorer Sep 11, 2018

CMLivingston Sep 11, 2018

cosmicexplorer Sep 11, 2018

cosmicexplorer Sep 11, 2018

cosmicexplorer Sep 11, 2018 •

edited

Loading

cosmicexplorer Sep 11, 2018

cosmicexplorer Sep 11, 2018

CMLivingston Sep 13, 2018

CMLivingston left a comment

cosmicexplorer commented Sep 11, 2018

stuhood left a comment •

edited

Loading

stuhood Sep 12, 2018

cosmicexplorer Sep 13, 2018

cosmicexplorer commented Sep 13, 2018

cosmicexplorer commented Oct 14, 2018

cosmicexplorer commented Oct 18, 2018

cosmicexplorer commented Oct 19, 2018 •

edited

Loading

cosmicexplorer commented Oct 22, 2018 •

edited

Loading

Eric-Arellano commented May 18, 2020


		return cls(args, external_lib_dirs=lib_dirs, external_lib_names=lib_names, *kwargs)
		return cls(

we can now depend on non-header-only libraries from conan by building them ourselves #6486

we can now depend on non-header-only libraries from conan by building them ourselves #6486

Conversation

cosmicexplorer commented Sep 11, 2018

Problem

Solution

Result

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CMLivingston left a comment

Choose a reason for hiding this comment

cosmicexplorer commented Sep 11, 2018

stuhood left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cosmicexplorer commented Sep 13, 2018

cosmicexplorer commented Oct 14, 2018

cosmicexplorer commented Oct 18, 2018

cosmicexplorer commented Oct 19, 2018 • edited Loading

cosmicexplorer commented Oct 22, 2018 • edited Loading

Eric-Arellano commented May 18, 2020

cosmicexplorer Sep 11, 2018 •

edited

Loading

stuhood left a comment •

edited

Loading

cosmicexplorer commented Oct 19, 2018 •

edited

Loading

cosmicexplorer commented Oct 22, 2018 •

edited

Loading