Skip to content

Avoid re-concretizing if the spec is already concrete#20192

Closed
scottwittenburg wants to merge 1 commit intospack:developfrom
scottwittenburg:buildcache-avoid-unwanted-concretization
Closed

Avoid re-concretizing if the spec is already concrete#20192
scottwittenburg wants to merge 1 commit intospack:developfrom
scottwittenburg:buildcache-avoid-unwanted-concretization

Conversation

@scottwittenburg
Copy link
Copy Markdown
Contributor

This fixes a problem we could see where asking spack to check a buildcache using a hash resulted in re-concretization and subsequent checking of a buildcache with a different hash.

E.g.:

spack buildcache check --rebuild-on-error --mirror-url https://some.location/mirror -s "/yrnbhxf"
...
==> [2020-11-30-23:29:12.399675] Error: Unable to determine whether [email protected]%[email protected]+cuda~ipo+openmp~rocm~static build_type=Release arch=linux-ubuntu20.04-x86_64/2b2hcaq needs rebuilding, caught exception attempting to read from https://some.location/mirror/build_cache/linux-ubuntu20.04-x86_64-gcc-9.3.0-spla-1.2.1-2b2hcaqr2khzjvsu27ek767o3fnjdbtm.spec.yaml.

Notice how spack actually looked for /2b2hcaq rather than /yrnbhxf, which was what we asked it to look for.

@scottwittenburg
Copy link
Copy Markdown
Contributor Author

@haampie For me, this fixed the issue we were digging into this evening. Hopefully it doesn't cause a bunch of test failures 😉

@haampie
Copy link
Copy Markdown
Member

haampie commented Dec 1, 2020

Looks good, but needs a test

@alalazo
Copy link
Copy Markdown
Member

alalazo commented Dec 1, 2020

Which issue is this PR solving? Is that with the new concretizer or the original one?

@haampie
Copy link
Copy Markdown
Member

haampie commented Dec 1, 2020

#20191, I used to do buildcache check -s /hash which now reconcretizes to something different, so I have provided the correct hash, but it rehashes to something else during the check.

Comment on lines +646 to +647
if not spec.concrete:
spec.concretize()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the full context, but this seems to be covering up a bug. My reasoning is that concretization should reach a fixed point i.e. for a concrete spec spec.concretize() should be a no-op.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method could have been called with a spec made from a .spec.yaml fetched from an arbitrary binary mirror, and in that case, isn't it true that re-concretization could change it completely?

However, it looks like #20196 is a better way to prevent this behavior from happening in any possible code path.

@haampie
Copy link
Copy Markdown
Member

haampie commented Dec 1, 2020

Seems like the hash mismatch is gone when adding -e ci:

$ spack -e ci buildcache check --rebuild-on-error --mirror-url https://some.location/mirror -s "/yrnbhxf"
==> Error: Unable to determine whether [email protected]%[email protected]+cuda~ipo+openmp~rocm~static build_type=Release arch=linux-ubuntu20.04-x86_64/yrnbhxf needs rebuilding, caught exception attempting to read from fileL////wut/build_cache/linux-ubuntu20.04-x86_64-gcc-9.3.0-spla-1.2.1-yrnbhxfafz5khc5k4rp3ckutdgg4xqxb.spec.yaml.

but I still think it shouldn't reconcretize a concrete spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants