Option to build Tor with -fPIC (Use libtool for building shared library?)
It seems like it's ideal to use libtool for generating a shared library. Simone made some progress on getting this working here: https://github.com/bassosimone/mkok-onion-event/pull/2/files.
Activity
changed milestone to %Tor: 0.3.5.x-final
added 034-included-20180402 034-roadmap-subtask 034-triage-20180328 035-roadmap-subtask 035-triaged-in-20180711 actualpoints::2 component::core tor/tor milestone::Tor: 0.3.5.x-final owner::nickm parent::25510 priority::medium resolution::implemented reviewer::ahf s8-api severity::normal sponsor::8 status::closed tor-mobile type::enhancement labels
I have this patch set where I basically:
-
modify the build system of tor to make convenience
libtoollibraries as opposed to convenience.aarchvies -
add an extra rule to create from such convenience libraries a single
.la/.aarchive
This simplifies integrating tor because, after this change, there is a single library to embed rather than many. Also such setup seems more robust because the composition of many libraries into a single one is done by Tor build system rather than by integrators.
Trac:
Username: sbs-
-
@nickm: I previously mentioned to you that tests were not passing after this changes, but it turns out this was transient (i.e., I have just built everything again and run tests and it is now okay). Since Wi-Fi was very bad in my hotel room, I am going to assume that that was the cause.
Trac:
Username: sbs 
I think overall this looks like it's fine, but I would like you to do the following:
- Fork the https://github.com/torproject/tor.git repository to your own account.
- Push your patches to a branch in your fork.
That should allow us to see if everything is going well with our Travis CI. Around build-system changes it is probably a very good idea to be sure we don't break anything here.
Trac:
Status: needs_review to needs_revision-
Done here: https://github.com/torproject/tor/pull/35 !
Trac:
Username: sbs
Status: needs_revision to needs_review Thanks for the pull request! It's good to see Travis passing.
I'm not very familiar with libtool, so sorry if I miss anything obvious. This seems to switch from building static libraries to building shared libraries, if I'm reading the rust warning correctly. Is this something we want? It could make the build more fragile in some ways.
Do we know how this will affect Windows builds? The
.$(OBJEXT)vs.lochange makes me nervous.I also made a minor comment on the pull request about verbosity.
-
catalyst: thanks for the comments. I have starting replying to the easiest ones directly in the PR at github (https://github.com/torproject/tor/pull/35#discussion_r178749931) and will address the other comments later today. I think I have a sense of how to reply to all comments but I'd rather read the manuals and double check that I understand correctly.
Trac:
Username: sbs -
Addressing more comments from catalyst:
Do we know how this will affect Windows builds? The .$(OBJEXT) vs .lo change makes me nervous.
What build system is used to build Tor on Windows? I see that there is a
Makefile.nmakeand aMakefile.amand the documentation mentions mingw (but mainly belowcontrib).Probably both
Makefile.amandMakefile.nmakeare used, then:If tor is built using MS tooling and
Makefile.nmake, then this change has no impact.Otherwise, to better understand, I tried to build
libevent(which useslibtool) on Windows usingMSYS2andmingw-w64and found that it produced.lofiles. I also searched online and it seems the documentation ofautomake, when specifically talking about files produced with and without libtool, does not mention any special variable for the extension of libtool-produced files; rather it usesfile.lo, while it specifically mentionsfile.$(OBJEXT)[1] [2].[2] https://www.gnu.org/software/automake/manual/html_node/Program-and-Library-Variables.html
Trac:
Username: sbs 
Trac:
Parent: #23684 (moved) to #25510 (moved)
Keywords: 034-triage-20180328 034-included-20180402 deleted, 034-triage-20180328 034-included-20180402 034-roadmap-subtask addedIt's possible to have rust build shared object libraries instead of static, see the "crate_type" argument in our
Cargo.tomls and also this reference page (we'd just have to add "dylib" to the lists, I think, or "cdylib" if it's being linked to C/Java/Swift/etc). We can even tell it to build both, so desktop systems can continue to use static linking and I guess mobile can do dynamic?-
So, I've looked over the codebase and made a couple of changes in a branch called "libtool_hack", including a workaround for the Rust warning issue. (It's only a plausible legitimate workaround if Rust builds with -fPIC though -- does it?)
Here are the open issues I want to check, if we can:
- Does this work on windows? (Only the Makefile.am build is supported)
- Does "make distcheck" still work? (It does without Rust. But with Rust, "make distcheck" appears to be broken in master already without this patch. Fixing.)
- How horrible is the above Rust hack?
One other issue here is that since none of the library are installed, no real shared libraries are actually built. (You can verify this yourself by noticing that no .so files are created anywhere.) I'm thinking that this is intentional, but I'm
-
Replying to nickm:
- Does "make distcheck" still work? (It does without Rust. But with Rust, "make distcheck" appears to be broken in master already without this patch. Fixing.)
Did that part in #25732 (moved)
Replying to nickm:
So, I've looked over the codebase and made a couple of changes in a branch called "libtool_hack", including a workaround for the Rust warning issue. (It's only a plausible legitimate workaround if Rust builds with -fPIC though -- does it?)
yes. it is highly unlikely that they will stop, due to the numerous benefits of PIC and negligible costs, but if so, there will be loud build time errors and we can figure it out then. we can safely assume that there will always be a way to turn PIC on, since otherwise it would be impossible to make even pure-Rust shared libraries.
- How horrible is the above Rust hack?
personally I think we should just ignore it. this hack will probably cause more problems than it solves. I believe the warning only applies to obsolete Unix systems that Rust does not and will never support anyways. as I said above, if there is any breakage it will be loud and at build time anyways.
-
Regarding linking Rust generated code and
-fPICcode, there is this additional data point [edit: in addition to what Hello71 already mentioned]. The rust developer themselves acknowledge that they use-fPICfor static libraries on github, explain why that is the default, and how to disable-fPIC[1].(I originally wrote a much longer comment but I would like to do more research before writing libtool-related statements of which I am not 100% sure yet.)
I would perhaps add that, after having worked a little more on this issue, I have a sense that what we actually need for mobile is mainly a single
.aarchive compiled with-fPICcode and a header. At least, this is exactly what we need on Android (for iOS I need to double check whether we can get away without-fPIC, but since Android needs-fPIC, I'd say we need-fPIC). [edit: this paragraph should explain why the diff doesn't make a shared library]In this regard, the main reason to use libtool is that it seems (based on my research, can provide more pointers if needed) the most portable, less hackish way to assemble several
.afiles (through.laor, with the aforementioned warning, using.agenerated by Rust).I'm hoping to dedicate more on this ticket this week and would like to do the following:
-
make a simple iOS and Android apps using the
.alibrary and confirm it works -
make sure it also works on Windows 10 (which is what I have :-)
Regarding the latter point, what is the reference build system? In addition to MSVC, I also use MSYS2 with mingw-w64 on Windows. Would MSYS2 be okay as a build system to test?
Also, is there interest in adding CI using AppVeyor? If so, I can probably try to add an
appveyor.ymlto my fork on github and try building with MSYS2. Then, if it's okay, also that patch can be upstreamed. Would that be of interest?Final question: integration would be further simplified if there was an autoconf/automake rule to install the static library and the header. Would a third diff adding that be accepted? (We have scripts that compile and install all dependencies in a specific location and not having rules to install means writing bash to copy the library and headers -- not a big deal but less smooth.)
[1] https://github.com/rust-lang/rust/issues/27142
Trac:
Username: sbs -
-
To try to answer your questions: we use msys+mingw for our windows builds; we don't support or recommend MSVC. MSYS2 should be fine.
I'd love appveyor support, but there's already a ticket for that, with some work in progress: see #25549 (moved). They aren't done yet; maybe they could use some help.
I'd be fine having an automake rule to install the static library and header, but it would need to be off-by-default, and enabled with --enable-library-install or something.
-
I think it would be interesting to hear from sbs if the current code works for what you need to use it for?
Is there a reason in our .gitignore file that we explicitly list the
*.liband*.lafiles and don't glob them like we do with*.oand*.lo?I think overall the patches in Nick's
libtool_hackbranch looks good. The above questions are the only ones I have so far. 
I started using the branch of nickm called libtool_hack to build a dylib on macOS.
In order to get it to actually build a .dylib on macOS I had to apply this patch: https://github.com/hellais/tor/commit/163d22294df6674411c3bfe6c46868df995a6bb2.
I think it would be very useful to have the work in this branch done and updated to the latest master so that we can better test
tor_api.h(it's easier to write tests using ffi in python that by writing c code).Based on this branch I wrote a simple failing test that makes tor crash when the
tor_run_main()function is called twice. See: https://gist.github.com/hellais/b56043d57eb5be885958e80b3665bfe2I am not sure if that is due to not all the fixes being part of that branch. In the meantime I have started to forward port the changes in this branch to the latest master, but there are quite a few conflicts that I am not entirely sure what is the best approach to follow to resolve them.
-
Hello! My apologies for a late reply and having neglected this ticket for quite some time. I was rightfully prodded by hellais to explain what we exactly need and I understand that any way in which this is simpler is easier for you guys to maintain. Long story short, what we need is the following: a single static library compiled with
-fPICand a header. (I do understand that having a shared library is probably a nice value proposition for people that perhaps wants to link to Tor, but I can also feel that it would complicate the build, so, since we do not need it, I would not advocate for it.) Thanks! :-)Trac:
Username: sbs -
Addition: as far as I can tell [1], there is no portable, non hackish way to make a single static library out of many static libraries. I remember discussing this with
ahfand concluding that probably usinglibtoolwas a good solution. It is a also a bit of a bummer that what we needlibtoolfor is to assemble the static library from the many other libraries produced by the build system. I think I initially considered making another diff that modified the build system to make a single static library, but then I started wondering aboutrustintegration and then suddenly aliboolrule for assembling a single static library from rust static libraries and the manytorfolders was not that bad. Still I am unhappy oflibtoolbecause it makes the build slower (even with--disable-sharedon by default IIRC it was a bit slower).[1] https://stackoverflow.com/a/23621751
Trac:
Username: sbs -
Replying to sbs:
Hello! My apologies for a late reply and having neglected this ticket for quite some time. I was rightfully prodded by hellais to explain what we exactly need and I understand that any way in which this is simpler is easier for you guys to maintain. Long story short, what we need is the following: a single static library compiled with
-fPICand a header. (I do understand that having a shared library is probably a nice value proposition for people that perhaps wants to link to Tor, but I can also feel that it would complicate the build, so, since we do not need it, I would not advocate for it.) Thanks! :-)This is much easier, and we can probably get it done in 0.3.5.
I think we can get away with not using libtool here, since although the ar tricks described on that link are not universal, the easiest ones ought to be something we can hack together.
-
One question: Instead of a single static library, would a target that gives you a list of all the static libraries you need be acceptable?
Right now, you can get a complete list of all the libraries you need to link for tor, in the correct order, by running "make show-libs". It's what we use in a few other places for external things like the clusterfuzz fuzzer that need to link "everything in tor".
That way instead of saying "tor/src/or/libtor_complete.a" in your linking command, you'd say "(cd tor && make show-libs)".
I suggest this instead of a single library because there are issues with Windows here, because Cargo builds its libraries as .lib rather than .a.
-
Instead of a single static library, would a target that gives you a list of all the static libraries you need be acceptable?
Yes, that would be acceptable!
[...] all the libraries you need to link for tor, in the correct order,
This is especially useful because I was not super happy having to keep track of the correct order, especially in the future when switching to new releases; but having an authoritative source for the names and order is great.
Is is correct to say that the reason why different
.alibraries are built in tor, as opposed to a single library, is that you need to apply different compiler flags to different portions of the code base, or is there another reason?[...] because Cargo builds its libraries as .lib rather than .a.
Yeah, using non portable tricks was making me a little nervous because I feared something I was most likely not aware of could break.
Curiosity: does this imply that rust produces binaries compatible with the format of MSVC as opposed to the one of GCC? Does this imply that MinGW is not used anymore for Windows, or is MinGW now able to cope with the format of MSVC?
Thank you!
Trac:
Username: sbs Replying to sbs:
Instead of a single static library, would a target that gives you a list of all the static libraries you need be acceptable?
Yes, that would be acceptable!
[...] all the libraries you need to link for tor, in the correct order,
This is especially useful because I was not super happy having to keep track of the correct order, especially in the future when switching to new releases; but having an authoritative source for the names and order is great.
Is is correct to say that the reason why different
.alibraries are built in tor, as opposed to a single library, is that you need to apply different compiler flags to different portions of the code base, or is there another reason?As I understand it, it's just an easy way to implement "module-like" functionality in autotools, since variable order is a problem (autotools doesn't support anything like the CMake "object library" function).
[...] because Cargo builds its libraries as .lib rather than .a.
Yeah, using non portable tricks was making me a little nervous because I feared something I was most likely not aware of could break.
Curiosity: does this imply that rust produces binaries compatible with the format of MSVC as opposed to the one of GCC? Does this imply that MinGW is not used anymore for Windows, or is MinGW now able to cope with the format of MSVC?
hey, I'm finally qualified to answer a question! Tor still uses mostly mingw to compile on Windows. MSVC is supported-ish but AFAIK no core Tor devs use it and Tor Browser still uses mingw. to use Rust on Windows, you must currently select to download the MSVC or the mingw version (which I assume are incompatible). Tor currently uses whatever you have installed, which now that I'm thinking about it, someone (hopefully not me) should write a configure test to ensure is linkable with the selected C toolchain, but it doesn't do that now.
-
Replying to sbs:
Instead of a single static library, would a target that gives you a list of all the static libraries you need be acceptable?
Yes, that would be acceptable!
[...] all the libraries you need to link for tor, in the correct order,
This is especially useful because I was not super happy having to keep track of the correct order, especially in the future when switching to new releases; but having an authoritative source for the names and order is great.
Is is correct to say that the reason why different
.alibraries are built in tor, as opposed to a single library, is that you need to apply different compiler flags to different portions of the code base, or is there another reason?It's some of what you said here (different compiler flags); somewhat an attempt at enforcing modularity; and somewhat because some of our tools and testing programs only need smaller portions of the Tor codebase.
[...] because Cargo builds its libraries as .lib rather than .a.
Yeah, using non portable tricks was making me a little nervous because I feared something I was most likely not aware of could break.
Curiosity: does this imply that rust produces binaries compatible with the format of MSVC as opposed to the one of GCC? Does this imply that MinGW is not used anymore for Windows, or is MinGW now able to cope with the format of MSVC?
I believe that mingw can link against .lib libraries; I don't know whether this is a full-compability thing, or whether the compatibility is partial.
Per https://trac.torproject.org/projects/tor/wiki/org/teams/NetworkTeam/SupportedPlatforms , msvc is still officially "Unsupported": We'll take clean patches to make it work, but we don't test with it or try hard to keep it working ourselves.
Trac:
Summary: Use libtool for building shared library to Option to build Tor with -fPIC (Use libtool for building shared library?) -
Minimal version for the -fPIC issue is
ticket23846; PR at https://github.com/torproject/tor/pull/321Trac:
Status: accepted to needs_review
