We are interested only in:
- a
mpgentool from thenative_libmultiprocesspackage and - a static
libmultiprocess.alibrary from thelibmultiprocesspackage
We are interested only in:
mpgen tool from the native_libmultiprocess package andlibmultiprocess.a library from the libmultiprocess packageConcept ACK. Can we avoid building them too?
Concept ACK. Can we avoid building them too?
Actually, this was my initial intention. Looks like some upstream changes are required. I'm working on them.
Code review ACK 330ff30b6825034a6d096ec468fb837244e44fdd. This seems ok, but a little fragile. My inclination would be to keep the build simpler, and not worry about removing unused functions of dependencies, unless the dependencies are pretty big like boost or qt, not small like libmultiprocess, and there is some notable time or space saving.
But the change looks safe, and as Luke points out it could be expanded to save some build steps too. I think the only change that would be strictly required upstream to do this would be to add COMPONENT arguments to install commands in https://github.com/chaincodelabs/libmultiprocess/blob/master/CMakeLists.txt. New custom install targets or configuration options could be added upstream to make this easier too.
One suggestion is maybe s/Clean up/Trim/ in PR and commit descriptions. It's definitely fair to say this change makes the packages smaller, but I'm not sure it makes them cleaner, since the package definitions are more complicated than before
Only the native `mpgen` tool is required at this point.
The `mpgen` tool is no longer required at this point.
Updated 330ff30b6825034a6d096ec468fb837244e44fdd -> f059620a9b483fdfeac5d3905ba702363e7972d4 (pr25941.01 -> pr25941.02, diff):
One suggestion is maybe s/Clean up/Trim/ in PR and commit descriptions.
This seems ok, but a little fragile. My inclination would be to keep the build simpler, and not worry about removing unused functions of dependencies, unless the dependencies are pretty big like boost or qt, not small like libmultiprocess, and there is some notable time or space saving.
It is mostly not about package sizes, rather about the actual content of the host prefix directory when using those packages.
I believe, keeping there only required staff, i.e., a builder-specific tool and a host-specific library, is clearer and less fragile.
Code review ACK f059620a9b483fdfeac5d3905ba702363e7972d4. Only change since last review is updating commit titles
re: #25941 (comment)
It is mostly not about package sizes, rather about the actual content of the host prefix directory when using those packages.
I am confused about the motivation for this if it is not reducing build times or package sizes. What is the benefit to the user or developer of manually deleting intermediate files that later build steps automatically ignore anyway? I'm in favor of whitelisting dependencies and only building things that are needed. But the approach of blacklisting dependencies complicates the build and doesn't seem reliable.
I believe, keeping there only required staff, i.e., a builder-specific tool and a host-specific library, is clearer and less fragile.
This change makes the build more fragile, not less fragile. Any of the paths this is hardcoding could change. Any of the directories this is recursively deleting could contain files that are needed in the future. It's easy to think of ways the build could break in the future by making this change. It's difficult to think of ways this change could ever prevent some breakage.
Which approach is "clearer" is subjective, but an approach that has simpler builder recipes and fewer build steps and fewer hardcoded install paths seems clearer to me.
I guess I don't think the PR in its current form is an improvement, but I don't think the cost is very high, and wouldn't want disagreement over this to hold up other work.
What is the benefit to the user or developer of manually deleting intermediate files that later build steps automatically ignore anyway?
"automatically ignore" is a build system feature which can be broken.
I'm in favor of whitelisting dependencies and only building things that are needed. But the approach of blacklisting dependencies complicates the build and doesn't seem reliable.
Closing this PR as it seems controversial.
"automatically ignore" is a build system feature which can be broken.
Oh, I completely forgot about that issue. I wish I would have known that issue was motivating this PR, because otherwise I was very confused didn't know how building unneeded files and then adding an extra step to deleting them could be a practical improvement.
I still think that the issue in #25046 was worth knowing about and fixing instead of masking, but I could understand the opposite point of view. If you think this PR is an improvement feel free to reopen. I never opposed it just didn't like the extra build step and didn't understand what was motivating it.