Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: reduce AttrScope usage #337

Merged
merged 5 commits into from
Jul 24, 2024
Merged

refactor: reduce AttrScope usage #337

merged 5 commits into from
Jul 24, 2024

Conversation

ee7
Copy link
Contributor

@ee7 ee7 commented Jun 13, 2024

The below line should enable slow tests for this PR:

tests:--slow

Issue

#269

Description

Continue (from 9c4fc0b) to prepare for the new con4m, by refactoring so that we get/set everything using the top-level chalk scope with fully-dotted keys. That helps prepare for the new con4m lookup interface, in con4m v2 was:

proc lookup*[T](ctx: RuntimeState, key: string): Option[T] =

where key is fully-dotted. And similarly in libcon4m:

extern c4m_value_t *
c4m_vm_attr_get(c4m_vmthread_t *tstate,
                c4m_str_t      *key,
                bool           *found);

Before this PR we were passing subscopes around, and my understanding was that in the new con4m:

  • There's essentially a dictionary of fully-dotted keys, and therefore there's no longer the equivalent of AttrScope.
  • We want to update the gets/sets to be fully string-based, matching the new design, as started all the way back in 66a5bb4.

Later we can try to:

  • Add more compile-time safety again.
  • Make the gets/sets more readable.

This PR also:

  • Makes getChalkSubsections yield only key names.
  • Replaces getObjectOpt with sectionExists.
  • Makes getObject/getContents style more consistent.

I previously had about 20 more reviewable, self-contained and test-passing commits for this PR, but there were too many tricky conflicts with main, and I'd lost context on this part of the branch. So unfortunately it was best to squash before rebasing this on main, to reduce the time spent and bugs introduced when resolving conflicts.

With this PR:

$ git log -1 --format='%h %s'
ab6ed52 Merge branch 'main' into ee7/reduce-AttrScope
$ git grep --ignore-case --break --heading AttrScope
src/run_management.nim
20:proc getChalkScope*(): AttrScope =
27:    if v.isA(AttrScope):
30:proc sectionExists*(scope: AttrScope, s: string): bool =
41:proc con4mAttrSet*(attrs: AttrScope, fqn: string, value: Box, attrType: Con4mType) =

src/sinks.nim
177:proc getAuthConfigByName*(name: string, attr: AttrScope = AttrScope(nil)): Option[AuthConfig] =

For quick extra sanity checks, bad hacks for trying to find missing dots without parsing:

$ git diff -U0 main | grep '^[+]' | grep -o 'getChalkScope().*&.*)' | grep -v '.' | wc -l
0
$ git diff -U0 main | grep '^[+]' | grep -o 'getChalkScope().*&.*)' | cut -d',' -f2 | sort | uniq
 base & ".attempt_install")
 base & ".get_command_args")
 base & ".get_tool_location")
 base & ".produce_keys")
 content & ".use")
 getOutputConfig() & ".mark_template")
 getOutputConfig() & ".mark_template"))
 getOutputConfig() & ".report_template")
 "keyspec." & k & ".apply_substitutions")
 outconf & ".mark_template")
 outconf & ".report_template")
 "plugin." & plugin.name & ".codec")
 "plugin." & plugin.name & ".ignore")
 "plugin." & plugin.name & ".overrides")
 "plugin." & plugin.name & ".post_chalk_keys")
 "plugin." & plugin.name & ".post_chalk_keys"))
 "plugin." & plugin.name & ".post_run_keys")
 "plugin." & plugin.name & ".post_run_keys"))
 "plugin." & plugin.name & ".pre_chalk_keys")
 "plugin." & plugin.name & ".pre_chalk_keys"))
 "plugin." & plugin.name & ".pre_run_keys")
 "plugin." & plugin.name & ".pre_run_keys"))
 "plugin." & plugin.name & ".priority")
 "plugin." & p.name & ".ignore")
 "plugin." & p.name & ".overrides")
 report & ".report_template")
 report & ".use_when")
 scope & ".directories")
 scope & ".filepaths")
 scope & ".process_names")
 scope & ".strict")
 section & ".codec")
 section & ".conf_as_system")
 section & ".enabled")
 section & "." & k)
 section & "." & k).getOrElse("")
 section & "." & k).getOrElse(@[])
 section & "." & k).getOrElse(0)
 section & "." & k).getOrElse(int64(10 * 1048576))
 section & ".on_write_msg")
 section & ".pinned_cert_file"
 section & ".system")
 spec & ".enabled")
 spec & ".report_template")
 spec & ".sink_configs")
 spec & ".use_when")
 ss) and get[bool](getChalkScope()
 tmpl & ".doc")
 tmpl & ".key." & k & ".use")
 "tool." & info.name & ".stop_on_success")
 tplate & ".key." & k)
 tplate & ".key." & k & ".order")
 val & ".category")
 val & ".file_scope")
 val & ".file_scope.excluded_filepaths")
 val & ".file_scope.excluded_filetypes")
 val & ".file_scope.filepaths")
 val & ".file_scope.filetypes")
 val & ".file_scope.head")
 val & ".file_scope.regex"))
 val & ".host_scope")
 val & ".subcategory")
 v & ".callback")
 v & ".enabled")
 v & ".kind")
 v & ".kind") != int(kt)
 v & ".priority")
 v & ".value")
$ git diff -U0 main | grep -E '^[+][^+]' | grep -v 'getChalkScope' | grep -v '\.'
+                tplate: string): seq[string] =
+proc toJson*(dict: ChalkDict, tplate: string): string =
+                      t: string): string =
+  for k in getChalkSubsections("keyspec"):
+proc registerKeys(templ: string) =
+    for name in getChalkSubsections(section):
+        subscribedKeys[name] = true
+        if k notin hostInfo or
+  for name in getChalkSubsections("custom_report"):
+            override:
+        if k notin hostInfo or
+                     tmpl: string): Rope =
+      for k in getChalkSubsections("mark_template"):
+      for k in getChalkSubsections("report_template"):
+proc filterByTemplate*(dict: ChalkDict, p: string): ChalkDict =
+proc getOutputConfig*(): string =
+template getMarkTemplate*(): string =
+template getReportTemplate*(): string =
+    discard attrLookup(
+      ["key"],
+      ix = 0,
+      op = vlSecDef,
+    )
+    if item notin keys:
+    con4mAttrSet(
+      pack(true),
+      Con4mType(kind: TypeBool),
+    )
+      discard attrLookup(
+        ["key"],
+        ix = 0,
+        op = vlSecDef,
+      )
+      if item notin keys:
+      con4mAttrSet(
+        pack(true),
+        Con4mType(kind: TypeBool),
+      )
+  for k in getChalkSubsections("keyspec"):
+  for k in getChalkSubsections("tool"):
+    # Should have crashed by now if section does not exist :)
+  # tech stack rules
+  tsRules              = initHashSet[string]()
+proc hostHasTechStack(scope: string, proc_names: HashSet[string]): bool =
+    for langName in getChalkSubsections("linguist_language"):
+      languages[ext] = langName
+    for key in getChalkSubsections("tech_stack_rule"):
+  for key in getChalkSubsections("tech_stack_rule"):
+proc setPerChalkReports(tmpl: string) =
+template buildHostReport*(tmpl: string): string =
+  for topic in getChalkSubsections("custom_report"):
+iterator getChalkSubsections*(s: string): string =
+      yield k
+proc sectionExists*(scope: AttrScope, s: string): bool =
+  if not sectionExists(attrRoot, section):
+    error(section & " is referenced but its missing in the config")
+    error(section & " is misconfigured: " & getCurrentExceptionMsg())

@ee7 ee7 marked this pull request as ready for review June 14, 2024 05:56
@ee7 ee7 requested a review from viega as a code owner June 14, 2024 05:56
@ee7 ee7 force-pushed the ee7/avoid-c4autoconf-part6 branch 2 times, most recently from 9c54d7b to b45dafd Compare June 18, 2024 18:58
Base automatically changed from ee7/avoid-c4autoconf-part6 to main June 19, 2024 15:18
Continue to prepare for the new con4m, by refactoring so that we get/set
everything using the top-level chalk scope with fully-dotted keys. That
helps prepare for the new con4m lookup interface, which in con4m v2 is:

    proc lookup*[T](ctx: RuntimeState, key: string): Option[T] =

where `key` is fully-dotted.

Previously we were passing subscopes around, but `AttrScope` doesn't
exist in the new con4m (where there's just one big dictionary of
fully-dotted keys).
@ee7 ee7 requested a review from miki725 June 19, 2024 17:35
Copy link
Contributor

@miki725 miki725 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall looks fine but wonder if there is any AST thing we can use to test correctness?

ee7 added 3 commits July 5, 2024 13:44
A while after I pushed the previous commit, I added `tests:--slow` to
the PR body and then manually triggered the testing job. But that didn't
make the slow tests run on the PR. Try again with a commit that was
created after the PR body was edited.
@ee7
Copy link
Contributor Author

ee7 commented Jul 24, 2024

Demonstrated that slow tests still pass here:

============ 178 passed, 1 skipped, 1 warning in 348.75s (0:05:48) =============

wonder if there is any AST thing we can use to test correctness?

It'd be possible. But a good fraction of problems here should be caught by the tests; for anything remaining, let's just try to shake out before the next release. Follow-up PRs will help too. Merging.

@ee7 ee7 merged commit 6ce7679 into main Jul 24, 2024
4 checks passed
@ee7 ee7 deleted the ee7/reduce-AttrScope branch July 24, 2024 16:29
miki725 added a commit that referenced this pull request Jul 30, 2024
)"

This reverts commit 6ce7679.

This commit is potentially dangerous and requires more internal testing
before releasing it as part of the release.
miki725 added a commit that referenced this pull request Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants