`Package.do_install` : removed `install_self` from arguments by alalazo · Pull Request #1956 · spack/spack

alalazo · 2016-10-07T11:29:09Z

Modifications

fixes package.do_install signature #1930
maintains the behavior of New command flag: spack install --dependencies-only #1603

alalazo · 2016-10-07T11:30:33Z

@citibeth Can you please confirm that it works as intended in #1603 ?

citibeth

In #1603 I was conservative with the UI. IF we're going to change
the UI, we should get rid of the negative logic
("ignore_dependencies"), as I did earlier in the code. I think we
should have a UI something like this:

subparser.add_argument(
       '--only',
        default='self,dependencies'
        dest='install_only',
        help="""Select which parts of the DAG to install --- just the top-level package, just the dependencies or both.  Comma-separated list that can include self and dependencies.  Eg: --only=self or --only=dependencies or --only=self,dependencies"""

You then parse install_only immediately after parsing the command line args:

install_only = set(args.install_only.split(','))

Now you can write sane code like if 'self' in install_only... and
get rid of the boolean variables currently in there.

Unless there's push-back, I'd get rid of the '-i' and '-d'
shortcuts. If there is push-back, you need to keep the old UI
exactly as-is. Middle ground of changing the UI
sort-of-but-not-quite sees sub-optimal.

Having more than one command line arg write into the same
destination variable also violates the "principle of least
surprise."
Yes, spack.cmd.parse_specs() can return more than one spec, if
the user supplied more than one spec. You should either:

a) Raise an exception if len(specs) != 1.

b) Iterate over the specs, installing each one of them. That's
what the code used to do, apparently. It is what I recommend
unless there's some weird technical reason that makes it not
advisable. You should probably also raise an exception if
len(specs) == 0, or at least tell users that nothing is being
installed because nothing was specified.
I see... you removed install-self from do_install() and moved
the functionality up into the function that calls it. Looks like a
good change to me, less hacky...

alalazo · 2016-10-07T14:53:39Z

I see... you removed install-self from do_install() and moved
the functionality up into the function that calls it. Looks like a
good change to me, less hacky...

That was my point in the first place and everything I wanted to do in this PR. So I don't get why you consider this not ready...

alalazo · 2016-10-07T15:01:32Z

Yes, spack.cmd.parse_specs() can return more than one spec, if the user supplied more than one spec. You should either:

Can you please share a command that actually starts installing 2 specs ? I was not able to get a string that at some point is parsed into multiple specs (that's why I added the FIXME)

citibeth · 2016-10-07T15:02:05Z

On Fri, Oct 7, 2016 at 10:53 AM, Massimiliano Culpo <
[email protected]> wrote:

I see... you removed install-self from do_install() and moved
the functionality up into the function that calls it. Looks like a
good change to me, less hacky...

That was my point in the first place and everything I wanted to do in this
PR. So I don't get why you consider this not ready...

Yes, the above quote affirms the basic thrust of the PR.

I marked it not ready because I also requested 3 significant changes. I
don't think they'll be hard to make, and I think result will be cleaner
going forward. [Maybe this is a glitch with GitHub's new "code review"
processes? Definitely look at the PR on GitHub if things don't make sense
in email]

citibeth · 2016-10-07T15:04:50Z

How about: spack install emacs python

Multiple specs used to work for me on what's now become spack module loads. It fell out of the code and I haven't bothered putting it back
in...

On Fri, Oct 7, 2016 at 11:01 AM, Massimiliano Culpo <
[email protected]> wrote:

Yes, spack.cmd.parse_specs() can return more than one spec, if the user
supplied more than one spec. You should either:

Can you please share a command that actually starts installing 2 specs ? I
was not able to get a string that at some point is parsed into multiple
specs (that's why I added the FIXME)

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

alalazo · 2016-10-07T15:06:28Z

How about: spack install emacs python

Multiple specs used to work for me on what's now become spack module loads. It fell out of the code and I haven't bothered putting it back
in...

Sorry, I still get :

$ spack install emacs python
==> Error: There are no valid versions for emacs that match ''

citibeth · 2016-10-07T15:10:25Z

Then I wouldn't bother with it and raise an exception if the user supplied

1 spec.

On Fri, Oct 7, 2016 at 11:06 AM, Massimiliano Culpo <
[email protected]> wrote:

How about: spack install emacs python

Multiple specs used to work for me on what's now become spack module
loads. It fell out of the code and I haven't bothered putting it back
in...

Sorry, I still get :

$ spack install emacs python
==> Error: There are no valid versions for emacs that match ''

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

alalazo · 2016-10-07T15:10:46Z

IF we're going to change
the UI, we should get rid of the negative logic
("ignore_dependencies"), as I did earlier in the code.

I didn't mean to change the ui, just to use the facility in argparse to limit the set of good choices. --mode is there just to state that out of the 4 combinations of 2 booleans (-i, -d) only three make sense.

I maintained the -i and -d flag not to break scripts in case somebody used those. I see though that I deleted accidentally the long version. Would it be fine with you if I put that back in place ?

To be clear : I am not against the changes you propose, just not in the scope of this PR.

citibeth · 2016-10-07T15:29:28Z

I didn't mean to change the UI...

Spack is currently pre-release, it's easier to change now than later. I'd rather break scripts in exchange for eliminating negative logic and redundant command line options now, than bake it into the UI long-term.

I also suggested changing the new flag from --mode to --only. This makes more sense in the context of the whole command. The word "mode" isn't very descriptive. Compare the two commands:

spack install --only=dependencies modele
spack install --mode=dependencies-only modele

Or more striking, compare these two:

spack install --only=self netcdf-fortran
spack install --mode=ignore-dependencies netcdf-fortran  # NEGATIVE LOGIC

OK, a mini-rant on negative logic... Examples are '--ignore-dependencies' It's bad in UIs and should be turned into positive logic whenever possible; for example, '--include-dependencies=no'. Negative logic makes UIs harder to understand. My "favorite" example of bad UI design with negative logic is "Airplane Mode" on my Android phone. When I turn Airplane Mode ON, that really means my phone's radio is being turned OFF. Similarly, turn Airplane Mode OFF to make my phone work again. Completely backwards...

alalazo · 2016-10-07T15:42:16Z

Spack is currently pre-release, it's easier to change now than later. I'd rather break scripts in exchange for eliminating negative logic and redundant command line options now, than bake it into the UI long-term.

As I said I don't mind those changes, so if @davydden @tgamblin @adamjstewart and other frequent users agree with the interface above, I'll make them here. Otherwise, I think we should open a new discussion issue to agree on what we should change in the interface.

Looking into the parser though may be a longer task better not done here

citibeth · 2016-10-07T15:59:22Z

Looking into the parser though may be a longer task better not done here

You mean the parser for multiple specs? I don't think it's worth bothering
about. You tried it, it didn't work out without fiddling. Time to move on.

alalazo · 2016-10-08T08:47:50Z

Ok, I think this should be ready ( but I'll let @citibeth decide on whether to re-apply the tag or not 😄 ). To speed things up I crafted the PR such that you can cherry pick everything but the last commit if you want to avoid backward incompatibility.

alalazo · 2016-10-08T08:48:12Z

Sorry, the message above missed @tgamblin 😄

citibeth · 2016-10-08T10:22:08Z

lib/spack/spack/cmd/install.py

+        '--only',
+        default='package:dependencies',
+        dest='things_to_install',
+        choices=['package', 'dependencies', 'package:dependencies'],


Should we also let the user do dependencies:packages here? I proposed the mini-parser to allow this flexibility in order; but for just 2 options, just adding them this way might be another approach. Probably not worth changing, since 'package:dependencies' is already the default, users will rarely use it.

Should we also let the user do dependencies:packages here?

Yes, that's the easiest way to delegate all of the parsing to argparse.

citibeth · 2016-10-08T10:23:31Z

lib/spack/spack/cmd/install.py

+    # FIXME : Is it ever possible to have more than one spec here ?
+    # FIXME : Or is it the parser wrong returning an iterable where
+    # FIXME : only an item at most can be computed ?
+    spec = specs.pop()


Remove the FIXMEs. Needs to raise an Exception if length(specs)>1

citibeth · 2016-10-08T10:24:17Z

I think it's close.

alalazo · 2016-10-13T12:11:54Z

@citibeth Are you fine with the latest changes to this PR ?

…the changes in behavior pulled in spack#1603

alalazo · 2016-10-18T07:58:58Z

@citibeth @tgamblin If you can review (or reject) this, I'll proceed with the other half of #1907

tgamblin · 2016-10-18T20:43:40Z

I made one minor modification (changed package:dependencies to package,dependencies) but otherwise this LGTM.

I believe this handles all of @citibeth's issues so hopefully she will approve the changes.

citibeth · 2016-10-18T21:38:03Z

Thanks, that looks really nice!

…r after rebasing spack#1956)

) * Removes the extra argument from Package.do_install while maintaining the changes in behavior pulled in spack#1603 * install : removed -i and -d shorthands (breaks backward compatibility) * Change ':' to ','

alalazo added the refactoring label Oct 7, 2016

alalazo added the ready label Oct 7, 2016

citibeth suggested changes Oct 7, 2016

View reviewed changes

citibeth removed the ready label Oct 7, 2016

citibeth reviewed Oct 8, 2016

View reviewed changes

alalazo added a commit to epfl-scitas/spack that referenced this pull request Oct 8, 2016

unit tests : added expected failure see spack#1956

2ce04c9

alalazo mentioned this pull request Oct 8, 2016

Add regression test for parsing multiple specs #1976

Closed

alalazo added 2 commits October 18, 2016 09:23

Removes the extra argument from Package.do_install while maintaining …

4b7615b

…the changes in behavior pulled in spack#1603

install : removed -i and -d shorthands (breaks backward compatibility)

9fe4a59

alalazo force-pushed the refactoring/do_install_signature branch from 2136dbb to 9fe4a59 Compare October 18, 2016 07:31

Change ':' to ','

9bc3d73

tgamblin force-pushed the refactoring/do_install_signature branch from 2017a0e to 9bc3d73 Compare October 18, 2016 20:42

tgamblin merged commit 0a3cc5e into spack:develop Oct 18, 2016

alalazo deleted the refactoring/do_install_signature branch October 18, 2016 21:28

alalazo added a commit to epfl-scitas/spack that referenced this pull request Oct 21, 2016

do_install : removed install_self from the list of arguments (leftove…

5ce3071

…r after rebasing spack#1956)

Conversation

alalazo commented Oct 7, 2016

Modifications

Uh oh!

alalazo commented Oct 7, 2016

Uh oh!

citibeth left a comment

Choose a reason for hiding this comment

Uh oh!

alalazo commented Oct 7, 2016

Uh oh!

alalazo commented Oct 7, 2016

Uh oh!

citibeth commented Oct 7, 2016

Uh oh!

citibeth commented Oct 7, 2016

Uh oh!

alalazo commented Oct 7, 2016

Uh oh!

citibeth commented Oct 7, 2016

Uh oh!

alalazo commented Oct 7, 2016

Uh oh!

citibeth commented Oct 7, 2016

Uh oh!

alalazo commented Oct 7, 2016

Uh oh!

citibeth commented Oct 7, 2016

Uh oh!

alalazo commented Oct 8, 2016

Uh oh!

alalazo commented Oct 8, 2016

Uh oh!

citibeth Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

alalazo Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

citibeth Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

alalazo Oct 8, 2016

Choose a reason for hiding this comment

Uh oh!

citibeth commented Oct 8, 2016

Uh oh!

alalazo commented Oct 13, 2016

Uh oh!

alalazo commented Oct 18, 2016

Uh oh!

tgamblin commented Oct 18, 2016

Uh oh!

citibeth commented Oct 18, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants