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

correctly resolve template values used for extensions #2852

Merged
merged 5 commits into from
May 1, 2019

Conversation

boegel
Copy link
Member

@boegel boegel commented May 1, 2019

This fixes a long-standing issue where using templates in exts_list either did not work at all, or where incorrect values were being used (i.e. the ones inherited by the 'parent').

fixes #1445, #1597, #1138

@boegel boegel added the bug fix label May 1, 2019
@boegel boegel added this to the next release (3.9.1) milestone May 1, 2019
@boegel
Copy link
Member Author

boegel commented May 1, 2019

@akesandgren While working on this I realized there's another problem with templates like %(pymajver)s when used in the context of multi_deps; that issue is not fixed yet in this PR (which fixes a long-standing more general problem).

More specifically: templates like %(pymajver)s are currently not correctly resolved yet during the fetch step when Python is listed via multi_deps.

The reason for this is that the fetch step is not included in the list of steps that are iterated over.
Fixing that issue is not that trivial I think, which I why I prefer tackling that in a separate PR...

Copy link
Contributor

@akesandgren akesandgren left a comment

Choose a reason for hiding this comment

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

A bit difficult to verify by manually looking but LGTM

@akesandgren
Copy link
Contributor

Going in, thanks @boegel!

# construct dictionary with template values;
# inherited from parent, except for name/version templates which are specific to this extension
template_values = copy.deepcopy(self.cfg.template_values)
template_values.update(template_constant_dict(ext_src))
Copy link
Member

Choose a reason for hiding this comment

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

Missing an option here, this should be

template_values.update(template_constant_dict(ext_src, skip_lower=False))

Copy link
Member Author

Choose a reason for hiding this comment

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

@ocaisa alternative fix via #2856

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants