-
Notifications
You must be signed in to change notification settings - Fork 795
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
Vendor FC #1301
Comments
It is an annoying change to have a code exists where a submodule used to. Switching back and forth between the branches is not handled smoothly, so I would want to merge the change just before our next release so that Having to support 0.19.1 without vendored FC and master that is would be a pain. But that is about deployment, and not whether we should or not. |
I am all for it - keep all the critical code in one repo. |
We should figure out what needs to stay in FC and what can go. There are large sections of FC that we don't use. If we vendor FC the dead code should be deleted to speed up compile time and discourage people from using half-baked code. In particular, unused third-party libraries and thin wrappers around Boost or stdlib functions can and should go, since almost any modern C++ project should assume Boost and stdlib are dependencies. "Thicker" wrappers should be considered on a case-by-case basis (for example, |
Part of our reasoning behind this is to have better control over FC changes. Yes, we have a repo for it, but our qa all exists in Steem, not in FC. I think vendoring FC is a requirement to start to removal of old code. So that we have controlled commits and regression testing for every part of the library removed. |
Should we vendor in FC to steem?
We will be making major changes to it that will differentiate it from cryptonomex/fc soon. Doing so protects it with the same branch protections that we have now and allows commits that change both steemd and fc code simultaneously rather than a steemd commit followed by an fc submodule bump and requiring PRs for both.
Thoughts?
The text was updated successfully, but these errors were encountered: