fix variable inheritance

metatypedev · Aug 28, 2024 · 22cb91c · 22cb91c
1 parent db40575
commit 22cb91c
Show file tree

Hide file tree

Showing 4 changed files with 162 additions and 79 deletions.
diff --git a/examples/kitchen/ghjk.ts b/examples/kitchen/ghjk.ts
@@ -107,7 +107,8 @@ env("python")
     ports.cpy_bs({ version: "3.8.18", releaseTag: "20240224" }),
     ports.tar(),
     ports.zstd(),
-  );
+  )
+  .mixin(pyEnv());
 
 env("dev")
   // we can inherit from many envs
@@ -123,4 +124,4 @@ env("dev")
   }));
 
 env("venv")
-  .mixin(pyEnv({ install: { version: "3.8.18", releaseTag: "20240224" } }));
+  .inherit(["python"]);
diff --git a/files/MergedEnvs.ts b/files/MergedEnvs.ts
@@ -0,0 +1,123 @@
+import { deep_eql } from "../deps/common.ts";
+import getLogger from "../utils/logger.ts";
+
+const logger = getLogger(import.meta);
+
+type Var =
+  | { kind: "static"; value: string; parentName: string }
+  | { kind: "dynamic"; taskId: string; parentName: string };
+
+export class ParentEnvs {
+  #childName: string;
+  #vars: Map<string, Var> = new Map();
+  #installs: Set<string> = new Set();
+  #onEnterHooks: string[] = [];
+  #onExitHooks: string[] = [];
+  #allowedBuildDeps: Map<string, [string, string]> = new Map();
+
+  constructor(childName: string) {
+    this.#childName = childName;
+  }
+
+  addHooks(onEnterHooks: string[], onExitHooks: string[]) {
+    this.#onEnterHooks.push(...onEnterHooks);
+    this.#onExitHooks.push(...onExitHooks);
+  }
+
+  mergeVars(parentName: string, vars: Record<string, string>) {
+    for (const [key, value] of Object.entries(vars)) {
+      const conflict = this.#vars.get(key);
+
+      if (
+        conflict && !(conflict.kind === "static" && conflict.value === value)
+      ) {
+        logger.warn(
+          "environment variable conflict on multiple env inheritance, parent 2 was chosen",
+          {
+            child: this.#childName,
+            parent1: conflict.parentName,
+            parent2: parentName,
+            variable: key,
+          },
+        );
+      }
+
+      this.#vars.set(key, { kind: "static", value, parentName });
+    }
+  }
+
+  mergeDynVars(parentName: string, dynVars: Record<string, string>) {
+    for (const [key, taskId] of Object.entries(dynVars)) {
+      const conflict = this.#vars.get(key);
+
+      if (
+        conflict && !(conflict.kind === "dynamic" && conflict.taskId === taskId)
+      ) {
+        logger.warn(
+          "dynamic environment variable conflict on multiple env inheritance, parent 2 was chosen",
+          {
+            child: this.#childName,
+            parent1: conflict.parentName,
+            parent2: parentName,
+            variable: key,
+          },
+        );
+      }
+
+      this.#vars.set(key, { kind: "dynamic", taskId, parentName });
+    }
+  }
+
+  mergeInstalls(
+    parentName: string,
+    installs: Set<string>,
+    allowedBuildDeps: Record<string, string>,
+  ) {
+    this.#installs = this.#installs.union(installs);
+
+    for (const [key, val] of Object.entries(allowedBuildDeps)) {
+      const conflict = this.#allowedBuildDeps.get(key);
+      if (conflict && !deep_eql(val, conflict[0])) {
+        logger.warn(
+          "allowedBuildDeps conflict on multiple env inheritance, parent 2 was chosen",
+          {
+            child: this.#childName,
+            parent1: conflict[1],
+            parent2: parentName,
+            variable: key,
+          },
+        );
+      }
+
+      this.#allowedBuildDeps.set(key, [val, parentName]);
+    }
+  }
+
+  finalize() {
+    const vars: Record<string, string> = {};
+    const dynVars: Record<string, string> = {};
+
+    for (const [key, value] of this.#vars) {
+      if (value.kind === "static") {
+        vars[key] = value.value;
+      } else {
+        dynVars[key] = value.taskId;
+      }
+    }
+
+    return {
+      installSet: {
+        installs: this.#installs,
+        allowedBuildDeps: Object.fromEntries(
+          [...this.#allowedBuildDeps.entries()].map((
+            [key, [val]],
+          ) => [key, val]),
+        ),
+      },
+      onEnterHookTasks: this.#onEnterHooks,
+      onExitHookTasks: this.#onExitHooks,
+      vars,
+      dynVars,
+    };
+  }
+}
diff --git a/files/mod.ts b/files/mod.ts
@@ -47,6 +47,7 @@ import {
 } from "../modules/envs/types.ts";
 import envsValidators from "../modules/envs/types.ts";
 import modulesValidators from "../modules/types.ts";
+import { ParentEnvs } from "./MergedEnvs.ts";
 
 const validators = {
   envVars: zod.record(
@@ -139,6 +140,14 @@ export type DenoTaskDefArgs = TaskDefArgs & {
 
 type TaskDefTyped = DenoTaskDefArgs & { ty: "denoFile@v1" };
 
+export type FinalizedEnvs = {
+  finalized: ReturnType<EnvFinalizer>;
+  installSetId?: string;
+  vars: Record<string, string>;
+  dynVars: Record<string, string>;
+  envHash: string;
+};
+
 export class Ghjkfile {
   #installSets = new Map<
     string,
@@ -149,15 +158,7 @@ export class Ghjkfile {
   #tasks = new Map<string, TaskDefTyped>();
   #bb = new Map<string, unknown>();
   #seenEnvs: Record<string, [EnvBuilder, EnvFinalizer]> = {};
-  #finalizedEnvs: Record<
-    string,
-    {
-      finalized: ReturnType<EnvFinalizer>;
-      installSetId?: string;
-      vars: Record<string, string>;
-      envHash: string;
-    }
-  > = {};
+  #finalizedEnvs: Record<string, FinalizedEnvs> = {};
 
   /* dump() {
     return {
@@ -390,6 +391,7 @@ export class Ghjkfile {
       };
       return config;
     } catch (cause) {
+      logger.error(`error constructing config for serialization`, { cause });
       throw new Error(`error constructing config for serialization`, { cause });
     }
   }
@@ -414,75 +416,27 @@ export class Ghjkfile {
   }
 
   #mergeEnvs(keys: string[], childName: string) {
-    const mergedVars = {} as Record<string, [string, string] | undefined>;
-    let mergedInstalls = new Set<string>();
-    const mergedOnEnterHooks = [];
-    const mergedOnExitHooks = [];
-    const mergedAllowedBuildDeps = {} as Record<
-      string,
-      [string, string] | undefined
-    >;
+    const parentEnvs = new ParentEnvs(childName);
     for (const parentName of keys) {
-      const { vars, installSetId, finalized } = this.#finalizedEnvs[parentName];
-      mergedOnEnterHooks.push(...finalized.onEnterHookTasks);
-      mergedOnExitHooks.push(...finalized.onExitHookTasks);
-      for (const [key, val] of Object.entries(vars)) {
-        const conflict = mergedVars[key];
-        // if parents share a parent themselves, they will have
-        // the same item so it's not exactly a conflict
-        if (conflict && val !== conflict[0]) {
-          logger.warn(
-            "environment variable conflict on multiple env inheritance, parent2 was chosen",
-            {
-              child: childName,
-              parent1: conflict[1],
-              parent2: parentName,
-              variable: key,
-            },
-          );
-        }
-        mergedVars[key] = [val, parentName];
-      }
-      if (!installSetId) {
-        continue;
-      }
-      const set = this.#installSets.get(installSetId)!;
-      mergedInstalls = mergedInstalls.union(set.installs);
-      for (
-        const [key, val] of Object.entries(set.allowedBuildDeps)
-      ) {
-        const conflict = mergedAllowedBuildDeps[key];
-        if (conflict && !deep_eql(val, conflict[0])) {
-          logger.warn(
-            "allowedBuildDeps conflict on multiple env inheritance, parent2 was chosen",
-            {
-              child: childName,
-              parent1: conflict[1],
-              parent2: parentName,
-              depPort: key,
-            },
-          );
-        }
-        mergedAllowedBuildDeps[key] = [val, parentName];
+      const { installSetId, vars, dynVars, finalized } =
+        this.#finalizedEnvs[parentName];
+      parentEnvs.addHooks(
+        finalized.onEnterHookTasks,
+        finalized.onExitHookTasks,
+      );
+      parentEnvs.mergeVars(parentName, vars);
+      parentEnvs.mergeDynVars(parentName, dynVars);
+      if (installSetId) {
+        const set = this.#installSets.get(installSetId)!;
+        parentEnvs.mergeInstalls(
+          parentName,
+          set.installs,
+          set.allowedBuildDeps,
+        );
       }
     }
-    const outInstallSet = {
-      installs: mergedInstalls,
-      allowedBuildDeps: Object.fromEntries(
-        Object.entries(mergedAllowedBuildDeps).map((
-          [key, val],
-        ) => [key, val![0]]),
-      ),
-    };
-    const outVars = Object.fromEntries(
-      Object.entries(mergedVars).map(([key, val]) => [key, val![0]]),
-    );
-    return {
-      installSet: outInstallSet,
-      onEnterHookTasks: mergedOnEnterHooks,
-      onExitHookTasks: mergedOnExitHooks,
-      vars: outVars,
-    };
+
+    return parentEnvs.finalize();
   }
 
   #resolveEnvBases(
@@ -592,6 +546,10 @@ export class Ghjkfile {
         ...base.vars,
         ...final.vars,
       };
+      const finalDynVars = {
+        ...base.dynVars,
+        ...final.dynVars,
+      };
 
       let finalInstallSetId: string | undefined;
       {
@@ -675,7 +633,7 @@ export class Ghjkfile {
             const prov: WellKnownProvision = { ty: "posix.envVar", key, val };
             return prov;
           }),
-          ...Object.entries(final.dynVars).map((
+          ...Object.entries(finalDynVars).map((
             [key, val],
           ) => {
             const prov = { ty: "posix.envVarDyn", key, taskKey: val };
@@ -704,6 +662,7 @@ export class Ghjkfile {
       this.#finalizedEnvs[final.key] = {
         installSetId: finalInstallSetId,
         vars: finalVars,
+        dynVars: finalDynVars,
         finalized: final,
         envHash,
       };

diff --git a/std/py.ts b/std/py.ts
@@ -25,7 +25,7 @@ export function pyEnv(
     }
     if (create) {
       builder.onEnter(ghjk.task({
-        name: "activate-py-venv",
+        name: "create-py-venv",
         fn: async ($, { workingDir }) => {
           const venvDir = $.path(workingDir).join(dir);
           if (!(await venvDir.exists())) {