Flag handlers #6415

becker33 · 2017-11-22T20:19:02Z

@tgamblin and I were not happy with my initial implementation of flag handlers. This new implementation provides a more intuitive syntax that can be more easily extended by individual packages.

The default behavior of compiler flags will not change: By default they are injected through the compiler wrappers.

This PR adds a flag_handler method to the package which controls how the compiler flags are passed to the build and can be used to add package-specific flags. Three builtin flag handlers are defined on PackageBase. inject_flags is the default, env_flags puts the flags into environment variables, and command_line_flags puts the flags on the command line for build systems that support doing so. Packages can also override the flag_handler method (called independently on each set of flags) by hand.

For further detail, see the accompanying changes to the documentation.

@reviewers: Let me know if you think the flags_to_cl_args method should be documented. I don't expect it to be user facing, but if you think there's use in putting it in the documentation I can add it. It would allow users to implement command line flag handling on a per-package basis, but I don't currently see a use case for that.

scheibelp

This is a preliminary review which is mostly questions. I plan on looking at this more tomorrow. Let me know if a question was unclear.

scheibelp · 2017-11-29T02:38:21Z

lib/spack/docs/tutorial_buildsystems.rst

@@ -103,7 +103,7 @@ This will open the :code:`AutotoolsPackage` file in your text editor.
    long examples. We only show what is relevant to the packager.


-.. literalinclude:: tutorial/examples/Autotools/autotools_class.py


I'm curious why this can be replaced now, previously it looks like this only differed in terms of referring to a specific dependency package pkgconfig. Are the other changes in this PR required to remove autotools_class and refer to build_systems/autotools? Was it that this could have been done before but is of interest now because otherwise you would have to update two files?

This could have been done before, and being done now to avoid updating multiple files for the same change. Similarly in the equivalent changes for other classes.

scheibelp · 2017-11-29T02:40:43Z

lib/spack/spack/build_environment.py

+    env_flags = {}
+    command_line_flags = {}
+    for flag in spack.spec.FlagMap.valid_compiler_flags():
+        injf, envf, clf = pkg.flag_handler(flag, pkg.spec.compiler_flags[flag])


I'd prefer more-descriptive variable names.

I'm trying to strike a balance between descriptive names and keeping lines of code together within the flake constraints. If you think I've struck the wrong point in that balance, I'm open to changing it.

I personally think it's worthwhile to break the line up to get more-descriptive variable names, potentially like:

inject_flags, env_flags, cmdline_flags = pkg.flag_handler( flag, pkg.spec.compiler_flags[flag])

scheibelp · 2017-11-29T02:42:23Z

lib/spack/spack/build_environment.py

+    command_line_flags = {}
+    for flag in spack.spec.FlagMap.valid_compiler_flags():
+        injf, envf, clf = pkg.flag_handler(flag, pkg.spec.compiler_flags[flag])
+        inject_flags[flag] = [] if injf is None else injf


Is the distinction between [] and None here what allows a user to remove all flags for a specified package? I noticed that the documentation was modified to add:

Note that it would be equivalent to return [] instead of None.

Removing None here allows the user to write more intuitive code (returning None for no flags) at very minimal cost. If we wanted to we could remove these lines and require that flag_handler return a triple of lists.

scheibelp · 2017-11-29T02:55:21Z

lib/spack/spack/build_systems/cmake.py

+            lang_flags = ' '.join(flags.get(flag, []) + flags.get('cppflags',
+                                                                  []))
+            if lang_flags:
+                self.cmake_flag_args.append(flag_string.format(lang,


This appears to be new behavior for cppflags for CMake-based packages, and I didn't see it documented (IMO it would be worthwhile to document the behavior for CMake explicitly).

Currently the flags are injected on the basis of the language and function under which the cc wrapper is called, and cppflags is applied to C, CXX, and Fortran compilers, as it is for autotools. CMake has no options that are directly equivalent to cppflags, so we have to manually add cppflags in all the cases in which it is relevant.

OK - I still think it's worthwhile to mention in the documentation somewhere that cppflags which are added as command-line arguments are tacked on to other flags for CMake

Added elsewhere in comments.

scheibelp · 2017-11-29T03:00:58Z

lib/spack/spack/test/flag_handlers.py

+        assert 'SPACK_CPPFLAGS' not in os.environ
+        assert 'CPPFLAGS' not in os.environ
+
+        expected = set(['-DCMAKE_C_FLAGS=-g', '-DCMAKE_CXX_FLAGS=-g',


I'm confused why setting cppflags should also set C_FLAGS and Fortran_FLAGS

cppflags is not C++ specific, it is for the C pre-processor. The C pre-processor is called on C, C++, and Fortran code. I don't entirely understand why it is called on Fortran code, but I went off of the Autotools implicit make variables (https://www.gnu.org/software/make/manual/html_node/Implicit-Variables.html)

cxxflags is C++ specific.

scheibelp

A few requests related to variable naming.

One thing I'm curious about is why a user would want to take compiler flags and express them as environment variable changes. The term "compiler flag" seems to me like the sort of thing that should be added to the compiler wrapper automatically. Is there some context discussion for why this support is added? Why not use a different variable to explicitly record when an argument should be added to the build system invocation? IMO that would be useful to add to the docs (but I'm probably just missing context).

scheibelp · 2017-11-29T18:49:36Z

lib/spack/spack/build_environment.py

+        env_flags[flag] = [] if envf is None else envf
+        command_line_flags[flag] = [] if clf is None else clf
+
+    # Add every compiler flag to the appropriate place


"the appropriate place" -> something more descriptive

scheibelp · 2017-12-01T21:51:59Z

lib/spack/docs/packaging_guide.rst

-and fall back on the default for that package class if neither exists.
+Packages can override the flag_handler method with one of three
+builtin flag_handlers. The builtin flag_handlers are named
+``inject_flags``, ``env_flags``, and ``command_line_flags``. The


Would build_system_flags be a more-explanatory name than command_line_flags? IMO this is more related to configuration than to the command line (although calling them command line flags is accurate in the sense that the other flags are not added to the command line).

IMO inject_flags could have a more-explanatory name too, although I haven't thought of a decent replacement yet.

scheibelp · 2017-12-01T22:30:05Z

lib/spack/spack/package.py

@@ -1707,6 +1707,36 @@ def setup_dependent_package(self, module, dependent_spec):
        """
        pass

+    def inject_flags(self, name, flags):


IMO it's worth adding a description here saying this is the flag handler implementation which injects all flags.

scheibelp · 2017-12-01T22:49:38Z

lib/spack/spack/test/flag_handlers.py

+        pkg.flag_handler = pkg.inject_flags
+        spack.build_environment.setup_package(pkg, False)
+
+        assert os.environ['SPACK_CPPFLAGS'] == '-g'


Would you want to clear the environment after these tests?

The mock environment fixture at the top handles that, it restores the previous environment after each test.

scheibelp · 2017-12-01T23:08:32Z

lib/spack/spack/build_systems/cmake.py

+            lang_flags = ' '.join(flags.get(flag, []) + flags.get('cppflags',
+                                                                  []))
+            if lang_flags:
+                self.cmake_flag_args.append(flag_string.format(lang,


OK - I still think it's worthwhile to mention in the documentation somewhere that cppflags which are added as command-line arguments are tacked on to other flags for CMake

scheibelp · 2017-12-01T23:16:57Z

lib/spack/spack/package.py

+    # inject_flags, env_flags, and command_line_flags methods are provided in
+    #  module scope as easy default options for users.
+
+    def flags_to_cl_args(self, flags):


I'd prefer the name flags_to_cmd_line_args (or flags_to_build_system_args)

scheibelp · 2017-12-02T04:16:16Z

lib/spack/spack/build_environment.py

+    env_flags = {}
+    command_line_flags = {}
+    for flag in spack.spec.FlagMap.valid_compiler_flags():
+        injf, envf, clf = pkg.flag_handler(flag, pkg.spec.compiler_flags[flag])


I personally think it's worthwhile to break the line up to get more-descriptive variable names, potentially like:

inject_flags, env_flags, cmdline_flags = pkg.flag_handler( flag, pkg.spec.compiler_flags[flag])

becker33 · 2017-12-07T01:35:21Z

@scheibelp Changes addressed.

scheibelp

This is mostly comments about the documentation where I think things could be clearer. There is one style change request. I'm curious if multiple flags need to be enclosed in quotes (and the tests should probably include examples of this).

scheibelp · 2017-12-15T02:35:18Z

lib/spack/docs/packaging_guide.rst

+to the build in one of three ways. By default, the build environment
+injects these flags directly into the compiler commands using Spack's
+compiler wrappers. The flags may alternatively be passed through
+environment variables or through build system arguments. The


IMO adding the following would clarify the intend of the handlers:

To register the flags with the build system, the flags may alternatively be passed through environment variables or as build system arguments.

Generally I think it would be good to emphasize the motivation for these functions where possible.

scheibelp · 2017-12-15T02:53:49Z

lib/spack/docs/packaging_guide.rst

+``inject_flags`` method is the default. The ``env_flags`` method puts
+all of the flags into the environment variables that ``make`` uses as
+implicit variables ('CFLAGS', 'CXXFLAGS', etc.). The
+``build_system_flags`` method is only available for


IMO it would be clearer to say:

Setting compiler flags with build system arguments is only currently supported for cmake and autotools packages...

Other implementations are possible where e.g. flags are injected and also reported to the build system, so IMO it is worthwhile to focus on the concept of how compiler flags are treated vs. the implementation detail.

scheibelp · 2017-12-15T02:56:25Z

lib/spack/docs/packaging_guide.rst

+``envf`` is a list of flags to pass to the build system through the
+environment using implicit variables, and ``bsf`` is a list of flags
+to pass to the build system as arguments. If ``bsf`` is non-empty in a
+package that does not support ``build_system_flags``, the build


Do you mean:

... in a package that does not support flags_to_build_system_args...

?

also I would avoid mentioning NotImplementedError as that is more of an implementation detail. E.g. rephrasing like:

Registering compiler flags with the build system is not supported for packages which do not implement the flags_to_build_system_args method (e.g. autotools and cmake)

scheibelp · 2017-12-15T03:06:27Z

lib/spack/docs/packaging_guide.rst

+           return (None, flags, None)
+       elif name == 'ldlibs':
+           flags.append('-lbar')
+       return (flags, None, None)

 Because these methods can pass values through environment variables,
 it is important not to override these variables unnecessarily in other


IMO it would clarify things to add a "for example":

... it is important not to override these variables unnecessarily (e.g. setting CFLAGS) in other...

scheibelp · 2017-12-15T03:20:00Z

lib/spack/docs/packaging_guide.rst

+list of flags whenever the flag handler is ``env_flags``. If the
+package passes flags through the environment or the build system
+manually (in the install method, for example), we recommend using the
+default flag handler or rewriting the package to pass its flags


IMO the following rephrasing:

... we recommend using the default flag handler, or removing manual references and implementing a customized flag handler method that adds the desired flags to export as environment variables or pass to the build system

is clearer than

... we recommend using the default flag handler or rewriting the package to pass its flags through the flag handler rather than manually

scheibelp · 2017-12-15T19:12:14Z

lib/spack/docs/packaging_guide.rst

+           return (None, flags, None)
+       elif name == 'ldlibs':
+           flags.append('-lbar')
+       return (flags, None, None)

 Because these methods can pass values through environment variables,
 it is important not to override these variables unnecessarily in other
 package methods. In the ``setup_environment`` and


I would add:

... in other package methods when using alternative flag handlers.

(i.e. clarify when people have to worry about manually setting compiler flags).

scheibelp · 2017-12-15T19:25:23Z

lib/spack/spack/build_environment.py

+    build_system_flags = {}
+    for flag in spack.spec.FlagMap.valid_compiler_flags():
+        injf, envf, bsf = pkg.flag_handler(flag, pkg.spec.compiler_flags[flag])
+        inject_flags[flag] = [] if injf is None else injf


IMO it would be slightly more idiomatic to say:

inject_flags[flag] = inf or []

(whenever I see tests for None specifically I have to think about why it is being treated specially)

I didn't realize I could do that, yes that's definitely cleaner!

scheibelp · 2017-12-15T19:37:26Z

lib/spack/spack/build_systems/cmake.py

+        # We specify for each of them.
+        if flags['ldflags']:
+            ldflags = ' '.join(flags['ldflags'])
+            ld_string = '-DCMAKE_{0}_LINKER_FLAGS={1}'


For multiple flags should this be enclosing in quotes to produce something like:

-DCMAKE_..._LINKER_FLAGS='-a -b'

? Likewise for all other constructions of environment/build-system variables

I don't believe so. When you invoke cmake by hand, the quote marks are stripped by the shell and the multiple flags are passed as a single space-delimited string. I believe the python implementation is still passing them as a single string, because the underlying Executable implementation does not use the shell argument to subprocess.Popen.

If you're not convinced by this I'll take that as evidence that I shouldn't be so sure of myself and do some more rigorous testing of this.

…with code

… warning

scheibelp

The remaining requests all relate to comments/documentation. Otherwise LGTM

scheibelp · 2017-12-19T19:16:54Z

lib/spack/spack/package.py

+    def build_system_flags(self, name, flags):
+        """
+        flag_handler that passes flags to the build system arguments.
+        Not implemented for all build systems.


I think the following would be clearer:

Any package which exports flags to build system arguments must also implement flags_to_build_system_args

scheibelp · 2017-12-19T19:38:24Z

lib/spack/spack/test/flag_handlers.py

+
+
+def add_O3_to_cl_cflags(name, flags):
+    cl_flags = []


Replace instances of "cl" with "build_system"?

scheibelp · 2017-12-19T19:42:16Z

lib/spack/spack/test/flag_handlers.py

+        pkg2 = spack.repo.get(s2)
+        spack.build_environment.setup_package(pkg2, False)
+
+        # Use cppflags as a canary


Perhaps call this test test_inject_all_flags since it is also confirming that environment variables are not set

This test is for when there are no flags.

The following test test_inject_flags tests for when all flags are injected.

I'm not entirely confident that this test is necessary, but the idea is that you would get different information from seeing test_inject_flags fail if this also fails. It's just another diagnostic for the absolute base case .

Got it - maybe a better name would be test_no_compiler_flags?

scheibelp · 2017-12-19T20:06:07Z

lib/spack/docs/packaging_guide.rst

+behavior.
+
+Packages can override the flag_handler method with one of three
+builtin flag_handlers. The builtin flag_handlers are named


Not very important but I think the word is "built-in"

becker33 · 2017-12-20T01:32:13Z

@scheibelp comments addressed.

scheibelp

Additional comments on documentation (typos/consistency)

scheibelp · 2017-12-20T02:35:50Z

lib/spack/spack/package.py

+        """
+        flag_handler that passes flags to the build system arguments.
+        Any package using `build_system_flags` must also implement
+        `flags_to_build_args`, or derive from a class that implements it.


Typo: flags_to_build_args -> flags_to_build_system_args

scheibelp · 2017-12-20T02:37:16Z

lib/spack/spack/package.py

+
+    flag_handler = inject_flags
+    # The flag handler method is called for each of the allowed compiler flags.
+    # It returns a triple of inject_flags, env_flags, command_line_flags.


Consistency: "inject_flags, env_flags, and build_system_flags."

becker33 · 2017-12-20T18:25:22Z

@scheibelp addressed comments.

This adds the ability for packages to apply compiler flags in one of three ways: by injecting them into the compiler wrapper calls (the default in this PR and previously the only automated choice); exporting environment variable definitions for variables with corresponding names (e.g. CPPFLAGS=...); providing them as arguments to the build system (e.g. configure). When applying compiler flags using build system arguments, a package must implement the 'flags_to_build_system_args" function. This is provided for CMake and autotools packages, so for packages which subclass those build systems, they need only update their flag handler method specify which compiler flags should be specified as arguments to the build system. Convenience methods are provided to specify that all flags be applied in one of the 3 available ways, so a custom implementation is only required if more than one method of applying compiler flags is needed. This also removes redundant build system definitions from tutorial examples

@adamjstewart

* fixes #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. * Quick fix for relocation issues. * elf relocation fix: cherry-picked from develop branch (#6889) * Revert "Quick fix for relocation issues." This reverts commit 57608a6. * Buildcache: relocate fixes (#6512) * Updated function which checks if a binary file needs relocation. Previously this was incorrectly identifying ELF binaries as symbolic links (so they were being excluded from relocation). Added test to check that ELF binaries are not considered symlinks. * relocate_text was not replacing paths in text files. Added test to check that text files are relocated properly (i.e. paths in the file are converted to the new prefix). * Exclude backup files created by filter_file when installing from binary cache. * Update write_buildinfo_file method signature to distinguish between the spec prefix and the working directory for the binary cache package. * Final changes for v0.11.0 (#6318) * Fix logo link in README.md to point to the develop branch. (#6969) * Compiler flag handlers (#6415) This adds the ability for packages to apply compiler flags in one of three ways: by injecting them into the compiler wrapper calls (the default in this PR and previously the only automated choice); exporting environment variable definitions for variables with corresponding names (e.g. CPPFLAGS=...); providing them as arguments to the build system (e.g. configure). When applying compiler flags using build system arguments, a package must implement the 'flags_to_build_system_args" function. This is provided for CMake and autotools packages, so for packages which subclass those build systems, they need only update their flag handler method specify which compiler flags should be specified as arguments to the build system. Convenience methods are provided to specify that all flags be applied in one of the 3 available ways, so a custom implementation is only required if more than one method of applying compiler flags is needed. This also removes redundant build system definitions from tutorial examples * Fix type issues with setting flag handlers (#6960) The flag_handlers method was being set as a bound method, but when reset in the package.py file it was being set as an unbound method (all python2 issues). This gets the underlying function information, which is the same in either case. The bug was uncovered for parmetis in #6858. This is a partial fix. Included are changes to the parmetis package.py file to make use of flag_handlers. * Bump version to 0.11.1 * Added flags to unit tests + OSX build done once per day (#6988) * Adding flags to codecov reports * OSX builds are triggered once a day * Pull R list_urls from upstream. * travis: removed /usr/local/include/c++ before installing gcc on OSX (#6515) (#7027) "brew install gcc" fails for travis build because of an existing /usr/local/include/c++. This commit removes the offending file as suggested by brew. * Fix gfortran 7 detection (#7017) * Add NameError to exceptions caught from configure_args in module generation (#7173) * Revert "Binary caching: remove symlinks, copy files instead (#9747)" This reverts commit 058cf81. * Make Spack relocate text files in build caches with relative binaries * add the tfel package * fix the tfel package * fix the tfel package * fix the tfel package * Taking Adam J. Steward' remarks into account * fixes trailing white spaces * Update description * Update dependencies following @adamjstewart adices * Style fixes * Style fixes * Add java optional support * add the maintainers attribute (following @alalazo advice), disable interface not selected (following @adamjstewart advice) * flake8 fixes * Fix Cast3M and python-bindings support. Python detection is made compatible with cmake'FindPythonLibs module (at least how it is used in TFEL) * Style fixes * Style fixes * Fix test on python version * Follow @adamjstewart advices: code is much cleaner and readable * Small fix * Small fix * Add comment * Small fix in cmake option * try again (trying to overcome Travis CI unstable build process) * Add support for the MFrontGenericInterfaceSupport project (MGIS) * Style fixes * Package documentation update * Package documentation update * Fix a typo thanks to Andreas Baumbach review * Follow Adam J. Stewart advices * Fix type * bugfix: add back r's for invalid regexes * tutorial basics section: fix gcc install version * version bump: v0.12.1 * bugfix: bring in .travis.yml from develop * Add new TFEL' versions (3.0.4, 3.1.4 and 3.2.1). Add new MGIS version (1.0.1). Fix MGIS dependency * merge with spack:develop * add missing dependency * new versions of and * Fix MGIS url. Fix duplicate variant in TFEL * Fix tfel packaging according to Adam J. Stewart' advices * Fix flake8 warning Co-authored-by: Massimiliano Culpo <massimiliano.culpo@gmail.com> Co-authored-by: Todd Gamblin <tgamblin@llnl.gov> Co-authored-by: Peter Scheibel <scheibel1@llnl.gov> Co-authored-by: Greg Becker <becker33@llnl.gov> Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>

This adds the ability for packages to apply compiler flags in one of three ways: by injecting them into the compiler wrapper calls (the default in this PR and previously the only automated choice); exporting environment variable definitions for variables with corresponding names (e.g. CPPFLAGS=...); providing them as arguments to the build system (e.g. configure). When applying compiler flags using build system arguments, a package must implement the 'flags_to_build_system_args" function. This is provided for CMake and autotools packages, so for packages which subclass those build systems, they need only update their flag handler method specify which compiler flags should be specified as arguments to the build system. Convenience methods are provided to specify that all flags be applied in one of the 3 available ways, so a custom implementation is only required if more than one method of applying compiler flags is needed. This also removes redundant build system definitions from tutorial examples

becker33 requested review from scheibelp and tgamblin November 23, 2017 00:09

scheibelp reviewed Nov 29, 2017

View reviewed changes

scheibelp reviewed Dec 2, 2017

View reviewed changes

becker33 force-pushed the flag-handlers branch 2 times, most recently from ad2bea9 to 090a2dd Compare December 7, 2017 03:00

scheibelp requested changes Dec 15, 2017

View reviewed changes

scheibelp mentioned this pull request Dec 19, 2017

HYPRE : Add compiler flag args for HYPRE build when using MPI #6716

Closed

becker33 added 13 commits December 18, 2017 17:32

Refactoring of flag handlers

8aec9c0

add tests for flag handlers

8edef56

Remove side effects from cc tests that affected flag handler tests

c8e0c21

Remove vestigial method from previous flag handler implementation

2e48041

Remove duplicate code from build systems tutorial to keep up-to-date …

15f781b

…with code

Update documentation for refactoring of flag_handlers and remove beta…

f075e15

… warning

remove ineffective module scoping attempt for builtin flag handlers

1d26c26

flake

b02d735

removed debugging print statements

86e17b3

code cleanup

eb720d3

code clarity and documentation fixes

2a4ecf8

Clarity edits: Address review concerns about documentation clarity

366e4c8

cleanup: better python idiom for None checks

778b268

becker33 force-pushed the flag-handlers branch from 090a2dd to 778b268 Compare December 19, 2017 01:54

scheibelp mentioned this pull request Dec 19, 2017

IPOPT : Add empty compiler flags for IPOPT build to use spack flags #6714

Merged

scheibelp reviewed Dec 19, 2017

View reviewed changes

Addressed review comments

7796a13

scheibelp reviewed Dec 20, 2017

View reviewed changes

updated two comments to match changes to function names

b837dc9

flake

23cb8cb

scheibelp merged commit 28d8784 into develop Dec 20, 2017

scheibelp mentioned this pull request Jan 2, 2018

How to build Trilinos with cxxflags=-O3 #6769

Closed

adamjstewart mentioned this pull request Jan 4, 2018

Travis failing: Unable to locate perl command #6830

Closed

tgamblin deleted the flag-handlers branch January 17, 2018 05:26

alalazo mentioned this pull request Jan 19, 2018

FFTW installed with Spack 0.11.0 gets built without optimisations #6999

Closed

alalazo mentioned this pull request Apr 14, 2018

per-package setting of compiler flags #3145

Closed

adamjstewart mentioned this pull request Aug 22, 2020

autotools: set 'ldlibs' as 'LIBS' #17254

Merged

		@@ -103,7 +103,7 @@ This will open the :code:`AutotoolsPackage` file in your text editor.
		long examples. We only show what is relevant to the packager.


		.. literalinclude:: tutorial/examples/Autotools/autotools_class.py

Flag handlers #6415

Flag handlers #6415

Conversation

becker33 commented Nov 22, 2017

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becker33 Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becker33 commented Dec 7, 2017

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becker33 Dec 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becker33 commented Dec 20, 2017

scheibelp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

becker33 commented Dec 20, 2017

becker33 Dec 1, 2017 •

edited

Loading

becker33 Dec 20, 2017 •

edited

Loading