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

Static linking #157

Closed
wants to merge 1 commit into from
Closed

Static linking #157

wants to merge 1 commit into from

Conversation

rainbreak
Copy link
Contributor

@rainbreak rainbreak commented Apr 29, 2016

Relates to ethereum/webthree-umbrella#337.

Work towards having a STATIC_LINKING compile flag.

Split into sub PRs:

@bobsummerwill
Copy link
Contributor

Thanks, @rainbeam!

We're going to need to break this up a bit, and merge it in piece by piece.

Also looks like we'll need some extra platform conditionals around this extra FindX.cmake. For my own http://github.com/doublethinkco/cpp-ethereum-cross cross-builds, it's quite a similar story. Static linkage and cross-builds require you to get ALL THE WAY to the end of your dependency chain.

So let's start with the simplest changes. Please could you explain the reordering of the library and include paths? Is the ordering meaningful there? I am not very experienced with CMake specifically, so I honestly don't know. I am happy to do those reorderings if that is more idiomatic.

Perhaps you could make a new PR with ONLY those reorderings, and we can merge that right away, to reduce the volume of diffs under review.

@bobsummerwill
Copy link
Contributor

The new files should also be independently mergable too, so how about making a second PR which only adds the following?

  • FindSSH2.cmake
  • UseOpenSSL.cmake
  • UseSSH2.cmake
  • UseZLIB.cmake

You could also include the edits to these files too at the same time:

  • UseCURL.cmake
  • UseJsonRpc.cmake

As long as you REMOVE those lines in UseCURL.cmake which chain the dependencies onto SSH2, OpenSSL and SSH2. Then that merge wouldn't break anything.

@bobsummerwill
Copy link
Contributor

When those two PRs are merged, we would just be left with the "chained dependencies" to work through, which would probably need some extra platform and modal conditionals around them.

And we would have to do the conditional-thing for switching mode (ie. something to do an equivalent of ethereum/webthree#163 and https://github.com/ethereum/webthree-umbrella/pull/493/files) but nicely.

@bobsummerwill bobsummerwill self-assigned this Apr 29, 2016
@rainbreak
Copy link
Contributor Author

rainbreak commented Apr 29, 2016

Thanks for the comments. Agree on splitting up.

Edit: progress list moved to top ^

@bobsummerwill
Copy link
Contributor

Hey @rainbeam!

Looks at the diffs in this PR now.

Want to make a third one for everything EXCEPT those 3 lines in UseCURL.cmake?

@rainbreak
Copy link
Contributor Author

OK will do. What does it take to trigger a build on here? I see it has 'triggered' but it doesn't seem to be doing anything.

@bobsummerwill
Copy link
Contributor

bobsummerwill commented Apr 29, 2016

Nothing required. It's just backlogged, I think. See http://ethbuilds.com.

@bobsummerwill
Copy link
Contributor

@rainbreak rainbreak mentioned this pull request Apr 29, 2016
@rainbreak
Copy link
Contributor Author

Added the static linking logic. Uses a flag -DSTATIC_LINKING=TRUE to enable.

7d5b89b comes from #163 and is a requirement for f1e99c1. I can rebase if we decide to do #163 differently.

@bobsummerwill
Copy link
Contributor

That looks great. Why don't you just move the content of these diffs into 163 and squash it, and we can merge that? Then we're just missing the optional dependencies bit.

@rainbreak
Copy link
Contributor Author

@bobsummerwill: once #163 is merged shall I go ahead and make changes like in ethereum/webthree#163 across the whole umbrella?

@bobsummerwill
Copy link
Contributor

All sub-tasks are complete. Should just be a matter of modifying the CMake files for particular executables now, to switch them to the new macro.

@rainbreak rainbreak deleted the static-linking branch April 30, 2016 21:23
@rainbreak rainbreak changed the title [WIP] Static linking Static linking Apr 30, 2016
@bobsummerwill
Copy link
Contributor

^ It's probably only worth doing eth and maybe ethminer, isn't it? What other executables did you have in mind?

Full list of EXEs I know of:

  • eth
  • ethminer
  • ethkey
  • ethvm
  • testeth
  • rlp
  • bench
  • testweb3core
  • solc
  • lllc
  • soltest
  • AlethZero
  • mix-ide

@rainbreak
Copy link
Contributor Author

Will do solc / soltest as well (as that's what started this whole thing off).

@rainbreak
Copy link
Contributor Author

Can see argument for not doing mix / alethzero. The others I'm ambivalent about.

@bobsummerwill
Copy link
Contributor

Yeah ... so I think soltest, solc, lllc would be a good set. ethminer too if that is easy?

But yeah ... diminishing returns for everything else.

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants