Only build objects when supports_start_end_lib is enabled #493
Conversation
keith
requested review from
armandomontanez,
c-mita,
comius,
hvadehra,
pzembrod and
trybka
as code owners
| if supports_start_end_lib and (artifacts_to_build.objects or artifacts_to_build.pic_objects): | ||
| if artifacts_to_build.pic_objects: | ||
| files_builder.extend(artifacts_to_build.pic_objects) | ||
| elif artifacts_to_build.objects: |
There was a problem hiding this comment.
Previously both the pic and non-pic version were built, now only one of the two flavors is requested. There are pros and cons to both approaches, I guess. We should at least leave a comment explaining why this differs between the two feature states.
There was a problem hiding this comment.
I was wondering why that was the case, or in what configurations you would end up with both, but I can play it safe and mirror today's behavior 🤔 , not sure what the ideal is?
There was a problem hiding this comment.
I would say it's safer if there are fewer outputs (assuming we change the type of outputs anyway) as this results in fewer downloads with default BwoB settings and smaller BES messages. Building pic of available and non-pic only as a fallback seems reasonable to me. I wouldn't expect non-pic compilation to fail if pic compilation passes.
| files_builder.extend(artifacts_to_build.objects) | ||
| else: | ||
| if artifacts_to_build.static_library != None: | ||
| files_builder.append(artifacts_to_build.static_library) |
There was a problem hiding this comment.
We could advertise new static_library and pic_static_library output groups so that users can still get these artifacts.
There was a problem hiding this comment.
added that for discussion, seems fine
| files_builder.append(artifacts_to_build.static_library) | ||
| if supports_start_end_lib and (artifacts_to_build.objects or artifacts_to_build.pic_objects): | ||
| if artifacts_to_build.pic_objects: | ||
| files_builder.extend(artifacts_to_build.pic_objects) |
There was a problem hiding this comment.
BES messages can get larger as they no longer announce just a constant number of outputs per cc_library. Just something to keep in mind, but I think that the change is still worth it.
2 participants
Fixes #419