Skip to content

graphviz: Simplify package and add quartz support#11128

Merged
scheibelp merged 3 commits intospack:developfrom
sethrj:graphviz-quartz
Aug 6, 2019
Merged

graphviz: Simplify package and add quartz support#11128
scheibelp merged 3 commits intospack:developfrom
sethrj:graphviz-quartz

Conversation

@sethrj
Copy link
Copy Markdown
Contributor

@sethrj sethrj commented Apr 8, 2019 •
edited
Loading

  • Add quartz variant for macOS and necessary patches to get it working on newer systems
  • Disable language bindings that simply emit spack errors
  • Restrict Qt support to Qt4 (newer versions of Qt are silently ignored)
  • The X11 error mentioned in the package (and not documented anywhere) no longer happens, and /opt/X11 isn't guaranteed to exist

@sethrj sethrj requested review from citibeth and hartzell and removed request for hartzell April 8, 2019 16:53
@citibeth
Copy link
Copy Markdown
Member

citibeth commented Apr 8, 2019

Remove language bindings that simply emit spack errors

I disagree with this. If the package COULD do it; but Spack does not YET do it, then I think it's better to leave the code in the Spack package but comment it out. Less work for someone in the future; and it also indicates we already have a plan (that can be followed) when/if people DO get these new bindings working.

This looks like a lot of macOS stuff. But it's a bit complicated. My question is: how does this PR affect things not macOS? If we can show it ONLY changes things on macOS, then that will make it MUCH easier to approve and merge.

@sethrj
Copy link
Copy Markdown
Contributor Author

sethrj commented Apr 9, 2019 •
edited
Loading

Thanks for the quick review!

There should be no visible changes for non-macOS systems. "Invisible" changes are:

  • The --disable-silent-rules configure flag gets added to make the build verbose. (I've seen other packages do similar things, so I'm assuming this is pretty standard spack practice.)
  • The QT version is restricted to 4; building with a newer version would silently skip QT support.
  • The X11 variant is now exposed, though I haven't tested it on my system. (However, now X11 support is explicitly disabled when +x isn't used, rather than allowing graphviz's configure script to default it to on.)

The two new patches only apply to macOS systems. I'll restore the other languages, but I feel like the extra variants in the output are unwanted noise (graphviz~lua~python~sharp~...), and that as a general principle it's better to simplify code and expand when there's a new use case, rather than to add complexity at the beginning in case someone someday might want to use the functionality.

@citibeth
Copy link
Copy Markdown
Member

citibeth commented Apr 9, 2019 via email

scheibelp
scheibelp reviewed Apr 16, 2019
View reviewed changes
sethrj added 3 commits July 28, 2019 11:42
@sethrj
Refine dependencies of graphviz
786b9e2 
- Explicitly require QT4
- Require libx11 when building against X windows
@sethrj
Simplify graphviz package and add quartz support
1ee0d20 
- X11 error no longer happens, and `/opt/X11` isn't guaranteed to exist
- Remove language bindings that simply emit spack errors
- Add quartz variant
- Fix building with clang (libc++ vs libstdc++)
@sethrj
Sort dependencies and variants
1605b32 
These changes are purely superficial.
@sethrj sethrj force-pushed the graphviz-quartz branch from 8dca6aa to 1605b32 Compare July 28, 2019 15:50
@sethrj
Copy link
Copy Markdown
Contributor Author

sethrj commented Jul 28, 2019

Rebased on upstream/develop, incorporated changes and clarified commit messages. @scheibelp can you please take a look again?

@scheibelp scheibelp assigned scheibelp and unassigned adamjstewart Aug 1, 2019
@scheibelp scheibelp merged commit 09d4fcc into spack:develop Aug 6, 2019
@scheibelp
Copy link
Copy Markdown
Member

scheibelp commented Aug 6, 2019

Thanks!

likask added a commit to likask/spack that referenced this pull request Aug 7, 2019
@likask
Merge commit 'f7026a058b63f5a3109691e2c3871ee77c08f756' into develop_…
2e5ffa7 
…upsream_develop

* commit 'f7026a058b63f5a3109691e2c3871ee77c08f756': (1881 commits)
  Version 19.8.1 of PLASMA  (spack#12299)
  new package: py-exodus (spack#12291)
  ncurses: fix pic and opt flags (spack#12272)
  pumi: new version 2.2.1 (spack#12282)
  tests: explain and test dependency flattening routines (spack#11993)
  graphviz package: add MacOS fixes and quartz support (spack#11128)
  Overhaul numpy package (spack#12170)
  mirrors: mirror config should use spack variable expansions (spack#9027)
  stacks: fix reference handling in env.write() (spack#12096)
  fltk: fix about variable types (spack#12292)
  Avoid sending empty reports to codecov (spack#12293)
  Packages/musl (spack#12288)
  c-blosc package: Add -std=gnu99 flag for gcc (spack#11959)
  Move new packages from tutorial to builtin (spack#12289)
  Balay/amrex 19.08 (spack#12287)
  openPMD-api: pre-load depend libs (spack#12279)
  Add version 19.8.0 of PLASMA (spack#12275)
  Add version 2.5.1 of MAGMA released today (spack#12274)
  ginkgo: add maintainers (spack#12273)
  new package: py-backports-tempfile (spack#12261)
  ...

# Conflicts:
#	.travis.yml
#	var/spack/repos/builtin/packages/moab/package.py
#	var/spack/repos/builtin/packages/mofem-cephas/package.py
#	var/spack/repos/builtin/packages/mofem-fracture-module/package.py
#	var/spack/repos/builtin/packages/mofem-users-modules/package.py
#	var/spack/repos/builtin/packages/petsc/package.py
dev-zero pushed a commit to dev-zero/spack that referenced this pull request Aug 13, 2019
@sethrj @dev-zero
graphviz package: add MacOS fixes and quartz support (spack#11128)
38047ba 
* Add patch to detect newer versions of MacOS and to fix a typo in
  configure.ac (AM_LIBTOOLSFLAGS should be AM_LIBTOOLFLAGS)
* Remove variant declarations for unsupported languages
* Add support for quartz on MacOS
* Add optional X Window support
* Specifically build against qt@4 when building QT support
* Point to appropriate C++ standard libraries when building with
  Clang on MacOS
* Disable parallel build (for all platforms)
* Increase verbosity of configure script by adding
  --disable-silent-rules
@sethrj sethrj deleted the graphviz-quartz branch October 6, 2019 11:09
@glennpj glennpj mentioned this pull request Jan 4, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@scheibelp scheibelp scheibelp left review comments

@citibeth citibeth Awaiting requested review from citibeth

Assignees

@scheibelp scheibelp

Labels

macOS update-package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@sethrj @citibeth @scheibelp @adamjstewart