Git's gitk needs TK's wish to be found in PATH by KineticTheory · Pull Request #8360 · spack/spack

KineticTheory · 2018-06-04T15:35:31Z

Background

gitk depends on TK at runtime. If TK's wish program isn't already on the system, gitk will fail to launch.
I've been manually modifying the git module file generated by spack to satisfy this requirement. This PR captures the requirement and automates the installation of tk and correct modulefile generation.

Changes

Add a runtime build dependency on tk
Add an environment rule to add the path to TK's wish program to $PATH for the generated git modulefile.

Impact

After this PR, spack install git must also build tk. This is a somewhat deep tree of dependencies.

  ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
        ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
            ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
                ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
            ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
            ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
                ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
                ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
                    ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
                ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
                ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
            ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
            ^[email protected]%[email protected] arch=linux-rhel7-x86_64 
        ^[email protected]%[email protected] arch=linux-rhel7-x86_64

After this PR, the git modulefile generated by spack will contain the extra line (or similar depending on compiler):

+ prepend_path("PATH", "${SPACK_ROOT}/opt/spack/linux-rhel7-x86_64/gcc-4.8.5/tk-8.6.8-uaspjhewvbjfgqt54bmzqy33rv5i5zij/bin")

* Add a _runtime_ dependency on `tk` * Add an environment rule to add the path to TK's `wish` program to $PATH for the generated `git` modulefile.

adamjstewart · 2018-06-04T16:39:43Z

This should definitely be a variant. It looks like configure has a --with-tcltk flag to enable/disable this. I would vote for default=False as I didn't even know gitk existed and I don't know anyone who uses it.

healther

I'm not sure how I feel about tk being an unconditional dependency of git though ... Is there an option to build git without gitk?

healther · 2018-06-04T16:39:07Z

var/spack/repos/builtin/packages/git/package.py

                self.spec['gettext'].prefix.include))

+        # gitk requires tk's wish to be found in $PATH.
+        run_env.prepend_path('PATH', self.spec['tk'].prefix.bin)


this is only executed at build time, just make the dependency of type ('build', 'run') and you will be fine.

KineticTheory · 2018-06-04T21:57:04Z

@adamjstewart gitk is almost certainly already being built when you spack install git since the default value for --with-tcltk is YES. It is already built by default for all of the systems that I support.

@healther Maybe I don't understand how setup_environment works, so correct my PR if there is an issue with it. At least for me, it looks like tk is only a runtime dependency of gitk (i.e. it is not needed for building gitk). Also, removing the prepend_path option from setup_enfironment breaks the generated modulefile as tk's $prefix/bin isn't added to the run environment.

I have provided an update to this PR to make my changes optional.

adamjstewart · 2018-06-04T23:01:16Z

This looks good to me now.

As for the run_env modules thing, I think this opens up a broader question. Shouldn't Spack always add all run-time dependencies to PATH and friends? This should at least be an option. I don't think it should be the job of the git package to explicitly list which dependencies make it into its module file, I think it should be done automatically.

@alalazo may have opinions on this.

citibeth · 2018-06-05T02:02:27Z

I'm uncomfortable with including dependencies' stuff in module files in an ad-hoc manner. There are TONS of cases where module B must be loaded for module A to work; in fact, that is almost always the case, except in the case of RPATH'd binaries. Anyone using Python, R, etc. must load tons of modules to get anything to work.

I would prefer a general solution that handles module dependencies.

healther

I'm still uncertain about the environment setting. If I understand our module generation (note: I don't really use these at all) correctly, then a module generates ONLY the environment for the specific package, it is the user's job to make sure that the dependencies are also module loaded or otherwise add the autoload stuff to the module generation.

So my questions are essentially: 1) In how far is this not consistent with how other modules behave? and 2) Is this a sensible behaviour in the first place?

I think the answer to 1) is "It is consistent" (which imho would mean remove the environment setting from this PR)
but the more problematic one is 2). The fundamental building block of spack is a DAG, if so shouldn't the provided modules reflect that? But this is a larger discussion and separate from adding the depends_on('tk'.

KineticTheory · 2018-06-05T15:08:29Z

I agree with @citibeth that a module-specific solution would be better. However, such a solution isn't available right now and, as it stands, the git recipe is installing an applications that doesn't work. Since Tcl/Tk is not an unusual environment or one that is likely to conflict with other installed packages, I consider this to be a low risk addition. The variant provided in my PR essentially provides a +gui build for the git install. This is consistent with spackage definitions for other tools that provide an optional GUI component.

Also, my experience is different from @adamjstewart, in that essentially everyone that uses these git installations also use gitk as an essential part of their work flow. So, everytime I install git with spack, it is effectively broken (and I'll hear about it!) until I either create symlinks or modify modulefiles (btw - Tcl/Tk is also installed via spack for the build environments).

As I see it, there are two issues discussed here:

Spack needs a module-specific solution for this scenario. This should be a separate development discussion/task and is beyond the scope of this PR. I'm not able to champion this effort. Maybe it can be rolled into another issue.
If this PR is considered abusive of the spack's current features, then it should be closed and I'll continue to manually update files that spack installs so that user's environments are functional. Otherwise, I would like constructive feedback as to how to modify this PR so that spack can install an out-of-the-box working version of gitk.

citibeth · 2018-06-05T15:21:45Z

@healther @KineticTheory

I agree with @KineticTheory, this probably needs to be merged. At the same time, I'm hoping we can use this issue to raise awareness and discussion on the use of Spack where recursive module loading is needed --- which is basically everywhere, except for RPATH'd binaries.

See my rant here:
https://groups.google.com/forum/#!topic/spack/FKWH-N02cEQ
See also the discussion at #3134

citibeth · 2018-06-05T15:24:27Z

@KineticTheory Can you comment on #8153? It could also be relevant to your experience here; and we need to get it merged.

healther · 2018-06-06T08:42:01Z

I'm not against merging it, but I think there is a way to do it with the module generation right now. The problem is, that I actually don't use this, so I don't have first hand knowledge, but iirc you can tell modules.yaml to autoload the dependencies

citibeth · 2018-06-06T13:32:59Z

@alalazo modules.yaml is a user-level file. Unless we're prepared to ship a particular modules.yaml file with Spack, it's not something we can do.

But wouldn't it be better (in the long run) if this information on recursive loading were provided in the package.py file where it belongs, rather than off in some external centralized modules.yaml? I was told this can't happen because modules aren't "really" a part of Spack. But I have never found that argument to be convincing. In any case, putting the information needed to correctly generate modules into the packages does not REQUIRE that modules be generated or used.

healther · 2018-06-06T13:55:16Z

Am I missing something? The information is there in the depends_on() isn't it? The problem is, that it doesn't get automatically handed down to the module generation. And yes I also thought the default should be the other way around :)

reason for block no longer applicable

citibeth · 2018-06-06T13:59:26Z

Am I missing something? The information is there in the depends_on() isn't it?

Almost... it depends on the package you're depending on. depends_on('zlib') does not require a recursive module load, but depends_on('py-numpy') does. To first order, this can be determined by the type of package (PythonPackage, AutotoolsPackage, etc). But some tweaking around the edges would almost certainly be needed.

The alternative is to just recursively load all modules, i.e. load anything where we see depends_on(); this is what spack module loads does. This is a bit messy, but it's gotten the job done for me in practice.

healther · 2018-06-06T14:53:39Z

it depends on the package you're depending on. depends_on('zlib') does not require a recursive module load, but depends_on('py-numpy') does

but isn't that exactly the difference between a link and a run dependency? Btw zlib is a poor example as it doesn't have any dependencies itself so autoload would be a noop right?

^Sure for run dependencies one could also subdivide which of the environment variables are really necessary, but that's an optimisation.

citibeth · 2018-06-06T14:55:38Z

I suppose so. So far, auto-loading everything has worked out well enough for me, so I haven't put in any effort to be more precise. And getting Spack Environments merged is a higher priority for me.

…

On Wed, Jun 6, 2018 at 10:53 AM, healther ***@***.***> wrote: it depends on the package you're depending on. depends_on('zlib') does not require a recursive module load, but depends_on('py-numpy') does but isn't that exactly the difference between a link and a run dependency? Btw zlib is a poor example as it doesn't have any dependencies itself so autoload would be a noop right? ^Sure for run dependencies one could also subdivide which of the environment variables are really necessary, but that's an optimisation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8360 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd-Nub7rEaWTtCS36TIRGoyeIISRKks5t5-z2gaJpZM4UZUM4> .

alalazo

Just my point of view, not a strict request. Git configure help says:

  --with-tcltk            use Tcl/Tk GUI (default is YES)
                          ARG is the full path to the Tcl/Tk interpreter.
                          Bare --with-tcltk will make the GUI part only if
                          Tcl/Tk interpreter will be found in a system.

I would be for adding an explicit:

if '+tcltk' in self.spec
    configure_args.append('--with-tcl-tk={0}'.format(self.spec['tk'].prefix.bin.wish))
else:
    configure_args.append('--with-tcl-tk=NO')  # Or whatever is the option to disable it

and making tk a (build, link) dependency.

alalazo · 2018-06-08T12:15:02Z

var/spack/repos/builtin/packages/git/package.py


+        # gitk requires tk's wish to be found in $PATH.
+        if '+tcltk' in self.spec:
+            run_env.prepend_path('PATH', self.spec['tk'].prefix.bin)


I think it's better to check:

if 'tk' in self.spec: run_env.prepend_path('PATH', self.spec['tk'].prefix.bin)

The way this statement is written right now is bound to fail when git is declared as an external (as the variant might be there, but the dependency is not).

@adamjstewart @KineticTheory Forgot to mention: I don't know tcl and tk at all, but it seems to me that the situation here might be common for packages that depend on tk. If somebody could confirm this impression, I think the proper place where to modify the run_env is the setup_dependnent_environment of the tk package.

@alalazo I must confess that I too am blissfully ignorant of tcl/tk environments. 😌

I'll implement your recommended changes in my branch and do a couple of test builds before I push the updates.

alalazo · 2018-06-08T12:28:28Z

var/spack/repos/builtin/packages/git/package.py

    depends_on('automake', type='build')
    depends_on('libtool',  type='build')
    depends_on('m4',       type='build')
+    depends_on('tk',       type='run', when='+tcltk')


This might be a build dependency it we pass the correct path at run-time to --with-tcltk. I understand your point on tk not being strictly needed during compilation, but maybe it's worth considering:

Making tk a (build, link) dependency conditional on the variant

Pass its prefix to the configure script if +tcltk

KineticTheory · 2018-06-09T17:28:10Z

The updates provided in 94e717e

provide the functionality that I was after (no changes to tk/packages.py required), and
prevents gitk from being built/installed unless requested via +tcltk,

Thanks to @alalazo, @citibeth, @healther, and @adamjstewart for helpful recommendations.

Git's gitk needs TK's wish to be found in PATH

ddd2da2

* Add a _runtime_ dependency on `tk` * Add an environment rule to add the path to TK's `wish` program to $PATH for the generated `git` modulefile.

adamjstewart added dependencies git labels Jun 4, 2018

healther previously requested changes Jun 4, 2018

View reviewed changes

Make TclTk run environment an optional variant for git.

ef9fae2

healther reviewed Jun 5, 2018

View reviewed changes

healther mentioned this pull request Jun 5, 2018

WIP: Install python applications into virtualenvs #8364

Closed

alalazo reviewed Jun 8, 2018

View reviewed changes

Cleanup based on PR recommendations.

94e717e

adamjstewart approved these changes Jun 9, 2018

View reviewed changes

adamjstewart merged commit 689e0cb into spack:develop Jun 9, 2018

KineticTheory deleted the git_gitk_needs_tk_wish branch April 2, 2019 21:27

Conversation

KineticTheory commented Jun 4, 2018

Background

Changes

Impact

Uh oh!

adamjstewart commented Jun 4, 2018

Uh oh!

healther left a comment

Choose a reason for hiding this comment

Uh oh!

healther Jun 4, 2018

Choose a reason for hiding this comment

Uh oh!

KineticTheory commented Jun 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjstewart commented Jun 4, 2018

Uh oh!

citibeth commented Jun 5, 2018

Uh oh!

healther left a comment

Choose a reason for hiding this comment

Uh oh!

KineticTheory commented Jun 5, 2018

Uh oh!

citibeth commented Jun 5, 2018

Uh oh!

citibeth commented Jun 5, 2018

Uh oh!

healther commented Jun 6, 2018

Uh oh!

citibeth commented Jun 6, 2018

Uh oh!

healther commented Jun 6, 2018

Uh oh!

citibeth commented Jun 6, 2018

Uh oh!

healther commented Jun 6, 2018

Uh oh!

citibeth commented Jun 6, 2018 via email

Uh oh!

alalazo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alalazo Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

alalazo Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

KineticTheory Jun 9, 2018

Choose a reason for hiding this comment

Uh oh!

alalazo Jun 8, 2018

Choose a reason for hiding this comment

Uh oh!

KineticTheory commented Jun 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

KineticTheory commented Jun 4, 2018 •

edited

Loading

alalazo left a comment •

edited

Loading