Skip to content

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