The latest commit has the obvious advantage of allowing users to enable/disable all language bindings supported by Graphviz (although the Perl and Java bindings might not build on your system, since Spack doesn't yet build Perl and Java).

More importantly, it changes the default behavior to minimize the number of dependencies required by graphviz. 99% of Spack users install Graphviz as a dependency for doxygen, which is a (soon-to-be) build dependency for some projects. Language bindings are completely unnecessary --- but they can cause conflicts, for example, with higher-level projects that use Python3. They also cause unnecessary build time for the common case where users just want doxygen. Users who need language bindings can enable them. (Note: language bindings that do not seem to add dependencies were left at their default value).

It seems like the old URL is back up now.

It seems like the old URL is back up now.

Do we have a policy on that? Fedora would seem to be a good place to grab tarballs from.

OK, I moved back to the "regular" download location. @tgamblin this should be ready to merge.

I rebased, should be ready to work. run-flake8 isn't doing it for me locally...

Spack does have Perl support now :)

OK... but I'm wondering what to do about this. Questions:

1. Which bindings involve bringing in a new dependency, and which ones
   don't? Python does, so it is turned off by default. In general, any
   language binding that requires a new dependency should be turned off by
   default.
2. Maybe we should turn off all bindings by default? It's confusing when
   some are default on and some default off.
3. BY FAR the most common use of graphviz is as a dependency to
   doxygen, which occurs as a build dependency for many packages. When a
   build process runs spack install doxygen (as a dependency for something
   else), we want a minimal set of stuff installed. We do NOT want an entire
   perl+ruby+python+r+php+... system to be built, just because you wanted to
   run doxygen. This thing is a dependency magnet...

Your thoughts?
-- Elizabeth

By far

On Wed, Oct 5, 2016 at 1:18 PM, Adam J. Stewart [email protected]
wrote:

@adamjstewart commented on this pull request.

In var/spack/repos/builtin/packages/graphviz/package.py:

• variant('sharp', default=True,

•    description='Enable for optional sharp language bindings.')
  

• variant('go', default=False,

•    description='Enable for optional go language bindings.')
  

• variant('guile', default=True,

•    description='Enable for optional guile language bindings.')
  

• variant('io', default=False,

•    description='Enable for optional io language bindings.')
  

• variant('java', default=False, # Spack has no Java support

•    description='Enable for optional java language bindings.')
  

• variant('lua', default=True,

•    description='Enable for optional lua language bindings.')
  

• variant('ocaml', default=True,

•    description='Enable for optional ocaml language bindings.')
  

• variant('perl', default=False, # Spack has no Perl support

•    description='Enable for optional perl language bindings.')
  

Spack does have Perl support now :)

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1089 (review), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1cdxEPKI3DR5OumNBSWTvjKiRv6LTVks5qw9vggaJpZM4I7U5D
.

I agree that we want as few dependencies as are actually required. Any idea what the defaults are for the configure script or in the documentation? That's what I usually go by. If it doesn't specify and none of them are needed for doxygen, then I would disable all by default.

Default configure options for graphviz are below. If something is enabled, there are three ways the configure script MIGHT work:

1. The language system is not needed; bindings get built/installed regardless.
2. The language system is needed. But if the configure script can't find it, the bindings will be silently turned off.
3. The language system is needed. And if the configure script can find it, it complains.

My guess is that the bindings here are a mixture of (1) and (2). But from a Spack point of view, I don't think it makes much sense to install language bindings without the language being avaialble. And if we try to do that, we risk having the configure script pick up random other go/sharp/python/etc. installations kicking around the system; time bomb for later.

I'm leaning toward turning them all off by default. If users want a binding, they have to turn it on. That at least is simple and predictable.

  --enable-swig=yes       swig-generated language bindings
  --enable-sharp=yes      C# language bindings
  --enable-go=no          go language bindings
  --enable-guile=yes      guile language bindings
  --enable-io=no          io language bindings
  --enable-java=yes       java language bindings
  --enable-lua=yes        lua language bindings
  --enable-ocaml=yes      ocaml language bindings
  --enable-perl=yes       perl language bindings
  --enable-php=yes        php language bindings
  --enable-python=yes     python language bindings
  --enable-python23=no    python23 language bindings
  --enable-python24=no    python24 language bindings
  --enable-python25=no    python25 language bindings
  --enable-python26=no    python26 language bindings
  --enable-python27=no    python27 language bindings
  --enable-r=yes          R language bindings
  --enable-ruby=yes       ruby language bindings
  --enable-tcl=yes        tcl language bindings

Flake8 has some complaints too. Your variant descriptions need to be indented by 4 more spaces, and there's two blank lines in a row.

I'll fix the flake8 stuff once we know we like the rest of the PR...

As I said before... I'm leaning toward disabling all language bindings by
default. Would you go along with that?

On Wed, Oct 5, 2016 at 1:59 PM, Adam J. Stewart [email protected]
wrote:

Flake8 has some complaints too. Your variant descriptions need to be
indented by 4 more spaces, and there's two blank lines in a row.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1089 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1cd4TxVu1st0mQZxeC7_lIiEkWXZQiks5qw-WVgaJpZM4I7U5D
.

@citibeth: I like that. When I install just graphviz I basically expect dot, neato, circo, etc. commands. I don't expect language bindings. Tools that need them can depend on the variant.

I don't know why I deleted my branch. Anyay, this PR is now reopened. It should be merged, IMHO.

I don't know why this is closed; reopening.

OK this looks about ready... @healther would you be able to give it a try? I copied over the jdk dependency from #3091. But I'm wondering if the dependency type is correct. Should it maybe be depends_on('jdk', when='+java', type=('build', 'run'))? @adamjstewart ???

JDK is always a run dependency. It can sometimes be a build dependency as well if the configure script searches for Java before building. But it's never a link dependency as far as I know.

How many of these variants actually build? It seems like a lot of them are missing dependencies.

Not sure. I agree, there are probably some missing dependencies.

Can you try enabling all of them and see if you can build it?

Sorry, I don't have the time for that. We can either:

1. Leave as-is, and add dependencies later as people find they're needed.
2. Comment out for now variants that we haven't tested.
3. Do a lot of testing.

I vote for (1), as it is the least effort. And it leaves the package at least as well-off as it was before: when people need a new binding, they can just add the dependency they need, rather than figuring out the configure flags, adding a variant, etc. --- in a possibly non-standard way.

Maybe should I add a comment explaining this? It would be nice to explain which language bindings DO work. I can't verify that any of them work. But if others can vouch for one or more bindings, we can add that info to the package.

Whether or not we identify which variants don't work, I would like to leave all variants in there --- either as non-functional variants; or, the package would give an error if you try to use them. That way, as people enable language bindings in the future, we don't end up changing hashes.

I vote for (2). If I run spack info for a package and see a bunch of variants I can add, and try building them and they crash, that's a "bug", not a "feature".

OK, how does this look? It checks the language bindings you've asked for, and raises an exception if that binding is not yet known to work. I want to keep all the variants in there to avoid endless annoying changes of hashes as people enable language bindings in the future.

My thinking was.... before this PR, perl was the only included language binding. And java is being added/tested now. So all the others are suspect.