Flags and hashes by becker33 · Pull Request #4939 · spack/spack

becker33 · 2017-08-01T01:14:40Z

This fixes an issue reported by @mamelara on the slack channel.

Previously, packages specified by hash would inherit additional compiler flags in the spec from their parents, and would therefore rebuild.

Example of previous behavior, assuming there is an installation of libelf with hash beginning lgv built with cflags="-g -O0":

$ spack spec libdwarf ldflags=-g ^\lgv
Input spec
--------------------------------
libdwarf ldflags="-g" 
    ^libelf@0.8.13%gcc@4.4.7 cflags="-g -O0"  arch=linux-rhel6-x86_64 

Normalized
--------------------------------
libdwarf ldflags="-g" 
    ^libelf@0.8.13%gcc@4.4.7 cflags="-g -O0"  arch=linux-rhel6-x86_64 

Concretized
--------------------------------
libdwarf@20160507%gcc@4.4.7 ldflags="-g"  arch=linux-rhel6-x86_64 
    ^libelf@0.8.13%gcc@4.4.7 cflags="-g -O0"  ldflags="-g"  arch=linux-rhel6-x86_64

Fixed behavior:

$ spack spec libdwarf ldflags=-g ^/lgv
Input spec
--------------------------------
libdwarf ldflags="-g" 
    ^libelf@0.8.13%gcc@4.4.7 cflags="-g -O0"  arch=linux-rhel6-x86_64 

Normalized
--------------------------------
libdwarf ldflags="-g" 
    ^libelf@0.8.13%gcc@4.4.7 cflags="-g -O0"  arch=linux-rhel6-x86_64 

Concretized
--------------------------------
libdwarf@20160507%gcc@4.4.7 ldflags="-g"  arch=linux-rhel6-x86_64 
    ^libelf@0.8.13%gcc@4.4.7 cflags="-g -O0"  arch=linux-rhel6-x86_64

becker33 · 2017-08-01T01:17:04Z

@mamelara please confirm this fixes the problem in your use case? I think my reproducer captured what was going on, but it's always good to check.

scheibelp · 2017-08-01T01:19:33Z

I'll be able to look through this before Wednesday (8/2)

scheibelp

Overall I would have preferred an approach which explicitly checks if the spec package is already installed and just avoids concretizing in that case. Is there a reason that would be problematic? I have a few notes on issues that I think would come up (but apparently aren't tested).

scheibelp · 2017-08-01T20:25:14Z

lib/spack/spack/spec.py

        sorted_keys = [k for k in sorted(self.keys()) if self[k] != []]
        cond_symbol = ' ' if len(sorted_keys) > 0 else ''
        return cond_symbol + ' '.join(
-            str(key) + '=\"' + ' '.join(


Why are the escape quotes no longer needed?

Because we no longer use escape quotes on the command line, and the string representation only had them to match the command line syntax. This was really a change that was missed in a previous PR of mine that I noticed while working on this one.

scheibelp · 2017-08-01T20:59:46Z

lib/spack/spack/concretize.py

        ret = False
        for flag in spack.spec.FlagMap.valid_compiler_flags():
            if flag not in spec.compiler_flags:
-                spec.compiler_flags[flag] = list()


Since all this logic was moved into the "if", I don't see how this can add flags for a given category if there are already flags defined in the spec. I suppose a test must be missing for this. This seems to be the primary change that avoids adding compiler flags to already-installed specs. I would prefer a more explicit approach, for example add a check at the beginning of this method like if spec.package.installed: return

Overall my understanding is that the original complaint on the slack channel had to do with flags being added to already-installed packages; the initial check for spec._concrete resolves this and the additional changes in this function are not necessary to deal with that problem. That being said my understanding is that this logic is intended to have the following effects:

Ensure that you do not merge flags from different compilers

Ensure that when a user explicitly sets compiler flags on a package, that flags from other compilers will not add to it

IMO this could benefit from an added unit test.

scheibelp · 2017-08-01T21:15:31Z

lib/spack/spack/concretize.py

+                        ret = True

        # Include the compiler flag defaults from the config files
        # This ensures that spack will detect conflicts that stem from a change


Since this logic hasn't changed, is it possible that compiler flags will be added from the default compiler spec to the already-installed dependency?

Not from the default compiler. But you're right, it could add flags to the spec if the flags for the compiler used to build that package have changed.

Bailing out at the beginning of the function if spec._concrete resolves my original concern here. But I wanted to make sure: it was your intent that the compiler definition be able to merge into compiler flags explicitly set by the user (although above in this PR you don't want to merge flags from other compiler specs)?

scheibelp · 2017-08-01T21:19:35Z

lib/spack/spack/concretize.py

+                    flags = set()
+
+                    if (nearest_flags - flags) or n.compiler_flags[flag] == []:
+                        spec.compiler_flags[flag] = list(nearest_flags | flags)


-> = list(nearest_flags)

IMO this should still be changed (flags is an empty set)

scheibelp · 2017-08-01T21:20:33Z

lib/spack/spack/concretize.py

+                    nearest_flags = set(n.compiler_flags[flag])
+                    flags = set()
+
+                    if (nearest_flags - flags) or n.compiler_flags[flag] == []:


I would prefer or flag in n.compiler_flags to or n.compiler_flags[flag] == []. For that matter, if I'm right that flag in n.compiler_flags implies that n.compiler_flags is a nonempty list, then I think you can strip the "if" altogether and do the next line unconditionally.

Those statements are not equivalent. flag in n.compiler_flags returns True iff there is no entry for flag in n.compiler_flags, while n.compiler_flags[flag] == [] returns True iff there is an entry for flag in n.compiler_flags and that entry is [].

If it is true that flag in n.compiler_flags what are the possible values of n.compiler_flags[flag]? I was assuming it would be a (possibly empty) container of some sort. Can it be None?

It being empty means something different than it not being present. Not present means it hasn't been concretized, empty means it has been concretized to empty.

From the logic preceding this statement, we know that flag in n.compiler_flags == True

My understanding is that if flag in n.compiler_flags, then it is a list (possibly empty).

If these two statements are correct, then based on how we arrived at this point, if nearest_flags or n.compiler_flags[flag] == [] is always true, and the if isn't required.

On top of that, based on the fact that you moved all of this into the if statement, the general logic is "set the flags here if they weren't set before"; but if we are at this point then we know that that self.compiler_flags hasn't been set.

scheibelp · 2017-08-01T21:40:47Z

Also I was having trouble replicating results when pulling this branch - If I install libdwarf with spack install libdwarf I see:

==> elfutils is already installed in /g/g17/scheibel/repos/destdir/spack/opt/spack/linux-rhel6-x86_64/gcc-4.4.7/elfutils-0.163-gcsyryoutwmy22qzazcz6otmlvqyuzzg

But if I then try the suggestion I get an error:

bash-4.1$ spack spec libdwarf ldflags=-g ^\gcs
Input spec
--------------------------------
libdwarf ldflags="-g" 
    ^gcs

Normalized
--------------------------------
==> Error: libdwarf does not depend on gcs

Could you let me know if I missed something there? Thanks!

becker33 · 2017-08-04T18:00:39Z

In your attempted reproducer you used a backslash \, which is interpreted by the shell and discarded, instead of a slash /. That should be the entire problem with it.

becker33 · 2017-09-18T17:42:51Z

@scheibelp I added the explicit concreteness checks we discussed.

scheibelp · 2017-09-19T02:08:09Z

I'll be able to look at this tomorrow (9/19)

scheibelp

I have a couple questions, a couple requests for simplifying the logic, and a request for a unit test.

Most of that goes away if concretize_compiler_flags is changed to only have the initial check for spec._concrete (vs. the additional changes in it). The additional changes are fine but I wanted to clarify the intent there since I don't think they are strictly required to handle the bug this was originally intended to address. Copied from the relevant comment:

My understanding is that this logic is intended to have the following effects:

Ensure that you do not merge flags from different compilers
Ensure that when a user explicitly sets compiler flags on a package, that flags from other compilers will not add to it

IMO this could benefit from an added unit test.

scheibelp · 2017-09-20T02:56:02Z

lib/spack/spack/concretize.py

-        compiler_match = lambda other: (
-            spec.compiler == other.compiler and
-            spec.architecture == other.architecture)
+        def compiler_match(other, strict=True):


IMO this shouldn't take a strict argument (I talk more about that where this function is called with strict=False) - i.e. I think the definition should stay the same.

Additionally: Now that there is a spec._concrete check at the beginning, I'm not sure that these changes are required now.

If you disagree, then IMO this could be more-succinctly written (in a way that makes it clearer):

compiler_matches = (spec.compiler == other.compiler and spec.arch == other.arch) other_compiler_is_concrete = other.compiler and other.compiler.concrete return (strict and compiler_matches) or not other_compiler_is_concrete

scheibelp · 2017-09-20T03:09:19Z

lib/spack/spack/concretize.py

+                    flags = set()
+
+                    if (nearest_flags - flags) or n.compiler_flags[flag] == []:
+                        spec.compiler_flags[flag] = list(nearest_flags | flags)


IMO this should still be changed (flags is an empty set)

scheibelp · 2017-09-20T03:10:38Z

lib/spack/spack/concretize.py

+                    nearest_flags = set(n.compiler_flags[flag])
+                    flags = set()
+
+                    if (nearest_flags - flags) or n.compiler_flags[flag] == []:


From the logic preceding this statement, we know that flag in n.compiler_flags == True

My understanding is that if flag in n.compiler_flags, then it is a list (possibly empty).

If these two statements are correct, then based on how we arrived at this point, if nearest_flags or n.compiler_flags[flag] == [] is always true, and the if isn't required.

On top of that, based on the fact that you moved all of this into the if statement, the general logic is "set the flags here if they weren't set before"; but if we are at this point then we know that that self.compiler_flags hasn't been set.

scheibelp · 2017-09-20T03:17:49Z

lib/spack/spack/concretize.py

+                except StopIteration:
+                    try:
+                        n = next(p for p in spec.traverse(direction='parents')
+                                 if (compiler_match(p, False) and


Given the definition of compiler_match and the prior checks performed. This would be equivalent to

if any((not x.compiler or not x.compiler.concrete) for x in spec.traverse(direction='parents'))

And:

would be clearer

avoids nested try/catch (not an error, but also makes things simpler)

scheibelp · 2017-09-20T03:19:42Z

lib/spack/spack/concretize.py

+                        ret = True

        # Include the compiler flag defaults from the config files
        # This ensures that spack will detect conflicts that stem from a change


Bailing out at the beginning of the function if spec._concrete resolves my original concern here. But I wanted to make sure: it was your intent that the compiler definition be able to merge into compiler flags explicitly set by the user (although above in this PR you don't want to merge flags from other compiler specs)?

scheibelp · 2017-09-20T03:22:41Z

lib/spack/spack/concretize.py

        ret = False
        for flag in spack.spec.FlagMap.valid_compiler_flags():
            if flag not in spec.compiler_flags:
-                spec.compiler_flags[flag] = list()


Overall my understanding is that the original complaint on the slack channel had to do with flags being added to already-installed packages; the initial check for spec._concrete resolves this and the additional changes in this function are not necessary to deal with that problem. That being said my understanding is that this logic is intended to have the following effects:

Ensure that you do not merge flags from different compilers

Ensure that when a user explicitly sets compiler flags on a package, that flags from other compilers will not add to it

IMO this could benefit from an added unit test.

becker33 · 2019-04-23T09:29:31Z

Closing: This is superceded by #5580

becker33 added 8 commits July 31, 2017 14:48

Fix compiler flag concretization for concrete deps

a5fec42

Fix test to match

d9b4559

flake

95bf2d9

test for this error

328514f

fix bug found in testing

2fafd7d

code cleanup

15843c7

more cleanup and better comments

caee97b

more code cleanup

3c70905

becker33 requested review from mamelara and scheibelp August 1, 2017 01:17

flake and testing fix

17f8206

scheibelp reviewed Aug 1, 2017

View reviewed changes

adamjstewart added hashes specs labels Aug 3, 2017

Add explicit checks for concreteness in concretizer

8aaea80

scheibelp requested changes Sep 20, 2017

View reviewed changes

scheibelp mentioned this pull request Oct 3, 2017

Don't change properties on already-installed packages #5580

Merged

becker33 closed this Apr 23, 2019

tgamblin deleted the flags-and-hashes branch September 3, 2019 05:31

Conversation

becker33 commented Aug 1, 2017

Uh oh!

becker33 commented Aug 1, 2017

Uh oh!

scheibelp commented Aug 1, 2017

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp Aug 4, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scheibelp commented Aug 1, 2017

Uh oh!

becker33 commented Aug 4, 2017

Uh oh!

becker33 commented Sep 18, 2017

Uh oh!

scheibelp commented Sep 19, 2017

Uh oh!

scheibelp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

becker33 commented Apr 23, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

scheibelp Aug 4, 2017 •

edited

Loading