Fix type issues with setting flag handlers by becker33 · Pull Request #6960 · spack/spack

becker33 · 2018-01-17T00:13:58Z

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 in either case, which is the same.

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.

(The other part of the fix is in #6970.)

KineticTheory · 2018-01-17T02:25:17Z

👍 I like this solution.

Also -- nice work finding those python2 issues. I don't think I could have figured out that conflict.

scheibelp

I have a request for additional testing (although I may have missed something - see the review comment)

scheibelp · 2018-01-17T05:20:56Z

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])
+        if isinstance(pkg.flag_handler, types.FunctionType):


This looks like it preps for the following possibilities:

A package implements flag_handler

A package sets flag_handler = ...

Are both approaches tested? If not, should they both be tested?

Also IMO it's worth mentioning what this is addressing here - e.g. add a comment like

# Address the case where the package chooses a flag handler using a statement like "flag_handler =..."

@scheibelp There are three types of "function" like objects in python2, and two types in python3.

In python2, there are 2 types of methods, "bound" and "unbound", and one type "function". The method.im_func attribute returns a function type object from either a bound or unbound method. I previously tested only converting from a bound method, although the conversion is the same. I've added a test for unbound methods.

In python3 there are not unbound methods. The testing already covers both functions and methods in python3 speak.

I don't think an additional test was necessary, because the conversion to function type works for both bound and unbound methods, so once we changed everything to function type they behave identically. However, writing the additional test took way less time than writing this comment, so I just added it anyway. More tests can't hurt.

@becker33 sorry could you also add a comment here mentioning that this addresses the different possible ways a user can set up their flag_handler method? Other than that this LGTM

becker33 · 2018-01-18T22:33:38Z

@scheibelp review addressed / comment added.

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.

@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 <[email protected]> Co-authored-by: Todd Gamblin <[email protected]> Co-authored-by: Peter Scheibel <[email protected]> Co-authored-by: Greg Becker <[email protected]> Co-authored-by: Adam J. Stewart <[email protected]>

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 spack#6858. This is a partial fix. Included are changes to the parmetis package.py file to make use of flag_handlers.

Fix type issues with setting flag handlers -- parmetis as example

510e752

becker33 requested a review from scheibelp January 17, 2018 00:14

becker33 mentioned this pull request Jan 17, 2018

Parmetis (CMakePackage) doesn't always respect cflags #6858

Closed

This was referenced Jan 17, 2018

WIP: Help the parmetis recipe respect spack's cflags and cxxflags. #6860

Closed

WIP: Teach the metis recipe to respect spack's cflags #6840

Closed

metis doesn't always respect cflags #6839

Closed

scheibelp requested changes Jan 17, 2018

View reviewed changes

becker33 added 2 commits January 17, 2018 10:03

flake

0351d85

explicitly test unbound methods (python2)

8763220

becker33 closed this Jan 18, 2018

becker33 reopened this Jan 18, 2018

Add comment on function type conversion

976589f

scheibelp approved these changes Jan 19, 2018

View reviewed changes

scheibelp merged commit 3686c25 into develop Jan 19, 2018

tgamblin deleted the flag-handlers-unbound-parmetis branch January 29, 2018 01:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix type issues with setting flag handlers#6960

Fix type issues with setting flag handlers#6960
scheibelp merged 4 commits intodevelopfrom
flag-handlers-unbound-parmetis

becker33 commented Jan 17, 2018 •

edited

Loading

Uh oh!

KineticTheory commented Jan 17, 2018

Uh oh!

scheibelp left a comment

Uh oh!

scheibelp Jan 17, 2018

Uh oh!

becker33 Jan 17, 2018 •

edited

Loading

Uh oh!

scheibelp Jan 18, 2018

Uh oh!

becker33 commented Jan 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

becker33 commented Jan 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KineticTheory commented Jan 17, 2018

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

scheibelp Jan 17, 2018

Choose a reason for hiding this comment

Uh oh!

becker33 Jan 17, 2018 • 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!

scheibelp Jan 18, 2018

Choose a reason for hiding this comment

Uh oh!

becker33 commented Jan 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

becker33 commented Jan 17, 2018 •

edited

Loading

becker33 Jan 17, 2018 •

edited

Loading