fix: add 'manual' option for //rust/settings/lto
#3120
Conversation
|
|
||
| # The user is handling LTO on their own, don't add any arguments. | ||
| if mode == "manual": |
There was a problem hiding this comment.
if we go with this approach, we should fix the conditions around line 110 and 113
There was a problem hiding this comment.
We can definitely iterate! Are you referring to the elif format == "only_bitcode" condition?
There was a problem hiding this comment.
I was referring to these which are not working today correctly. If we choose this approach we should fix the below conditions according to the new behavior.
if format in ["unspecified", "object_and_bitcode"]:
# Embedding LLVM bitcode in object files is `rustc's` default.
args.extend([])
elif format in ["off", "only_object"]:
args.extend(["embed-bitcode=no"])
There was a problem hiding this comment.
Sweet, more than happy to change these if you can describe how they're broken? If you can share an error message you're getting from a build that would be great!
I ask because I've tested all variants of the LTO setting on a relatively large project that links a few large C libraries (e.g. librdkafka, rocksdb, openssl) and all of the builds succeed?
There was a problem hiding this comment.
I mean that they logically wrong. (enable embed-bitcode=no for off and unspecified, don't set anything for object_and_bitcode format)
Based on what you are proposing it should look something like this in my opinion:
if format == "object_and_bitcode":
# Embedding LLVM bitcode in object files is `rustc's` default.
args.extend([])
elif mode in ["unspecified", "off"] or format == "only_object":
args.extend(["embed-bitcode=no"])
There was a problem hiding this comment.
I see, thanks for pointing this out!
As-is the existing implementation is logically the same as you've written here. I think where the disconnect comes from is how "unspecified" and "off" are still referenced w.r.t. format. In a previous iteration of the original PR they were formats but I realized that was confusing, so I changed _determine_lto_object_format to return one of "only_object", "only_bitcode", or "object_and_bitcode". "unspecified" and "off" already map to "only_object" but making this clearer is a good call!
* remove commented out line * remove 'format' references that are no longer possible
Related to #3119
Adds an option named "manual" to
//rust/settings/ltowhich was introduced in #3104. When set to "manual" we will not automatically set any flags so users with customized setups can configure the relevant flags on their own.FWIW this was an oversight and IMO should have been included in the original PR. Apologies to anyone who stumbled over this or encountered build issues!