99c145df11c04f80820690fd67e9499f296b4d7d braney Sat Aug 1 12:58:39 2026 -0700 harvesters: resolve #defines per file, not pooled refs #37923 refs #37925 refs #37838 The first nightly run reported a new URL parameter, hggw_term, that had been in the tree since the hgGateway redesign. It was not new; the harvester had been answering the question differently in two checkouts of the same commit. SEARCH_TERM is "hggw_term" in hgGateway and "hgcd_term" in hgChooseDb. The #define table was pooled across the whole tree with setdefault, so the winner was whichever file the filesystem walk reached first, and hgGateway's reads came out as hgcd_term in my working tree and hggw_term in a fresh clone. For a nightly cron that means mail whenever a directory listing changes order, which is worse than no cron. So a name a file defines itself now wins, and a name the tree defines inconsistently resolves to {NAME} instead of to a guess. This is the call bdc473369e1 already made for char * constants, for the same reason and with the same tradeoff written up in the CONST_RE comment: an honest {ident} beats a confident wrong answer. Verified by harvesting both trees and diffing. Twelve names leave the URL baseline as a result, the gisaidTable and hgg_ prefix families, which were pooled values from sibling CGIs rather than reads in the file they were attributed to. Recovering those properly means following the #include chain to the header that defines them, which is not done here. The URL catalog carried hgt_tSearch twice, once correctly as the track search variable and once as hgGateway's search term, which was this bug showing up in the curated half. --check did not catch the duplicate because it only looks within a section. hgGateway now has hggw_term and hgChooseDb hgcd_term, both confirmed at their call sites and in their CGI's excludeVars. diff --git src/hg/utils/cartTrackVarCatalog/harvestCartVars.py src/hg/utils/cartTrackVarCatalog/harvestCartVars.py index fc0b674e40b..dac403f0309 100755 --- src/hg/utils/cartTrackVarCatalog/harvestCartVars.py +++ src/hg/utils/cartTrackVarCatalog/harvestCartVars.py @@ -57,59 +57,86 @@ # Everything that draws or configures a track. hgc and hgTables are in the # default list because they read and write per-track vars too, which is easy # to forget. DEFAULT_DIRS = "hg/lib,hg/hgTracks,hg/hgTrackUi,hg/cgilib,hg/hgc,hg/hgTables" # Extra trees mined for #define values only, not scanned for call sites. MACRO_DIRS = ["inc", "lib", "hg/inc"] # --------------------------------------------------------------------------- # macro table # --------------------------------------------------------------------------- def build_macros(dirs): - """Map every #define that resolves to a string literal, chasing aliases.""" + """(name -> literal, names defined inconsistently) over the scanned dirs. + + The second return value exists because a pooled table gives a name that two + files define differently whichever value the directory listing reached + first, so the answer changes between two checkouts of the same commit. + Those names resolve to {NAME} unless the file being scanned defines them + itself, the same call the per-file char * constants make. + """ macro = {} + conflict = set() chains = [] def_re = re.compile( r'^\s*#\s*define\s+([A-Za-z_][A-Za-z0-9_]*)\s+' r'("(?:[^"\\]|\\.)*")\s*(?:/[/*].*)?$') chain_re = re.compile( r'^\s*#\s*define\s+([A-Za-z_][A-Za-z0-9_]*)\s+' r'([A-Za-z_][A-Za-z0-9_]*)\s*(?:/[/*].*)?$') for d in list(dirs) + MACRO_DIRS: p = os.path.join(ROOT, d) if not os.path.isdir(p): continue for fn in os.listdir(p): if not fn.endswith((".h", ".c")): continue for line in open(os.path.join(p, fn), errors="replace"): m = def_re.match(line) if m: - macro.setdefault(m.group(1), m.group(2)[1:-1]) + name, val = m.group(1), m.group(2)[1:-1] + if name in macro and macro[name] != val: + conflict.add(name) + macro.setdefault(name, val) continue m = chain_re.match(line) if m: chains.append((m.group(1), m.group(2))) for _ in range(5): for a, b in chains: if a not in macro and b in macro: macro[a] = macro[b] - return macro + if b in conflict: + conflict.add(a) # alias of an ambiguous name + return macro, conflict + + +# #define NAME "literal" in the file being scanned. Its own definition is the +# one that file means, whatever the rest of the tree says. +LOCAL_DEFINE_RE = re.compile( + r'^[ \t]*#[ \t]*define[ \t]+([A-Za-z_]\w*)[ \t]+' + r'("(?:[^"\\]|\\.)*")[ \t]*(?:/[/*].*)?$', re.M) + + +def local_defines(text): + # CONST_RE next door captures inside the quotes, so strip them here to + # match: localconst holds bare values. + return {m.group(1): m.group(2)[1:-1] + for m in LOCAL_DEFINE_RE.finditer(text)} # --------------------------------------------------------------------------- # C parsing, such as it is # --------------------------------------------------------------------------- CONST_RE = re.compile( r'(?:static\s+)?(?:const\s+)?char\s*\*\s*([A-Za-z_][A-Za-z0-9_]*)\s*=\s*' r'"((?:[^"\\]|\\.)*)"') # Kent style puts function bodies at column 0, so a bare "safef(...)" at # column 0 looks like a function definition. Requiring a return type plus # whitespace (or a star) before the name is what keeps that from matching. FUNCDEF_RE = re.compile( r'^(?:static\s+)?(?:INLINE\s+)?(?:const\s+)?' @@ -146,45 +173,52 @@ depth += 1 elif c in ")]}": depth -= 1 if c == "," and depth == 0: out.append(cur.strip()) cur = "" i += 1 continue cur += c i += 1 if cur.strip(): out.append(cur.strip()) return out -def resolve(arg, localconst, macro): - """Turn an argument into the string it evaluates to, if we can.""" +def resolve(arg, localconst, macro, conflict=None): + """Turn an argument into the string it evaluates to, if we can. + + conflict is the set of names the tree defines inconsistently; without the + file's own definition to go on, those stay {NAME} rather than taking + whichever value was seen first. + """ arg = arg.strip() if re.fullmatch(r'"((?:[^"\\]|\\.)*)"', arg): return arg[1:-1] toks = re.findall(r'"(?:[^"\\]|\\.)*"|[A-Za-z_][A-Za-z0-9_]*', arg) plain = re.sub(r'"(?:[^"\\]|\\.)*"|[A-Za-z_][A-Za-z0-9_]*|\s+', '', arg) if plain == "" and toks: # nothing but literals and identifiers, i.e. C string concatenation vals = [] for t in toks: if t.startswith('"'): vals.append(t[1:-1]) elif t in localconst: vals.append(localconst[t]) + elif conflict and t in conflict: + vals.append("{" + t + "}") elif t in macro: vals.append(macro[t]) else: vals.append("{" + t + "}") return "".join(vals) return "EXPR:" + re.sub(r'\s+', ' ', arg)[:60] def match_close(txt, i): """Index of the paren that closes the one at i.""" depth, j, instr = 0, i, False while j < len(txt): c = txt[j] if instr: if c == "\\": @@ -206,121 +240,126 @@ def enclosing_functions(lines): """Map 1-based line number to the name of the function containing it.""" encl = [None] * (len(lines) + 2) cur = None for idx, line in enumerate(lines): if (line and not line[0].isspace() and not line.startswith(("#", "/", "*", "}", "{"))): m = FUNCDEF_RE.match(line) if m: cur = m.group(1) encl[idx + 1] = cur return encl -def scan_file(fp, rel, macro): +def scan_file(fp, rel, macro, conflict=None): """Return one record per track-scoped cart name found in one file.""" txt = open(fp, errors="replace").read() + # The file's own #defines join its char * constants: both are what this + # file means, whatever the rest of the tree calls the same name. localconst = {m.group(1): m.group(2) for m in CONST_RE.finditer(txt)} + localconst.update(local_defines(txt)) encl = enclosing_functions(txt.split("\n")) starts = [0] for i, ch in enumerate(txt): if ch == "\n": starts.append(i + 1) def lineno(pos): lo, hi = 0, len(starts) - 1 while lo < hi: mid = (lo + hi + 1) // 2 if starts[mid] <= pos: lo = mid else: hi = mid - 1 return lo + 1 recs = [] for m in CTH_RE.finditer(txt): i = m.end() - 1 args = split_args(txt[i+1:match_close(txt, i)]) if len(args) >= 4: ln = lineno(m.start()) - recs.append(dict(var=resolve(args[3], localconst, macro), + recs.append(dict(var=resolve(args[3], localconst, macro, + conflict), file=rel, line=ln, func=encl[ln], how="cth")) for m in FMT_RE.finditer(txt): i = m.end() - 1 args = split_args(txt[i+1:match_close(txt, i)]) ln = lineno(m.start()) fi = None for k, a in enumerate(args): if a.strip().startswith('"'): fi = k break if fi is None: continue - fmt = resolve(args[fi], localconst, macro) + fmt = resolve(args[fi], localconst, macro, conflict) rest = args[fi+1:] mm = re.match(r'^%s([._])(.*)$', fmt or "") if not mm: continue sep, tail = mm.group(1), mm.group(2) if "%" not in tail and tail: recs.append(dict(var=sep+tail, file=rel, line=ln, func=encl[ln], how="fmtlit")) elif tail == "%s" and len(rest) >= 2: # "%s.%s", track, SUFFIX -> the suffix is the SECOND vararg - v = resolve(rest[1], localconst, macro) + v = resolve(rest[1], localconst, macro, conflict) if v and not v.startswith("EXPR:"): recs.append(dict(var=sep+v, file=rel, line=ln, func=encl[ln], how="fmt")) elif tail == "%s.%s" and len(rest) >= 3: - v1 = resolve(rest[1], localconst, macro) - v2 = resolve(rest[2], localconst, macro) + v1 = resolve(rest[1], localconst, macro, conflict) + v2 = resolve(rest[2], localconst, macro, conflict) if not v1.startswith("EXPR:") and not v2.startswith("EXPR:"): recs.append(dict(var=sep+v1+"."+v2, file=rel, line=ln, func=encl[ln], how="fmt3")) return recs # --------------------------------------------------------------------------- # entry point for the catalog next door # --------------------------------------------------------------------------- def harvest(dirs=None, quiet=False): """Scan the tree and return the raw records. cartTrackVarCatalog.py --reconcile imports this, so the scan has one definition rather than one here and a second one written out by hand. """ dirs = dirs or [d.strip() for d in DEFAULT_DIRS.split(",") if d.strip()] - macro = build_macros(dirs) + macro, conflict = build_macros(dirs) files = [] for d in dirs: p = os.path.join(ROOT, d) if not os.path.isdir(p): sys.exit("no such directory: %s" % p) for fn in sorted(os.listdir(p)): if fn.endswith(".c"): files.append(os.path.join(p, fn)) records = [] for fp in files: - records.extend(scan_file(fp, os.path.relpath(fp, ROOT), macro)) + records.extend(scan_file(fp, os.path.relpath(fp, ROOT), macro, + conflict)) if not quiet: print("scanned %d files in %d dirs, %d macros, %d records" % (len(files), len(dirs), len(macro), len(records)), file=sys.stderr) return records def resolved(records): """name -> first file:line, for the names the scan resolved to a literal. An EXPR: or {ident} record marks a name built at run time, which is signal for a person reading the harvester output but cannot be compared against a catalog of literal names, so it is dropped here. """