-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add universal build_type variant to CMakePackage #4797
Add universal build_type variant to CMakePackage #4797
Conversation
Yes, that's correct. And we want to build it with debugrelease bacause dealii is clever to build both versions and users can switch between the two in their projects |
Maybe just do it in class hierarchy? Keep CmakePackage untouched and add those defaults to a new class CmakePackageDefaults and derive from it where the defaults apply? |
That would work OOB and we won't need to make packages comply with any choice of build variant (Debug/Release/....) |
I'm not sure I understand what you're suggesting. Are you talking about adding more |
I am suggesting to keep CMakePackage class as-is and instead derive from it another class CMakePackageBuildTypy (or any other name), which would have this extra Muti-valued build_type variant. Then any actual package u modify in this PR would derive from CMakePackageBuildType and thereby would have this variant automatically |
Meh, I don't like that suggestion. Every package that uses CMake should have this variant. It's just a matter of what values to use. I don't want some CMake packages to behave differently than others. |
If u can figure how to override the build type in derived packages, that's more elegant, of course ;-) |
Oh shit, this actually works!!! I'm gonna do that. |
So it looks like if you grep |
So far I've discovered multiple duplicate packages:
Also, there's a package for What should we do about these? |
I'd been hoping for this for a while. Let's just make sure we choose terminology correction. There are two different things that could be called "build type":
As long as we have good ideas of how will will call and distinguish between these concepts, I'm OK with whatever we plan on calling them. |
I chose |
|
||
def build_type(self): | ||
spec = self.spec | ||
if '+debug' in spec: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Many of these packages overrode build_type
even though they didn't have a +debug
variant.
how about edit: i would stick with |
Also, there's a package for everytrace-example which points to an empty
repository ***@***.*** <https://github.com/citibeth>).
I dunno, that sounds weird. Just remove the package if you feel like it.
|
Okay, I went through all 104 current CMakePackages and made sure that
We can deal with the duplicate/empty packages in another PR. |
P.S. For anyone doing any work on this in the future, there are actually two variables to check for in
|
"""Returns the correct value for the ``CMAKE_BUILD_TYPE`` variable | ||
# https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html | ||
variant('build_type', default='RelWithDebInfo', | ||
description='The build type to build', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this description sucks, but I couldn't think of anything more clear. Any ideas?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about:
description='CMake build type'
?
i would use
|
# https://cmake.org/cmake/help/latest/variable/CMAKE_BUILD_TYPE.html | ||
variant('build_type', default='RelWithDebInfo', | ||
description='The build type to build', | ||
values=('Debug', 'Release', 'RelWithDebInfo', 'MinSizeRel')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah well, these: debug
, release
, rel_with_deb_info
, min_size_rel
do not look pretty...
Probably keep it as it is now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about lowercase without underscores: debug
, release
, relwithdebinfo
, minsizerel
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do that, but then it becomes a lot of work to translate the name from lowercase to CamelCase. Unless CMake doesn't care about capitalization? Even then, I think it looks better as CamelCase and would be closer to what users are expecting.
So, the consensus has been that variants should always be lowercase, with or without underscores, although we still need to convert a few variants ( |
Or `link_type`? I like that.
…On Mon, Jul 17, 2017 at 8:14 PM, Denis Davydov ***@***.***> wrote:
how about linking_type or alike for static/dynamic/fpic/etc?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4797 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1cd0e3VKXnb8fXD_4239q7chOKyZn9ks5sO_aIgaJpZM4OajgJ>
.
|
On Tue, Jul 18, 2017 at 11:55 AM, Denis Davydov ***@***.***> wrote:
I think it's better to keep a multi-valued variant but allow different
values based on build system or a specific package.
I think we should keep multi-valued variants, but name them differently for
different build systems. Then if you want debug universally, you will set:
cmake_build_type=debug, autotools_build_type=debug, etc. We can't be
requiring users to set this stuff individually for every single package in
their DAG.
|
I may be missing something: what is in the code a universal variant? If it is just a variant in a base class, then I think |
If you set a variant under `all:` in `packages.yaml`, then that variant
will be set that way by default (for every package that declares such
variant). Ideally, someone would put in `packages.yaml`:
```
all:
cmake_build_type=debug
autotools_build_type=deubg
python_build_type=deubg
```
and so forth, in order to set the build type for most packages. Then they
just have to set individual `build_type` for ad-hoc packages.
…On Tue, Jul 18, 2017 at 12:51 PM, Massimiliano Culpo < ***@***.***> wrote:
I may be missing something: what is in the code a *universal variant*?
If it is just a variant in a base class, then I think build_type is
already a good name. We don't need to call it cmake_build_type, as we
inherit from CMakePackage anyway.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4797 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1cd7Uyr60iMG0LvSeJ0enRnEL9a9Zcks5sPOJ0gaJpZM4OajgJ>
.
|
I think it's better to make it so that Spack has a semi-consistent notion of what packages:
all:
build_type: debug much clearer than this: packages:
all:
cmake_build_type: debug
autotools_build_type: debug
python_build_type: debug The second one doesn't scale... e.g. if we add a WAF package or an R package, they're all of a sudden not covered by the user's debug request, because WAF and R don't speak the same language as the other packages. Spack can provide some commonality. If users want to be specific about which packages are optimized, they can do something like this: packages:
py-numpy:
build_type: release
netlib-lapack:
build_type: release
all:
build_type: debug |
Then we need to use a set of standardized build type names across all
packages.
…On Jul 18, 2017 7:22 PM, "Todd Gamblin" ***@***.***> wrote:
I think it's better to make it so that Spack has a semi-consistent notion
of what build_type means across build systems. People configuring
packages.yaml shouldn't necessarily have to know what build systems their
packages use, and I find this:
packages:
all:
build_type: debug
much clearer than this:
packages:
all:
cmake_build_type: debug
autotools_build_type: debug
python_build_type: deubg
The second one doesn't scale... e.g. if we add a WAF package or an R
package, they're all of a sudden not covered by the user's debug request,
because WAF and R don't speak the same language as the other packages.
Spack can provide some commonality.
If users want to be specific about which packages are optimized, they can
do something like this:
packages:
py-numpy:
build_type: release
netlib-lapack:
build_type: release
all:
build_type: debug
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#4797 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AB1cd6kDYAa7kDi-KyybMciJWLOeSCCzks5sPT3_gaJpZM4OajgJ>
.
|
I think standardizing the names would be good. I don't think that has to mean that all packages support all types, but I think it's useful to have some consistency. Thoughts? |
On Tue, Jul 18, 2017 at 9:58 PM, Todd Gamblin ***@***.***> wrote:
I think standardizing the names would be good. I don't think that has to
mean that all packages support all types, but I think it's useful to have
some consistency. Thoughts?
If I set `build_type=xyz` in the `all` section of `packages.yaml`, then as
long as `xyz` is a standard build type, things should work out sensibly.
For example... Suppose I set `build_type=release_with_symbols` and some
package only supports stripped releases (without symbols). That package
should do something reasonable, rather than crashing on an "unknown
build_type".
What I'm saying is... we need to decide on a standard set of build types
that ALL packages will reasonably support --- if not support explicitly, at
least support by substituting something else that makes sense. Standard
build types should be AT LEAST `debug` and `release`.
Some packages might additionally support some non-standard build types.
Those can be set individually per-package, but can NOT be set globally in
the `all` section of `packages.yaml`.
|
Sounds reasonable to me. |
ff5d815
to
dbb297c
Compare
dbb297c
to
6e6c039
Compare
Ping. Is there any other changes that need to be made? |
@@ -43,10 +43,6 @@ class Llvm(CMakePackage): | |||
version('3.0', 'a8e5f5f1c1adebae7b4a654c376a6005', | |||
url='http://llvm.org/releases/3.0/llvm-3.0.tar.gz') | |||
|
|||
variant('debug', default=False, | |||
description="Build a debug version of LLVM, this increases " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe keep this comment somewhere in this package so that is someone has issues, he could look through the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
* satsuma2: new package (spack#4838) * salmon: new package (spack#4833) * New Package: C-Ares (spack#4840) Adds the c-ares library, a C library for asynchronous DNS requests. Required for the google gRPC library. * sickle: new package (spack#4851) * seqprep: new package (spack#4850) * smalt: new package (spack#4853) * singularity: new package (spack#4852) * lmdb: Update to 0.9.21 (spack#4830) Convert to MakefilePackage and add pkg-config file. * added new pruners-ninja version (spack#4859) * sortmerna: new package (spack#4866) * revbayes: trying this again (spack#4861) * new package: miniGMG (spack#4849) * new package: miniGMG * changed based on comments * removed cuda version * sparta: new package (spack#4867) * sparta: new package * fixing homepage * Savanna (spack#4856) Installing the stable version 0.5 through the checksummed tar.gz does not fetch the git submodule in the package. The submodule appears as an empty directory. Thus, clone the commit tagged as v0.5 using git to get around this issue * savanna: modified adios dependency spec * Replaced adios+staging with adios+flexpath+dataspaces * savanna: Enabling fortran support in adios by default * savanna: reverting to variant 'staging' for enabling all staging transports * Make testing spack commands simpler (spack#4868) Adds SpackCommand class allowing Spack commands to be easily in Python Example usage: from spack.main import SpackCommand info = SpackCommand('info') out, err = info('mpich') print(info.returncode) This allows easier testing of Spack commands. Also: * Simplify command tests * Simplify mocking in command tests. * Simplify module command test * Simplify python command test * Simplify uninstall command test * Simplify url command test * SpackCommand uses more compatible output redirection * gBenchmark: Development Package (spack#4847) * gBenchmark: Development Package Add the development version (master branch) of `gBenchmark` * gBenchmark: Remove Duplicate Remove duplicate `gbenchmark` library and keep its patch to remove the shipped -Werror * Perl - allow package activation without PERL5LIB variable (spack#4540) * perl: prepend default perl @inc path to support package activation * perl: remove stray comma from list of configure arguments * perl: final comma in configure arguments makes adding arguments safer This reverts commit fdc10cd. * perl: add comment about modified @inc (thanks to George Hartzell) * perl: use self.prefix.lib and self.prefix.bin for clarity * perl: convert tabs added by editor to spaces for flake8 * perl: use new path syntax: prefix.lib.perl5 * perl: avoid line break before binary operator * perl: use compact spack syntax for perl executable * fix sphinx dependencies, add v1.6.3 (spack#4870) * Add cuda variant for mvapich2. (spack#4800) * Add cuda variant for mvapich2. * Disable cuda for mvapich2 by default. * Add a py-theano version from git repo (spack#4871) * Change Version formatting properties and functions to return Version objects (spack#4834) * Change version.up_to() to return Version() object * Add unit tests for Version.up_to() * Fix packages that expected up_to() to return a string * Ensure that up_to() preserves separator characters * Use version indexing instead of up_to * Make all Version formatting properties return Version objects * Update docs * Tests need to test string representation * stringtie: new package (spack#4878) * added new version of cdo (spack#4877) * subread: new package (spack#4882) * structure: new package (spack#4879) * structure: new pacakge * fixing package structure (not a pun) * swarm: new package (spack#4885) * added new version of jdk 8u141-b15 (spack#4876) * stacks: new package (spack#4875) * fix GobjectIntrospection on Darwin (spack#4872) * fix GobjectIntrospection on Darwin * minor * Rename the gpu variant to cuda, this is to be consistent with other (spack#4890) packages. * Update zstd version (spack#4873) * Update zstd version * Change order of versions * Use MakefilePackage * sumaclust: new package (spack#4884) * sumaclust: new package * tweaking url and make specs * tabix: new package (spack#4886) * tabix: new package * fixed docs location * cleaveland4: new package (spack#4894) * cleaveland4: new package * fixing return line in viennarna url_for_version * fix config.guess patch for ppc64le (spack#4858) * fix config.guess patch for ppc64le * explicit patch for config.guess not required * trimgalore: new package (spack#4899) * transposome: new package (spack#4896) * transdecoder: new package (spack#4895) * transdecoder: new package * fixed package structure * fix callpath bug (spack#4659) * fix callpath bug I found while testing env/cc * fix hanging indent for flake * tmux should not set PKG_CONFIG_PATH (spack#4901) * fixes spack#967 * Version bump to 0.9.1 - Bugfixes for spack find - 0.9.1 can read specs from current develop. * Don't assume spack is in the path when building docs. * Remove PKG_CONFIG_PATH from tmux configure * Change tmux to AutotoolsPackage * Correct link to libtinfo in tmux * Add universal build_type variant to CMakePackage (spack#4797) * Add universal build_type variant to CMakePackage * Override build_type in some packages with different possible values * Remove reference to no longer existent debug variant * Update CBTF packages with new build_type variant * Keep note on build size of LLVM * shortstack: new package (spack#4905) * added MPI dependency to Nekbone package (spack#4903) * removed the tags as per comment in PR# 4749 * addressed above comments * changed fortran compiler. * added proxy application tags. * added tags by removing them from description. * addressed comments * used join_path instead of path concat. * removed the tags as per comment in PR# 4749 * addressed above comments * changed fortran compiler. * added proxy application tags. * added tags by removing them from description. * addressed comments * used join_path instead of path concat. * added tags. * changes to use MPI as depedency. * removed MPI as variant. * changed pointer to filtered makenek file. * flake 8 fix. * Updated Namespace of BML Repository (spack#4910) * Improve version detection for URLs with dynamic after version (spack#4902) * snptest: new package (spack#4900) * snptest: new package * fixed version things * fixed install phase * openblas: add 0.2.20 (spack#4915) * New Package: RSbench (spack#4752) * New Package: RSbench * minor change * removed tags as per PR# 4749 * addressed comments and added gcc compiler. * added proxy app tags to description. * removed setting CC to pgicc through spec. * removed compiler as depedency * removed pgi variant. * flake 8 fix. * added mpi depedency with pgi compiler * added pgi compiler * removed PGI compiler as depedency. * added tags and addressed other code formattings. * added tags and addressed other code formattings. * addressed comments. * Fix for Krell openspeedshop spack package bug. New multi-value variant for GUI build. (spack#4880) * Update the krell institute products to use the latest features of spack for building on cluster platforms. * Address travis error messages and resubmit the pull request. * Update the contents of openspeedshop package.py so it passes the flake8 tests. * Fix flake8 error-whitespack issue in mrnet package.py file. * Add updates based on spack reviewer feedback. * More fixes based on comments from reviewers. Switch using extend to using append, remove additional setting of PATH and LD_LIBRARY_PATH that should not be required due to RPATH. * More review related changes. Update MPIOption.append lines and take out xercesc references. * Create a base options function for common openspeedshop base cmake options to reduce redundencies. * Add libxml2+python depends on to get around issues with the libxml2 package file. * Using boost over 1.60.0 causes compile errors. This is a known boost bug. Also, dyninst-9.2.0 is set to be the vesrion of dyninst to use with OSS, as of now. The newer version fails to build. * Fix bad syntax in specifying the boost version range. * Update the version numbers for the krell institute components and tools: cbtf and openspeedshop. * Do not build glib for qt3, it is not needed and causes build problems at this time anyway. * A fix was added for setting LD_LIBRARY_PATH in the qt3 build, but if LD_LIBRARY_PATH is not set the qt build fails. So so check and set LD_LIBRARY_PATH if not set, update if it is set. * Update the fix for qt3 build by setting LD_LIBRARY_PATH instead of checking for whether it is set or not per Adams comment that spack clears LD_LIBRARY_PATH. * A fix was added for setting LD_LIBRARY_PATH in the qt3 build, but if LD_LIBRARY_PATH is not set the qt build fails. So so check and set LD_LIBRARY_PATH if not set, update if it is set. * Trim comments to fit more concisely. * Fix tabs versus spaces and swap if and else clause check from a negative to a positive check. * Fix issues with the cbtf-argonavis build, update to use dyninst-9.3.2, fixes to openspeedshop package build. * Fix issues with the cbtf-argonavis package.py files related to comments. * Add changes for changing the krell packages from Package to CMakePackage. * Add better changes for changing the krell packages from Package to CMakePackage. * Add more modifications for changing the krell packages from Package to CMakePackage. * Add additional modifications for changing the krell packages from Package to CMakePackage and fixing Travis erros * Fix new travis errors. * Fix new travis errors. * Add more changes for PR 4765. * Add more refinements to the conversion from Package to CMakePackage. * Fix new travis errors. * Add dependencies for MPI to be passed to cbtf-krell, so it can build the MPI collectors requested by the builder of openspeedshop. * Remove extra unnecessary routine to adjust build arguments. Fix if-else clause issue. * Fix more flake issues caused by last changes. * Fix a bug where openspeedshop will not build when no mpi variants are specified. Also switch to a multiple level variant for building the gui(s). Use none, qt3, and qt4 as the variants with qt3 being the default. * Add fix for spack issue spack#4843, where LTDL include files were not found. * Add the build_type variant back into the openspeedshop package file. * New Package: lcals (spack#4792) * New Pacakge: lcals * added logic for arch detection and compiler choice. * fixes for comments. * addressed comments. * removed LCALS_ARCH and added flags though spack. * addressed comments. * flake 8 fix. * reerted the changes along with comments. * Added Proxy App tag (spack#4917) * Added Proxy App tag * NO changes except proxy app tag * bml: fix homepage (spack#4918) * Adding QWT package. (spack#4911) * Adding QWT package. * Using builtin file filtering. * Formatting. * Fix for m4%clang (spack#4912) * Fix for m4%clang * Restricted condition to not subsitute rtlib on OSX * Initial Spackage for qmd-progress library (spack#4924) * Initial Spackage for qmd-progress library PROGRESS is a library is focused on the development of general solvers that are commonly used in quantum chemistry packages. * Removed LA-CC from description to fix formatting * Added Additional Formatting Requests Added requested formatting changes and also ensured that graphlib and mpi are disabled if not enabled * gBenchmark: v1.2.0 (spack#4935) Adds a the latest version of gBenchmark, release 1.2.0. This is the first gBenchmark version with proper [CMake config package installs](google/benchmark#363). This is important for dependencies building against it, such as gRPC. * zsh: add variant that skips tcsetpgrp test (spack#4923) zsh's configure script fails if there's it tries to test for terminal functionality if there's not a terminal (e.g. in a Jenkins build). The configure script has a switch that asserts that tcsetpgrp works and thereby avoids running that test. This commit adds a variant that invokes that switch, defaulting to True. * Add latest version of apr (spack#4931) * Add latest version of expat (spack#4930) * Add missing dependencies to unixodbc (spack#4928) * ZeroMQ: C++ Headers (cppzmq) (spack#4841) Adds the cppzmq library, adding a C++ API to ZeroMQ (libzmq). In order to find the autotools-build libzmq, this requires the upcoming cppzmq release (or development branch). * Add latest version of SCons (spack#4929) * Add --color=[always|never|auto] argument; fix color when piping (spack#3013) * Disable spec colorization when redirecting stdout and add command line flag to re-enable * Add command line `--color` flag to control output colorization * Add options to `llnl.util.tty.color` to allow color to be auto/always/never * Add `Spec.cformat()` function to be used when `format()` should have auto-coloring * Fix preference for X.Y version when mixed with X.Y.Z versions (spack#4922) For packages which contain a mix of versions with formats X.Y and X.Y.Z, if the user entered an X.Y version as a preference in packages.yaml, Spack would get confused and favor any version A.B.Z where X=A and Y=B. In the case where there is a mix of these version types, this commit updates preferences so Spack will favor an exact match. * Clarify docs on using a hash in a spec (spack#4908) * Fix xsdk build broken by petsc and trilinos (spack#4893) * Fix xsdk build broken by petsc and trilinos See spack#4891 for details * Fix version conflict in trilinos package Trilinos version 11 may conflict with superlu-dist. The version "xsdk-0.2.0" was conflicting with superlu-dist, even though it shouldn't. I added a lower bound to the comparison to fix this problem. Thanks for the help @davydden!
This PR adds a variant to every package that extends
CMakePackage
to controlCMAKE_BUILD_TYPE
.Instead of having every package override
build_type
on their own, I figure the best way to do this is to create a universal variant for allCMakePackage
packages.Still stumbling over a few hurdles. Apparently not all CMake packages have the same possible build types? @davydden Can you confirm that
dealii
only has the following possible values?# CMAKE_BUILD_TYPE should be DebugRelease | Debug | Release
symengine
may or may not be in the same boat. Does anyone know how to check what the possible values are for any given project? I want to make sure I didn't break any of the other packages.So I need a way of being able to override the possible values of a variant in a subclass. A simple variable didn't do the trick as the variant was registered before the value was changed. @alalazo Any ideas?
Another question: do we want the values to be lowercase or CamelCase?
Just a heads up, this will change the variants (and therefore the specs/hashes) of every CMakePackage in Spack.
@mathstuf may also be interested in this discussion.