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

Step.Run: Add ability to specify LazyPaths as values for env_map #20554

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

der-teufel-programming
Copy link
Contributor

Implements #19482

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

Hmmm this approach has a couple problems. The central point is that the environment map previously allowed being set directly as a field, while now the set of lazy items is separately maintained, leading to inconsistent and surprising state possibly. For example, what takes priority? The lazy items or the other ones? This is a question that should not even be possible to be asked because the data is modeled correctly.

Better to define an Environment data structure that has a tagged union for each value, and then continue to have only one field for the environment data.

This can be done without a breaking change by making it a new field and deprecating the old one.

@@ -592,6 +608,12 @@ fn make(step: *Step, prog_node: std.Progress.Node) !void {
var man = b.graph.cache.obtain();
defer man.deinit();

for (run.lazy_env_map.items) |lazy_env_var| {
const path = lazy_env_var.value.getPath2(b, step);
run.setEnvironmentVariable(lazy_env_var.key, path);
Copy link
Member

Choose a reason for hiding this comment

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

this function must be called only during the configure phase. consider that the make function can be re-executed multiple times, so it should not call this every time.

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