Skip to content

Protect exts_lists from templating in dump method...#2619

Merged
boegel merged 9 commits intoeasybuilders:developfrom
ocaisa:limit_dump_templating
Oct 15, 2018
Merged

Protect exts_lists from templating in dump method...#2619
boegel merged 9 commits intoeasybuilders:developfrom
ocaisa:limit_dump_templating

Conversation

@ocaisa
Copy link
Copy Markdown
Member

@ocaisa ocaisa commented Oct 15, 2018

(since they are templated internally themselves)


# values for these keys will not be templated in dump()
EXCLUDED_KEYS_REPLACE_TEMPLATES = ['description', 'easyblock', 'homepage', 'name', 'toolchain', 'version'] \
EXCLUDED_KEYS_REPLACE_TEMPLATES = ['description', 'easyblock', 'homepage', 'name', 'toolchain', 'version', 'exts_list'] \
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

line too long (121 > 120 characters)

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Oct 15, 2018

Fixes #2618

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Oct 15, 2018

Hmm, the new test isn't broken before I add the fix, not sure what's going on there...but I confirm that it does fix the error described in #2618

Comment thread test/framework/easyconfig.py Outdated
'name',
'toolchain',
'dependencies', # checking this is important w.r.t. filtered hidden dependencies being restored in dump
'exts_list', # exts_lists (in Python easyconfig) use another layer of templating so shouldn't change
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

at least two spaces before inline comment

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Oct 15, 2018

@boegel I cannot get this additional test to break, I can see the difference in the outputs with --try-toolchain but the templating is not as aggressive for some reason in the Python case I included

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Oct 15, 2018

Ok, if I look directly at ectxt in the test I can see the fix works, but this is not causing problems when parsing. I tried a few things to get the test to break but I am out of ideas.

@ocaisa
Copy link
Copy Markdown
Member Author

ocaisa commented Oct 15, 2018

Ok, the test now fails without the fix so this is good to go. I don't understand why templating needs to be switched off in the parsed easyconfig but it at least catches the error and seems to be what happens in the main trunk. I would suspect it is because that there is nested templating in exts_list but maybe @boegel can explain.

@boegelbot

This comment has been minimized.

# values for these keys will not be templated in dump()
EXCLUDED_KEYS_REPLACE_TEMPLATES = ['description', 'easyblock', 'homepage', 'name', 'toolchain', 'version'] \
+ DEPENDENCY_PARAMETERS
+ ['exts_list'] + DEPENDENCY_PARAMETERS
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@ocaisa Let's keep this sorted alphabetically, it'll be way easier to tell whether a particular parameter is included or not that way upon visual inspection...

Done in ocaisa#38

@boegel boegel added the bug fix label Oct 15, 2018
@boegel boegel added this to the 3.7.1 milestone Oct 15, 2018
@boegel boegel changed the base branch from develop to 3.7.x October 15, 2018 17:42
maintain alphabetical sorting for values of EXCLUDED_KEYS_REPLACE_TEMPLATES
@boegel
Copy link
Copy Markdown
Member

boegel commented Oct 15, 2018

@ocaisa To answer your question: I think we deliberately don't resolve things like %(namelower)s for extensions because of the ambiguity w.r.t. main name or extension name...

@boegel boegel changed the base branch from 3.7.x to develop October 15, 2018 19:02
@boegel
Copy link
Copy Markdown
Member

boegel commented Oct 15, 2018

Going in, thanks @ocaisa!

@boegel boegel merged commit 1429426 into easybuilders:develop Oct 15, 2018
@ocaisa ocaisa deleted the limit_dump_templating branch October 16, 2018 07:21
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.

4 participants