Do not retain a name string for generated module maps#789
Conversation
|
So for monorepos, this should be a no-op, but for normal repos it would properly handle external repositories? |
fmeum
changed the title
Module maps generated for a target are named after the target's label. Since the generated module map file is declared by that very target, the name can be derived on demand from the file's owner label, which the File object already retains. This fixes two issues at once: * The name string previously stored in the module map struct (one per target) no longer needs to be retained. This achieves the memory savings of bazelbuild#633, which was rolled back because retaining Labels instead of strings increased memory usage overall - deriving the name on demand retains nothing. * Module map names were generated as 'workspace_name + "//" + package + ":" + name', which Label() fails to parse for targets in external repositories (e.g. 'rules_cc+//pkg:lib' results in 'invalid package name'), breaking header module compilation for all such targets. Module names are now the unambiguous canonical label string, except that main-repository labels drop the leading '@@' ('//pkg:lib'), preserving the traditional module name format for the main repository. Names of external-repository targets keep the '@@' and are thus valid label strings; this changes their module names, but header module compilation never worked for them due to the Label() parsing failure. The new get_module_map_label() helper returns file.owner directly for derived names and restores the '@@' prefix for explicitly named main-repository module maps (such as separate module maps). Special module maps (the crosstool module map and ObjC internal module maps) keep their explicitly provided names.
|
@trybka Yes, that's the plan. @lilygorsheneva Please take a look, this is another attempt at #633. |
|
Looks good to me; I'll benchmark this to make sure it doesn't cause the regression again before we submit |
|
Getting a lot of analysis failures when building protobuf (and therefore everything that depends on it); not sure if it's a widespread issue or if it's proto-specific. I don't think I'll get to the bottom of this one today, i'll look at it again on Monday. |
|
It might be proto-specific, but also protos leverage module compilation so not surprising that it broke there first. Looks like an aspect might be involved, but the error is |
3 participants
Module maps generated for a target are named after the target's label. Since the generated module map file is declared by that very target, the name can be derived on demand from the file's owner label, which the
Fileobject already retains. Thenamefield of the module map struct becomes optional, withNonemeaning "named after the label of the target that generated the module map file", and two central helpers (get_module_map_name,get_module_map_label) derive the string form and theLabelform where needed.This fixes the round-trip bug originally addressed in #633, but avoids the memory regression that caused it to be rolled back in b81b6e4 (because retaining
Labelobjects increased memory usage overall). Deriving the name on demand retains nothing extra as the owner label is already retained by theFileand all name materializations are transient only (such as inmap_eachcallbacks).Names are now the unambiguous canonical label string, except that main repository labels drop the leading
@@(//pkg:lib), keeping the traditional module name format. Special module maps (the crosstool module map, ObjC internal module maps) keep their explicitly provided names.