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

Basic vcpkg. #12859

Closed
wants to merge 6 commits into from
Closed

Basic vcpkg. #12859

wants to merge 6 commits into from

Conversation

aarlt
Copy link
Member

@aarlt aarlt commented Mar 25, 2022

This is just a test to integrate vcpkg in our build system. The main advantage would be that we don't rely anymore on local system libraries, e.g. boost & z3.
For z3 I needed to change the port file a bit. I just removed the python2 build dependency (See aarlt/vcpkg@c1dc3a6). It looks like that this is not needed for our stuff. I guess its only used to build the python library for z3, but I did not check that. So far vcpkg seem to be compatible with our stuff. I only needed to update some stuff related to jsoncpp and Boost::Test. I also needed to rename MSIZE to _MSIZE, because (at least for macOS) a global macro gets defined by that name #define MSIZE (1 << MSIZESHIFT) /* size of an mbuf */. I also removed support for CVC4 ;)

However, I added vcpkg as a submodule, that means you need to run git submodule update --init --recursive to get its content. I guess that our CI will just fail for now, because we are not initialising the submodule yet. But it would be great if anyone could just try this. So far I only tested it on macOS.

Let's discuss in this PR whether we would like to use vcpkg.

Build instructions:

$ git clone [email protected]:ethereum/solidity.git
$ cd solidity
$ git checkout use_vcpkg
$ git submodule update --init --recursive
$ mkdir build
$ cd build
$ cmake ..
$ make

@leonardoalt
Copy link
Member

Are we sure we want to add a submodule just like that?

@cameel
Copy link
Member

cameel commented Apr 4, 2022

Yeah, I think the submodule is not the best idea if it's just for something that should be completely optional like a package manager.

@aarlt
Copy link
Member Author

aarlt commented Apr 4, 2022

Are we sure we want to add a submodule just like that?

Nope, I guess we're not sure whether we should add this as a submodule. We could also just add only the vcpkg stuff that we really need and just add those as part of our repository, just in a normal sub-directory. (e.g. we don't need to add packages that we don't need).

Yeah, I think the submodule is not the best idea if it's just for something that should be completely optional like a package manager.

Not sure whether we can do this completely optional, but yes, theoretically it should be possible.

@leonardoalt
Copy link
Member

But why do we need vcpkg in our system at all?

@cameel
Copy link
Member

cameel commented Apr 4, 2022

I mean, some people were opposed to adding a package manager even as an optional thing. If it's mandatory, I don't really see this getting merged.

The way I see this, a package manager might make sense as a way to install some packages in a way that cmake can find them. But it must also be possible to install them like you do now, without touching vcpkg at all.

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2022

Yeah - if there's general consensus apart from me that it's a good thing, I'll let myself be outvoted on this, but I'm personally enthusiastically against any kind of package manager in the build system, optional or otherwise :-).
On the other hand I'm also against our use of ExternalProject_Add and would appreciate moving towards plain cmake search for dependencies and not have it be the concern of the build system at all to take special care of or even actively download, build and install dependencies...

@aarlt
Copy link
Member Author

aarlt commented Apr 4, 2022

But why do we need vcpkg in our system at all?

It's just a way to make sure that anyone can just build solidity without the need of installing system libraries manually (e.g. boost & z3). We would have these components under our control. But for sure where are other ways available to achieve this. We could do this by our own, or use another package manager like conan. I just thought that it will be easier, if we would use vcpkg (we could also use conan, if preferred).

I mean, some people were opposed to adding a package manage even as an optional thing. If it's mandatory, I don't really see this getting merged.

For me it would be ok, because vcpkg can be nicely integrated with cmake. E.g. there is no need to install vcpkg manually, everything is self-contained. It's integration is totally transparent, that's why I see not a big problem if vcpkg needs to be used always - but you're right, it might make sense to also support a build without using vcpkg at all. But I'm not that sure whether this would really make sense.

The way I see this, a package manager might make sense as a way to install some packages in a way that cmake can find them.

Yes, I see it exactly like this. It make mainly sense for installing & managing some packages that cmake can find (and build) them.

But it must also be possible to install them like you do now, without touching vcpkg at all.

If the usage of vcpkg is totally transparent for the user I don't see a big problem with this. The user will probably not even notice that vcpkg is used at all.

@cameel
Copy link
Member

cameel commented Apr 4, 2022

The problem is that there's more than one package manager. There are valid reasons for wanting a different one or even not wanting any at all (what about distro packaging for example?). People building from source are usually not the ones who prioritize the convenience anyway (they'd use a binary otherwise) and may have some special requirements. I fully support the idea of having a package manager as an option for those who want it, but I'm also strongly against it being mandatory.

@leonardoalt
Copy link
Member

Yea I completely agree with @cameel

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2022

If the usage of vcpkg is totally transparent for the user I don't see a big problem with this. The user will probably not even notice that vcpkg is used at all.

That's a decisively bad thing. The proper "user" to think of when talking about a build system is not an end-user, but a package maintainer and any kind of transparent hidden magic pulling in dependencies is a huge pain for any package manager.

The advantage of not having to manually install one or two dependencies for somebody who wants to build from source - which is something this person has to do only once and which is always easy, if we support current versions of dependencies in their standard configuration - pales in comparison of having package maintainers who deal with a lot of packages at once for each of them by customly inspecting which crazy package manager settings the repo chooses to impose - and that each time they build the package, since they'd need to verify that this hasn't changed.

This applies to any software whatsoever and arguably applies less so in our specific case, since the use of solidity is often via frameworks and targets specific and not the latest version, etc. - but I think it's still something every repository should adhere to in general :-).

That being my highly opinionated and extreme take on all this ;-). But as I said, if you all think this is a good thing, I'll not stand in the way, even though I think it's misguided personally ;-).

EDIT: seeing @cameel's reply just now: yeah... what I just said is less of an issue, if the package manager is optional. At least if it is optional and disabled by default, but can be enabled e.g. by a simple cmake option. I still don't like that at all, because I generally think package managers (apart from distribution package managers) should not exist and only complicate things in the bigger picture, but I wouldn't oppose that as strongly :-).

@aarlt
Copy link
Member Author

aarlt commented Apr 4, 2022

Yeah - if there's general consensus apart from me that it's a good thing, I'll let myself be outvoted on this, but I'm personally enthusiastically against any kind of package manager in the build system, optional or otherwise :-).

Why? I see the dependency management as an integral part of the build system, where a package manager is exactly made for this. Or is it like that you see it more on a different level? E.g. everything should be done with the system package manager, where cmake will just try to use those?

On the other hand I'm also against our use of ExternalProject_Add and would appreciate moving towards plain cmake search for dependencies and not have it be the concern of the build system at all to take special care of or even actively download, build and install dependencies...

I think there is always a trade-off. Basically I don't like to read documentation or ask anyone to find the exact version of a dependency that is needed, to install it manually. Of course we could also write some scripts that will install those system libraries, but we will always have version differences between different distros and/or development environments in general. Thats why I think it would be useful to just use a package manager within the build system. With this we exactly know what e.g. boost/z3 version we need, we can just define a specific version and everybody who is compiling our code, will automatically use exactly this version.

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2022

By the way: the very maintainers of vcpkg argue the same point as I do ;-): https://www.youtube.com/watch?v=sBP17HQAQjk

@leonardoalt
Copy link
Member

I don't think the build is complicated enough to warrant this. We have one required lib dependency, Boost. The other common dependency, z3, also isn't hard to install. Adding one whole new big dependency, because of one other dependency, only doubles the problem instead of fixing it, imo.

@ekpyron
Copy link
Member

ekpyron commented Apr 4, 2022

Why? I see the dependency management as an integral part of the build system, where a package manager is exactly made for this. Or is it like that you see it more on a different level? E.g. everything should be done with the system package manager, where cmake will just try to use those?

The job of the build system is to tell you what dependencies are required and to find them using standard mechanisms. Installing dependencies on the other hand is very much not something the build system should take care of. If anyone likes package managers, they should use them and a repository should not make it hard to be used together with an external package manager. The only good choice for that IMO is a distribution package manager, but you can also argue this for any other - as soon as you hardcode one dependency manager into your repo, you make it harder to use it with any other.

I think there is always a trade-off. Basically I don't like to read documentation or ask anyone to find the exact version of a dependency that is needed, to install it manually. Of course we could also write some scripts that will install those system libraries, but we will always have version differences between different distros and/or development environments in general. Thats why I think it would be useful to just use a package manager within the build system. With this we exactly know what e.g. boost/z3 version we need, we can just define a specific version and everybody who is compiling our code, will automatically use exactly this version.

The build system should tell you Install boost version >=X.Y.Z if it can't find it, and then you install it. You don't even need to read docs for that ever. That's what the build system should do, nothing more, nothing less ;-). If your distribution does not contain a sufficent boost >= X.Y.Z, then you complain to them and flag their package out of date - thereby moving towards updated software everywhere instead of doing the contrary by hardcoding a then generally outdated dependency version in the repo.

@cameel
Copy link
Member

cameel commented Apr 4, 2022

With this we exactly know what e.g. boost/z3 version we need, we can just define a specific version and everybody who is compiling our code, will automatically use exactly this version.

I don't think this is necessarily a good thing. This will lead to the build working only with one exact version of boost or Z3. It will make it even harder to package in an environment where you can't get that exact version.

This is actually related to a recent discussion about package-lock.json in solc-js: ethereum/solc-js#570.

@aarlt
Copy link
Member Author

aarlt commented Apr 4, 2022

Yes, maybe we don't need this. I'm totally open to everything. I just wanted to know how complicated the integration of vcpkg would be - and it seem to be manageable. But of course the big question is, whether we want this. I totally agree that building solidity from source is not difficult or something. I was just curious whether we could improve this somehow (even if it is already easy to do). I saw an advantage of defining the exact versions of external dependencies. With this everyone is using exactly the same version. But maybe this is really not that important for us. I thought it would be good to discuss this.

So far it looks like that we don't want to add a package manager here, because of different reasons (additional complexity & usefulness).

@chriseth chriseth closed this Apr 11, 2022
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.

5 participants