[Bug Fix (and docs too)] : Do not select @develop version by default by citibeth · Pull Request #1933 · spack/spack

citibeth · 2016-10-06T14:05:53Z

This PR fixes a bug in #1561. #1561 was supposed to fix bugs, not change behavior in any significant way.

Before #1561, the @develop version would not be selected in concretization, see comment from old code:

# Disregard @develop and take the next valid version

After #1561, the @develop version would be selected by default if it was available, breaking the pre-#1561 behavior. This could result in naive users (i.e. anyone not familiar with a particular package) "accidentally" obtaining the @develop version of a package without specifically requesting it.

This PR fixes that bug introduced by #1561, with two changes:

Change v.isdevelop() to not v.isdevelop(), ensuring that the @develop version will have lower default priority compared to numeric versions
Remove unnecessary v.isnumeric() from the key. This is already tested when comparing version numbers (the last part of the key), and could possibly mess things up if done separately.

This PR also comes with related documentation improvements:

Improved comments explaining the concretization preference algorithm; avoid future bugs.
Section added to the manual explaining the same thing in English.

algorithm, the intent was that the @develop version, although "greater" than numberic versions, is never preferred BY DEFAULT over numeric versions. To test this... suppose you have a package with no `preferred=True` in it, and nothing in `packages.yaml`, but with a `develop` version. For the sake of this example, I've hacked my `python/package.py` to work this way. Without bugfix (WRONG: user should never get develop by default): ``` python@develop%[email protected]~tk~ucs4 arch=darwin-elcapitan-x86_64 ... ``` With bugfix (RIGHT: largest numeric version selected): ``` [email protected]%[email protected]~tk~ucs4 arch=darwin-elcapitan-x86_64 ... ```

davydden

looks good to me, a few minor typos.

davydden · 2016-10-06T14:09:38Z

lib/spack/spack/concretize.py

            if any(v.satisfies(sv) for sv in spec.versions)]

+        # The keys below show the order of precedence of factors used
+        # to select a version when concretizing.  The the item with


typo: The the

davydden · 2016-10-06T14:11:10Z

lib/spack/spack/concretize.py

-            # Numeric versions more preferred than non-numeric
-            v.isnumeric(),
+
+            # ---------- Regular case: use lateston-develop version by default.


lateston-develop -> latest non-develop?

tgamblin · 2016-10-06T14:40:36Z

@citibeth: do you mind adding a regression test for this in one of the spec tests? Could be a separate PR.

citibeth · 2016-10-06T14:53:03Z

I think it needs to be added, but I have no idea how. If someone wants to
add the "shell" of a test with an "insert your code here" thing, that would
save me a lot of time.

On Thu, Oct 6, 2016 at 10:40 AM, Todd Gamblin [email protected]
wrote:

@citibeth https://github.com/citibeth: do you mind adding a regression
test for this in one of the spec tests? Could be a separate PR.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1933 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1cdyDmE6qeDj-LPiIvKXwNqZjGnOiPks5qxQhngaJpZM4KP-07
.

adamjstewart · 2016-10-06T15:00:00Z

@citibeth Added an issue requesting documentation on writing unit tests. See #1935.

tgamblin · 2016-10-06T15:08:13Z

Look at the spec_semantics and concretize_preferences tests. They use mock packages in var/spack/repos/builtin.mock. I believe between those two there is enough scaffolding for this. Probably this test can just live in concretize_preferences.

citibeth · 2016-10-06T15:11:40Z

And how do I run the test while I'm developing it?

On Thu, Oct 6, 2016 at 11:08 AM, Todd Gamblin [email protected]
wrote:

Look at the spec_semantics and concretize_preferences tests. They use mock
packages in var/spack/repos/builtin.mock. I believe between those two
there is enough scaffolding for this. Probably this test can just live in
concretize_preferences.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1933 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1cd6L3pvrl-IcnvMkMK5suq7th2kYQks5qxQ7ggaJpZM4KP-07
.

adamjstewart · 2016-10-06T15:15:33Z

@citibeth You can run the test by running spack test <test-name>. See spack test --list for options.

citibeth · 2016-10-06T15:54:56Z

Thanks for the info on unit/regression tests. I will not have time to write the tests today, but I believe this PR should be merged ASAP. See #1940.

Elizabeth Fischer added 2 commits October 6, 2016 09:43

Documented version selection in concretization algo.

a5d9efe

citibeth added bug Something isn't working documentation Improvements or additions to documentation labels Oct 6, 2016

citibeth mentioned this pull request Oct 6, 2016

[Bug Fix] Issues with non-numeric versions, as well as preferred=True #1561

Merged

davydden reviewed Oct 6, 2016

View reviewed changes

Fix typos

e956364

flake8

de24127

citibeth mentioned this pull request Oct 6, 2016

Add regression test for @develop version #1940

Closed

citibeth closed this Oct 6, 2016

citibeth reopened this Oct 6, 2016

tgamblin mentioned this pull request Oct 6, 2016

Document how to make regression tests #1941

Closed

tgamblin merged commit 98f8f40 into spack:develop Oct 6, 2016

davydden mentioned this pull request Oct 25, 2016

add unit tests for conretization with develop version #2064

Merged

davydden mentioned this pull request Jan 20, 2017

extend Version class so that 2.0 > 1.develop > 1.1 and develop > master > head > trunk > 9999 #1983

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Bug Fix (and docs too)] : Do not select @develop version by default#1933

[Bug Fix (and docs too)] : Do not select @develop version by default#1933
tgamblin merged 4 commits intospack:developfrom
citibeth:efischer/161006-VersionDocs

citibeth commented Oct 6, 2016

Uh oh!

davydden left a comment

Uh oh!

davydden Oct 6, 2016

Uh oh!

davydden Oct 6, 2016

Uh oh!

tgamblin commented Oct 6, 2016

Uh oh!

citibeth commented Oct 6, 2016

Uh oh!

adamjstewart commented Oct 6, 2016

Uh oh!

tgamblin commented Oct 6, 2016

Uh oh!

citibeth commented Oct 6, 2016

Uh oh!

adamjstewart commented Oct 6, 2016

Uh oh!

citibeth commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

citibeth commented Oct 6, 2016

Uh oh!

davydden left a comment

Choose a reason for hiding this comment

Uh oh!

davydden Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

davydden Oct 6, 2016

Choose a reason for hiding this comment

Uh oh!

tgamblin commented Oct 6, 2016

Uh oh!

citibeth commented Oct 6, 2016

Uh oh!

adamjstewart commented Oct 6, 2016

Uh oh!

tgamblin commented Oct 6, 2016

Uh oh!

citibeth commented Oct 6, 2016

Uh oh!

adamjstewart commented Oct 6, 2016

Uh oh!

citibeth commented Oct 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants