fix(generate-env): resolve path vars before dependent expansion#1612
fix(generate-env): resolve path vars before dependent expansion#1612ST3V1K wants to merge 1 commit into
Conversation
Dependent variables like GOPROXY referenced stale relative paths when kind="path" targets were expanded after mappings were built, producing invalid file:// URLs at build time. Pre-resolve legacy path variables into the substitution mappings before expanding dependents. Fixes hermetoproject#1424 Signed-off-by: Jakub Nekvinda <jnekvind@redhat.com>
There was a problem hiding this comment.
Code Review
This pull request refactors environment variable resolution in hermeto/core/extras/envfile.py by extracting it into a helper function _resolve_env_vars and resolving legacy path variables first to handle dependent expansions correctly. The review feedback suggests optimizing this process by replacing the expensive deepcopy operation on Pydantic models with a shallow .copy(), reusing already resolved path variables to prevent side-effect mutations on the input configuration, and subsequently removing the unused deepcopy import.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| for env_var in build_config.environment_variables: | ||
| if env_var.kind == "path": | ||
| mappings[env_var.name] = deepcopy(env_var).resolve_value(mappings) | ||
|
|
||
| return [ | ||
| (env_var.name, env_var.resolve_value(mappings)) | ||
| for env_var in build_config.environment_variables | ||
| ] |
There was a problem hiding this comment.
Using deepcopy on Pydantic models introduces significant performance overhead because it recursively copies all internal metadata and private attributes.
Additionally, calling resolve_value on the original env_var objects in the second pass mutates the input build_config (since resolve_value prepends ${output_dir}/ to self.value for kind="path" variables). This side-effect can lead to unexpected behavior if the build_config is reused or inspected later.
We can avoid both the deepcopy overhead and the side-effect mutation of the input build_config by:
- Using
.copy()(a shallow copy) on the Pydantic model during the first pass. - Reusing the already resolved path variables from
mappingsin the second pass instead of re-evaluating them on the original objects.
| for env_var in build_config.environment_variables: | |
| if env_var.kind == "path": | |
| mappings[env_var.name] = deepcopy(env_var).resolve_value(mappings) | |
| return [ | |
| (env_var.name, env_var.resolve_value(mappings)) | |
| for env_var in build_config.environment_variables | |
| ] | |
| for env_var in build_config.environment_variables: | |
| if env_var.kind == "path": | |
| mappings[env_var.name] = env_var.copy().resolve_value(mappings) | |
| return [ | |
| ( | |
| env_var.name, | |
| mappings[env_var.name] | |
| if env_var.kind == "path" | |
| else env_var.resolve_value(mappings), | |
| ) | |
| for env_var in build_config.environment_variables | |
| ] |
References
- Focus to maintain secure, readable and reliable code (link)
|
|
||
|
|
||
| def _resolve_env_vars(build_config: BuildConfig, relative_to_path: Path) -> list[tuple[str, str]]: | ||
| """Resolve the environment variables for the given build config and path.""" |
There was a problem hiding this comment.
It would have been nice to have some form of tests to accompany this change, maybe some doctests?
Dependent variables like GOPROXY referenced stale relative paths when kind="path" targets were expanded after mappings were built, producing invalid file:// URLs at build time.
Pre-resolve legacy path variables into the substitution mappings before expanding dependents.
Fixes #1424