Skip to content

Fix initialization of defaults in yaml schema validation#7959

Merged
scheibelp merged 3 commits intospack:developfrom
scheibelp:bugfix/config-merge
May 2, 2018
Merged

Fix initialization of defaults in yaml schema validation#7959
scheibelp merged 3 commits intospack:developfrom
scheibelp:bugfix/config-merge

Conversation

@scheibelp
Copy link
Copy Markdown
Member

Fixes #7924

See also: #7925

Spack's yaml schema validator was initializing all instances of unspecified properties with the same object, so that updating the property for one entry was updating it for others (e.g. updating versions for one package would update it for other packages).

This updates the schema validator to instantiate defaults with separate object instances and adds a test to confirm this behavior (and also confirms #7924 without this change).

@adamjstewart
Copy link
Copy Markdown
Member

So does this replace #7925 or just complement it?

@scheibelp
Copy link
Copy Markdown
Member Author

So does this replace #7925 or just complement it?

Replace

Copy link
Copy Markdown
Member

@citibeth citibeth left a comment

Choose a reason for hiding this comment

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

This fix addresses the same problem as #7925; but with a greater depth of understanding, and also a unit test! I'm happy to assume it fixes the problem and merge.

}


def test_merge_with_defaults(config, write_config_file):
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.

@scheibelp Don't know how others feel about it, but for what is worth I started marking regressions like:

@pytest.mark.regression('7924')
def test_merge_with_defaults(config, write_config_file):
   ...

This may serve both to document the origin of the test + in case we want to run all the regressions we could do:

spack test -m regression

@scheibelp scheibelp merged commit d173722 into spack:develop May 2, 2018
@scheibelp
Copy link
Copy Markdown
Member Author

@alalazo thanks for the suggestion and for the review!

@citibeth thanks for reporting the problem along with detailed reproducer info, and for submitting the original fix this is based on!

@adamjstewart thanks for taking a look!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants