fanquake
commented at 7:24 pm on April 2, 2022:
member
The Boost 1.81.0 tarball is ~118mb, and expands to much larger than that, however we end up with ~150mb of headers copied into the /include/boost dir in depends. This is a lot by itself, and even more when it’s 170mb * 9 (HOSTS for a guix build).
With the changes in this PR, we end up with ~50mb of Boost headers in depends, which with some creative patching/pruning, could be trimmed even further. i.e sometimes you end up pulling in an entire boost module, because of a single include in another header we use, but in code that we don’t actually need. In other cases there are deprecated headers which are still being used, which could be removed if the modules we care about stopped using them. I will open some PRs upstream to try and improve that situation, ie: https://github.com/boostorg/multi_index/pull/57.
fanquake added the label
Brainstorming
on Apr 2, 2022
fanquake added the label
Build system
on Apr 2, 2022
hebasto
commented at 5:28 am on April 3, 2022:
member
Concept ACK.
DrahtBot
commented at 6:44 am on April 3, 2022:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviews
See the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
No conflicts as of last run.
DrahtBot added the label
Needs rebase
on Apr 4, 2022
laanwj
commented at 1:53 pm on April 4, 2022:
member
Concept ACK.
I’d slightly prefer a full list of accepted headers in a separate file, instead of a list of commands in the makefile having -r directives which could come to include more files if boost changes.
Or is this very long or unwieldy? 33mb of headers still sounds insane to me.
fanquake force-pushed
on Apr 5, 2022
fanquake
commented at 2:07 pm on April 5, 2022:
member
I’d slightly prefer a full list of accepted headers in a separate file, instead of a list of commands in the makefile having -r directives which could come to include more files if boost changes.
Or is this very long or unwieldy? 33mb of headers still sounds insane to me.
There doesn’t seem to be a super straight forward approach to doing this, hence why I opened it up for some brainstorming.
One alternative is instead of trying to copy what we need, we just prune what we know we definitely don’t need. I played around with this, and got the headers down to around 70mb, which is still much better than 170. This approach is also simpler code-wise, and probably easier to maintain across different HOSTS. There is still the potential of Boost introducing more cross-module dependency going forward, but I think the risk of that being a problem is low.
laanwj
commented at 2:10 pm on April 5, 2022:
member
One alternative is instead of trying to copy what we need, we just prune what we know we definitely don’t need.
I definitely like the white-list approach is better. It gives an exact overview of what we need. And might, at some point, when whittled down enough, include in the repository.
Agree the other way around is bound to be easier.
There is still the potential of Boost introducing more cross-module dependency going forward
Yes, whatever you do, this is something new that needs to be maintained when the boost version in depends changes.
DrahtBot removed the label
Needs rebase
on Apr 5, 2022
DrahtBot added the label
Needs rebase
on Apr 6, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
DrahtBot removed the label
Needs rebase
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 8, 2022
fanquake force-pushed
on Apr 9, 2022
fanquake
commented at 7:17 pm on April 9, 2022:
member
Sorry for all the force push noise, this now (CI passing) seems to be in a working state for all HOSTS. Boost Process is a mess, and is the primary reason for dragging in a huge amount of extra crap that we certainly don’t otherwise use; i.e filesystem, system, asio, fusion, winapi + more.
I’ve opened a few more minor PR’s upstream to remove some deprecated header usage from Boost modules. Will run a Guix build shortly.
jarolrod
commented at 0:02 am on April 11, 2022:
member
fanquake renamed this:
[POC] build: prune Boost headers in depends
build: prune Boost headers in depends
on Apr 26, 2022
fanquake
commented at 9:06 am on April 26, 2022:
member
Rebased past #22953. Updated the PR description. Might split out one other related change.
fanquake force-pushed
on May 19, 2022
fanquake force-pushed
on Jun 1, 2022
fanquake force-pushed
on Jun 10, 2022
fanquake force-pushed
on Jul 5, 2022
fanquake force-pushed
on Jul 17, 2022
fanquake force-pushed
on Aug 1, 2022
fanquake force-pushed
on Aug 13, 2022
fanquake
commented at 1:34 pm on August 13, 2022:
member
Rebased on master, #25803, and a commit from 25696 that updates Boost to 1.80.0. Also updated to remove boost/detail/winapi, which is a duplicate of boost/winapi, and no-longer needed.
fanquake force-pushed
on Aug 16, 2022
fanquake force-pushed
on Aug 23, 2022
fanquake referenced this in commit
3c537f1cc8
on Sep 21, 2022
fanquake force-pushed
on Sep 21, 2022
fanquake
commented at 12:33 pm on September 21, 2022:
member
Split (and extended) part of this out into #26148 after discussion with @theuni. Also now rebased for Boost 1.80.0, and based on #25465. Please review either of those two PRs first.
fanquake force-pushed
on Sep 21, 2022
fanquake
commented at 3:38 pm on September 21, 2022:
member
Updated to include the BOOST_ASIO_STANDALONE commit, and drop some additional dirs from depends, including filesystem.
sidhujag referenced this in commit
a7834914f5
on Sep 23, 2022
fanquake force-pushed
on Nov 30, 2022
fanquake force-pushed
on Dec 8, 2022
DrahtBot added the label
Needs rebase
on Jan 13, 2023
fanquake force-pushed
on Jan 13, 2023
DrahtBot removed the label
Needs rebase
on Jan 13, 2023
fanquake force-pushed
on Feb 2, 2023
fanquake
commented at 12:01 pm on February 2, 2023:
member
How painful is it to update this when boost changes?
fanquake
commented at 2:45 pm on February 2, 2023:
It’s not too awful, and should mostly be removing headers (there are already a few we can drop in the next Boost version). Note that the removal of the winapi dir here is because boost ships it twice (one in boost/winapi and another copy in boost/detail/winapi). If we can (hopefully) get rid of external signer there are a large amount of headers we could drop from here.
hebasto
commented at 2:41 pm on February 2, 2023:
member
c08c0a7524f4f821a0caa977e3e7324727e475cc
Changes look correct, but I’m also curios about maintainability…
theuni
commented at 8:46 pm on February 2, 2023:
member
It’s really nice to have a list of exactly what headers we need, nice work.
How painful is it to update this when boost changes?
I’m nervous about this too. Is it possible to group the copies somewhat? Like “these are required for external signer”? Or are they so intertwined that it doesn’t make sense to try?
I’m also a little nervous about the procedure. When updating to a new boost version, it’s possible that we accidentally forget to ship a new header and it gets picked up from a different version on the local filesystem instead, no? I guess we’d notice that quickly enough, but I don’t love the trial/error requirement for updates.
fanquake
commented at 4:35 pm on February 5, 2023:
member
Like “these are required for external signer”? Or are they so intertwined that it doesn’t make sense to try?
I think it’s a bit too much of a mess. I had considered splitting up our depends boost package into multiple, and having options to turn various bits on and off, but, the overlap in headers, and complication in handling that probably isn’t worth it.
As mentioned above, I’d much rather us remove these nice-to-have, but no-one really uses (not 0, but in terms of instances of bitcoind, the % of users is close enough to 0) features, which are a blocker for removing/integrating the bare-minium of Boost we need (with a signals2 replacement this isn’t too far-fetched).
Dependencies like Boost Process remain fairly poorly maintained, in a adhoc fasion, by a single person upstream (very similar to upnpc and libnatpmp), and I think it continues to makes less and less sense for these kinds of dependencies to be a part of this project.
jarolrod
commented at 4:42 pm on February 5, 2023:
member
I use external signer
the % of users is close enough to 0
There is no evidence for such a claim, aside from an issue wasn’t detected by windows users
fanquake force-pushed
on Feb 14, 2023
fanquake
commented at 3:24 pm on February 14, 2023:
member
Dropped one additional unused header.
fanquake force-pushed
on Mar 3, 2023
DrahtBot added the label
Needs rebase
on Mar 9, 2023
fanquake force-pushed
on Mar 9, 2023
DrahtBot removed the label
Needs rebase
on Mar 9, 2023
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-12-21 15:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me