multi-valued variants comparison fix#5593
Closed
scheibelp wants to merge 3 commits intospack:developfrom
Closed
Conversation
…ent representation for comparisons
Member
Author
|
This fails for python 3.x since basestring is not there apparently but this still illustrates the problem and passes for 2.x |
Member
Author
|
I have re-implemented this in terms of method overrides |
Member
I don't think that's true: it would have failed way before if that was the behavior, and there's a test here to check that ordering is not arbitrary + what you do in this PR is already done in the value setter. The problem seems instead to be that in #5476 we started setting private members directly, without sorting their arguments. I refer to this line in the current code. I'll try to submit a patch for that. |
alalazo
added a commit
to epfl-scitas/spack
that referenced
this pull request
Oct 4, 2017
refers spack#5587 closes spack#5593 The implementation introduced in spack#5476 used `dedupe` to set a private member of a `MultiValuedVariant` object. `dedupe` is meant to give a stable deduplication of the items in a sequence, and returns an object whose value depends on the order of items in its argument. Now the private member is set implicitly using the associated property setter. The logic changes from stable deduplication to a totally ordered deduplication (i.e. instead of `dedupe(t)` it uses `sorted(set(t))`). This should also grant that packages that shuffle the same set of patch directives maintain the same hash.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5587
@alalazo, @tgamblin
While debugging #5587 I noticed the following:
And noticed the patches are differently-ordered in the two specs, which led me to look at
_cmp_keyfor variants. It looks like multi-valued variants arbitrarily order their values which was resulting in a failed comparison.This is hackish and WIP but demonstrates the issue.