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

External dependencies: json-cpp, fmtlib, ranges-v3, intx, z3, cvc4 #8860

Closed
ekpyron opened this issue May 6, 2020 · 20 comments
Closed

External dependencies: json-cpp, fmtlib, ranges-v3, intx, z3, cvc4 #8860

ekpyron opened this issue May 6, 2020 · 20 comments
Labels
build system 🏗️ closed due inactivity The issue/PR was automatically closed due to inactivity. low effort There is not much implementation work to be done. The task is very easy or tiny. medium impact Default level of impact stale The issue/PR was marked as stale because it has been open for too long.

Comments

@ekpyron
Copy link
Member

ekpyron commented May 6, 2020

Situation:

  • We have been talking about dependencies a while recently, but never came to a decision.
  • It looks like we like to introduce dependencies on fmtlib and ranges-v3 and maybe intx. There also was the idea to replace json-cpp with nlohmann/json.
  • Our repo contains a copy of evmc.
  • For json-cpp we use the cmake ExternalProject_Add mechanism
  • We also depend on boost and optionally on Z3 and CVC4. For those we use plain standard cmake find_package.

Issues with ExternalProject_Add:

  • There is no way to override it and dynamically link against system json-cpp instead.
  • make requires a network connection and starts downloading
  • The latter complicates e.g. PPA scripts.
  • We can't pre-pack dependencies to our CI docker images, increasing build time.

Apart from boost, all the other dependencies except json-cpp (including nlohmann/json as potential json-cpp replacement) are header-only or nearly header-only.

When considering to add new dependencies we could:

  • Reuse the ExternalProject_Add mechanism despite its drawbacks.
  • Use git submodules for the new dependencies (or all dependencies).
  • Instead of submodules use subtrees.
  • Switch everything to plain standard cmake find_package, while maintaining a better ìnstall_deps script to locally install dependencies in the build tree.
  • Combine find_package with other mechanisms as fallback.
@elenadimitrova elenadimitrova added this to New issues in Solidity May 14, 2020
@chriseth chriseth moved this from New issues to Icebox in Solidity Jul 21, 2020
@ekpyron ekpyron changed the title External dependencies: json-cpp, fmtlib, ranges-v3, intx External dependencies: json-cpp, fmtlib, ranges-v3, intx, z3, cvc4 Sep 14, 2020
@axic
Copy link
Member

axic commented Sep 18, 2020

We discussed this countless times on gitter, but apparently no record of that here.

I would say this issue should not block any other issues in https://github.com/ethereum/solidity/projects/40, in particular #3851 and #6900.

For those issues to proceed I think it is fine just to use ExternalProject as we do with jsoncpp.

Independently later we could clean up dependency handling. My personal preference is for git subtrees for reasonably sized projects (and intx, nlohmann-json, ranges-v3 all fit this bill).

@christianparpart
Copy link
Member

My personal preference is for git subtrees for reasonably sized projects (and intx, nlohmann-json, ranges-v3 all fit this bill).

@axic the good thing about submodules is, you do not have to check them out and can still opt for system installed dependencies.

This ticket is also quite old, and I'm now at some ticket where fmtlib actually does make sense (solidity LSP server) for server side logging. I think I'd just use submodules for now until someone complains in the PR, as I think I can always change that at a later point.

p.s.: I favor submodules for the above reasons and can be also easily integrated into our CI.

@axic
Copy link
Member

axic commented Jan 11, 2021

@christianparpart I am not sure I follow how a git submodule could "opt for system installed dependencies" ?

@cameel
Copy link
Member

cameel commented Oct 2, 2021

Here's another idea: why not make our cmake config agnostic to where the libs come from and just support multiple ways of getting them as a separate step before running cmake? This way we could support the C++ package managers that are getting more popular (conan, hunter, vcpkg, etc) as well as manual installation or getting them using system package manger. All without forcing any particular method on the user.

In #12077 we got a request to publish solc on conan and while I think that just having the executable there does not add much, having a conan config in the repo that one could optionally use to get dependencies would be pretty convenient.

My proposal would be this:

  • Have CMake find all libraries via find_package() and not download anything by default.
  • Add conan config so that users can run conan command before running cmake if they choose to. Conan apparently can install packages in such a way that CMake will be able to find them (it can generate CMake config for them or packages can provide it by themselves).
  • Keep the ExternalProject_Add for the current deps as a simple alternative that does not require installing extra tools. Require a special CMake flag to enable it. Alternatively, just use a standalone script for downloading dependencies instead of doing it in CMake config.
  • In the future we could add config for other package managers if we find any of them popular enough to be worth supporting.

@christianparpart
Copy link
Member

@cameel on Windows CI (and me at home) we're already using vcpkg for installing dependencies. vcpkg is btw also available on all 3 major platforms (win, osx, linux). and now they do also support manifest files.

Personally I'd prefer to not support too many package managers (or ways to tell a project how to find them). vcpkg luckily supports find_package(), I'm not sure about conan though. I personally though had a good experience with CPM, too (which is a successor of ExternalProject_Add and FetchContent module).

@cameel
Copy link
Member

cameel commented Oct 6, 2021

Conan does support find_package(). I.e. it can generate the config that lets find_package() find the package or allow the package to provide it by itself.

I have a slight preference towards Conan (vcpkg is very simplistic from what I've read and I'm a bit afraid that it will pull in some heavy MS tooling with it) but I haven't really tried any of them in practice. As long as it's just one of the methods to get packages, I don't have anything against trying it out.

@christianparpart
Copy link
Member

I have a slight preference towards Conan (vcpkg is very simplistic from what I've read and I'm a bit afraid that it will pull in some heavy MS tooling with it) but I haven't really tried any of them in practice. As long as it's just one of the methods to get packages, I don't have anything against trying it out.

I wasn't about to convince us all to hard-depend on vcpkg. It just happens to be used by CI on windows (and my personally on my windows). the solidity project does not have a single line of vcpkg outside of CircleCI files :)

@cameel
Copy link
Member

cameel commented Oct 6, 2021

@chriseth's opinion from the chat:

I think we should not use a package manager because we should keep our depenencies as small as possible

@ekpyron ekpyron moved this from Icebox to Design Backlog in Solidity Nov 5, 2021
@ekpyron ekpyron moved this from Design Backlog to Icebox in Solidity Nov 5, 2021
@ekpyron ekpyron moved this from Icebox to Design Backlog in Solidity Nov 5, 2021
@cameel
Copy link
Member

cameel commented Nov 5, 2021

A bit of discussion has started on this topic after my comment in one of the PRs (#11967 (comment)) so I'm reposting it here to keep things in context (especially my recent proposal to support conan but only as an optional thing):

@cameel

Maybe we should look into adopting some dependency management system to make using libraries less of a pain? I see that for example Conan and Hunter are getting some popularity in the last few years. There's also vcpkg but it seems to be tied to MS stuff and VS in some ways even if it's ultimately meant to be cross-platform. Never used any of them but they look promising.

Tagging @ekpyron becuse he'll likely have an opinion on this :)

@ekpyron

Please don't pull in crazy package managers in the build system, things are bad enough as they are :-).
We should just use submodules for this stuff and be done with it. I know there have been complications with using submodules in the past, but IIUC that was due to developing both ends in parallel, which is much more of a hassle.
Pulling in fixed versions of external code that are occasionally updated with submodules is zero hassle.

@ekpyron

IMHO the only viable alternative to git subtrees or git submodules (of which I'd prefer the latter for this), is just don't do anything at all, document the dependency, have users install it and cmake find it. Everything else is enormously bad and that goes especially for "dependency management systems". That's the worst you can do to anyone who ever wants to package your package and that's who one should be concerned about primarily - regular users rarely build anyways.

@ekpyron

Just to rant a bit more about this :-D: ethereum/solc-bin#101 is exactly what you get from dependency managers - it's so much more convenient for everyone to just use a random dependency manager (which of course doesn't have like an API and interface of its own you first need to know about, if you ever want to override anything), that it's totally worth it to then require people to track down opaque dependency chains with 10 indirections without being able to influence any of it. That's really superior to the extreme hassle of an occasional git submodule update :-).

But yeah, sorry :-D. I really think the seeming convenience of dependency managers is an absolute fallacy - it exactly prevents you from hitting problems in cases in which you definitely should hit problems as in that you should update your stuff to work with proper up-to-date external dependencies. Even submodules are actually bad in that regard, but for some small header-only dependencies they may be excusable...

@ekpyron
Copy link
Member Author

ekpyron commented Nov 5, 2021

Since we're accumulating more and more dependencies that are downloaded at build time, I'm trying to make my case on this once more:

I really think this is an extreme antipattern and we really shouldn't be doing that. There is two proper ways to deal with dependencies: one is not at all, i.e. let users install the dependency and have cmake find it - or if one really wants to avoid that, for small header-only dependencies, it may be acceptable to use git submodules. But I'd actually even prefer keeping those external as well.

Doing any other crazy dependency management - be it hand-written or in one of the big evil "dependency manager"s - may seem like it provides convenience, but it's an absolute fallacy. If your code breaks with a new version of dependencies, the quicker you notice and fix it, the better - insulating oneself from that by pulling fixed versions just leads to ever growing chains of outdated dependencies. Also https://www.youtube.com/watch?v=sBP17HQAQjk comes to my mind: the people for which the build system should work best, i.e. the users of the build system, are, ideally, package maintainers, not end-users. And conversely, every end-user who may want to build from source needs to be able to install boost development packages anyways, so why not the others as well.

@ekpyron
Copy link
Member Author

ekpyron commented Nov 5, 2021

@cameel Ah, yeah, ok, reposting that stuff was maybe the simpler alternative to re-summarizing it here again :-).

@cameel
Copy link
Member

cameel commented Nov 5, 2021

I totally agree regarding npm - that ecosystem is just crazy and the massive dependency trees with a separate tiny lib for every single thing are not something I'd like us to replicate. On the other hand I think that having everything as a submodule/subtree is not great either. I mean, it's acceptable for tiny libs like nlohmann-json or range-v3 but I wouldn't want to manage a dependency on boost or Z3 like this. And people often do have problems installing them.

is just don't do anything at all, document the dependency, have users install it and cmake find it.

What do you think about #8860 (comment)? It would basically give us this but with extra possibilities. I agree that forcing a dependency manager would be bad for packagers so my proposal is now to allow but not require a manager. Basically only as a quick, simple and optional way for something that you can accomplish manually. We'd just have a manager config with dependency versions listed (or whole supported ranges, if possible) on the condition that any manager we support has to be unobtrusive - i.e. just install stuff in such a way that cmake can find it and move out of the way.

We could also keep the option to download the libs in cmake (disabled by default), or, better, just extracted into a separate helper script. You could choose the manager, completely ignore it and use the script or ignore both and do things your way.

@ekpyron
Copy link
Member Author

ekpyron commented Nov 5, 2021

Ok, sounds like we're generally on the same page about this then. (And yes, ideally I'd also want to avoid submodules and I'd even oppose pulling in boost or z3 like that - but as you say: it may be acceptable for the tiny header only libs - but only if we can't agree otherwise)

I wouldn't strongly oppose stuff like adding a package manager config, although I'm also not enthusiastic about it - as far as I'm concerned all these package managers should go back to the hell they came from, so I personally don't want to play nice with any of them :-D - but on a more serious note and with less personal bias: if we add such a config, we need to keep it up to date and test it, etc. pp. which is additional overhead with questionable gain.

I agree and am absolutely strongly for keeping cmake clean and agnostic about any of this (at most it should provide additional information in error messages when it doesn't find something, but that's it - apart from that it should be kept as close to its default behaviour as possible).

And personally, I think we can keep it at just that. If we want to provide some kind of convenience on top of that (even though I think this is generally questionable), then as separate and non-invasive as possible, as you say.
And we'd need to be careful not to end up with something like the script we removed in #12237 (the situation here is somewhat different in that pulling in sources and building is more platform-agnostic than installing using native mechanisms, but where should we draw the line between stuff we support building and which we require to install elsewise? Z3 for example is generally outdated in debian/ubuntu - would our script handle building that? Is that worth the maintenance effort?)

@cameel
Copy link
Member

cameel commented Nov 5, 2021

if we add such a config, we need to keep it up to date and test it, etc. pp. which is additional overhead with questionable gain.

I'm pretty sure some people on the team would use it if it was available. At least I probably would - even if just to try it out and see if I like that workflow. Being able to easily install arbitrary versions only locally for a single project would be a nice benefit. It's not always practical to install dev stuff system-wide. I probably won't need it that much since we generally support bleeding edge versions and on Arch I have just that but it would be even more appealing on a non-rolling-release distro.

I think these managers can't be that bad if they're used for the right job. I.e. to manage project dependencies and not your system.

Z3 for example is generally outdated in debian/ubuntu - would our script handle building that? Is that worth the maintenance effort?

Right. I was actually thinking mostly about the stuff that that we currently download in cmake. Z3, boost and other heavy stuff I'd rather leave up to the user as it is now. If you want it quick and dirty with least possible effort, use the dependency manager. Otherwise you probably have opinions on how you want stuff installed and don't want a script fiddling with your system package manager anyway.

@ekpyron
Copy link
Member Author

ekpyron commented Nov 5, 2021

You don't need to install anything system wide to have cmake find it and you don't need a dependency manager for building things locally for a single project either :-).

@cameel
Copy link
Member

cameel commented Nov 6, 2021

I know but installing them manually is usually a bit of a hassle. You have to install things one by one, figure out how to build them properly, etc. It's a custom process every time.

With a dependency manager the build file for each lib is public so I still have that option but I can also just roll with it. It's also more uniform in how you manage these packages. You can easily rebuild or reinstall them wholesale. Same reason why I prefer using AUR in Arch Linux to just grabbing sources manually and building them.

With how few dependencies we have in Solidity I guess it's pretty manageable without a dependency manager but I still like the automation in principle.

@ekpyron
Copy link
Member Author

ekpyron commented Nov 8, 2021

I guess we could argue about this for all eternity :-).
E.g. it's only a hassle to build your dependency, if it does the same mistake: i.e. overcomplicate its build mechanism for example by using dependency managers ;-). Ideally, every package and every dependency has at the very least sane defaults and building them is thereby canonical and a non-issue. Anything beyond that - which includes custom build files for your own dependencies that are not just default builds and thereby superfluous anyways - only adds to the general issue.
There's also nothing to prevent you from automating the process of building and managing dependencies outside each project for which you want them managed - I still don't see how it's the business of the project itself to define any kind of management like that.

That being said, let me emphasize, that - while I don't like it - I'd still be fine with using any kind of dependency management (be it just the mechanism we use now or something more involved up to a full blown dependency manager - even though my dislike increases with that ;-)), as long as it's not the default.

@cameel cameel added low effort There is not much implementation work to be done. The task is very easy or tiny. medium impact Default level of impact labels Sep 14, 2022
@ekpyron ekpyron moved this from To do to Stub in Solidity Technical Debt Nov 17, 2022
@ekpyron ekpyron moved this from To do to Backlog in Solidity Technical Debt Nov 17, 2022
@github-actions
Copy link

github-actions bot commented Mar 4, 2023

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 4, 2023
@ekpyron ekpyron removed the stale The issue/PR was marked as stale because it has been open for too long. label Mar 5, 2023
@github-actions
Copy link

github-actions bot commented Jun 4, 2023

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Jun 4, 2023
@github-actions
Copy link

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Jun 11, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2023
Solidity Technical Debt automation moved this from Backlog to Done Jun 11, 2023
Solidity automation moved this from Design Backlog to Done Jun 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build system 🏗️ closed due inactivity The issue/PR was automatically closed due to inactivity. low effort There is not much implementation work to be done. The task is very easy or tiny. medium impact Default level of impact stale The issue/PR was marked as stale because it has been open for too long.
Projects
Solidity
  
Done
Development

No branches or pull requests

5 participants