Add full_hash and json index to buildcache by scottwittenburg · Pull Request #8451 · spack/spack

scottwittenburg · 2018-06-12T20:53:27Z

No description provided.

scottwittenburg · 2018-06-12T20:55:29Z

I have made these changes to support the work I'm doing in #8444, but the topic may need adjustments to make sure its sensible for everyone else.

Per our discussion this morning @tgamblin @opadron @aashish24 @bryonbean

tgamblin

This looks good, but I think the file should be json, and the index shouldn't be optional.

tgamblin · 2018-07-03T15:49:56Z

lib/spack/spack/binary_distribution.py

    f.close()


+def generate_yaml_index(pathList, outputPath):


this is a good start but likely needs a schema like the other YAML files. Also, I think json is actually better for things to be read by computers (not people). Does your checkbinaries command actually look at this YAML file or just the HTML?

I'll change it to json, do we already have json schemas somewhere in the repo? Once I have it change to json, I'll update the check-binaries code to read that instead.

tgamblin · 2018-07-03T15:50:57Z

lib/spack/spack/cmd/buildcache.py

    create.add_argument('-d', '--directory', metavar='directory',
                        type=str, default='.',
                        help="directory in which to save the tarballs.")
+    create.add_argument('-i', '--include_full_hash', action='store_true',


I would just do this and the index by default. Having them there should not hurt.

Sounds good.

scottwittenburg · 2018-08-09T19:00:57Z

@tgamblin Except for some issue I ran into (described in #8929), this is working and seems pretty close. I have addressed the issues you brough up, and I think it's time for a second look whenever you get a chance. Thanks!

scottwittenburg · 2018-08-20T18:14:51Z

fyi @zackgalbreath

tgamblin

@scottwittenburg: LGTM mostly! I have some minor requests and then I'm curious why you're seeing specs with patches but without _patches_in_order_of_appearance. Are they concrete?

tgamblin · 2018-08-21T06:23:17Z

lib/spack/spack/binary_distribution.py

+            os.remove(p)
+
+    generate_html_index(path_list, index_html_path)
+    generate_json_index(path_list, index_json_path)


instead of removing then generating in-place, can you generate to a .index.json.tmp file, and then move that on top of index.json? That way the change is atomic and you don't risk failing with missing or partial indices. The local name/suffix (as opposed to tmp or something) guarantees it's on the same filesystem, so a move will be atomic.

Sounds good, I'll do it.

tgamblin · 2018-08-21T06:35:27Z

lib/spack/spack/binary_distribution.py

    buildinfo['relative_prefix'] = os.path.relpath(
        spec.prefix, spack.store.layout.root)
    spec_dict['buildinfo'] = buildinfo
+    spec.concretize()


Is this needed? I would assume that any spec passed to build_tarball is concrete -- probably needs an assert or at least a check and a raised ValueError -- or are you relying on the spec to be concretized here?

It seemed the spec was not concrete here, and then computing the full_hash in the next line would throw an exception. As this was the last possible moment before the spec information was written out to the .spec.yaml, I assumed that if it hadn't been done by this point, then I should do it myself.

tgamblin · 2018-08-21T06:40:47Z

lib/spack/spack/cmd/buildcache.py

    create.add_argument('-d', '--directory', metavar='directory',
                        type=str, default='.',
                        help="directory in which to save the tarballs.")
+    create.add_argument('-s', '--skip_rebuild_index', action='store_true',


Can this be: --no-rebuild-index and have no short argument? Seems like it would be used infrequently or only in scripts (where you want readability), but maybe I'm underestimating. Also I dunno why allow_root has an underscore -- but nearly all the other commands in Spack use -.

tgamblin · 2018-08-21T06:43:30Z

lib/spack/spack/spec.py


+        variants_patches = self.variants['patches']
+
+        if not hasattr(variants_patches, '_patches_in_order_of_appearance'):


why was this needed? Are you seeing specs without it?

Yes, sometimes I was getting an exception without this (I believe it was when I was testing creating a buildcache entry after I made the change which added the full_hash). Perhaps it was just something about the state of my repo and local configs at the time? At any rate, sometimes variants_patches did not have a _patches_in_order_of_appearance attribute, so I just guarded it.

@tgamblin I found some notes where I had documented (kind of) the issue that was happening here. It happened when creating a buildcache entry:

==> creating binary cache file for package [email protected]%[email protected]+dbm~optimizations patches=123082ab3483ded78e86d7c809e98a804b3465b4683c96bd79a2fd799f572244 +pic+pythoncmd+shared~tk~ucs4 arch=linux-ubuntu16.04-x86_64 ==> Error: 'MultiValuedVariant' object has no attribute '_patches_in_order_of_appearance'

I have checked out the current develop branch, and tried creating a spec for python similar to the one shown above, then concretizing and full_hash-ing it, but I could not reproduce the issue.

I wonder if the thing to do is to remove the change from my PR until such time as I encounter the issue again? Let me know what you think.

Also create an index.json in addition to the normal index.html file.

scottwittenburg · 2019-01-07T23:54:46Z

Closing in favor of #10274

tgamblin self-requested a review June 15, 2018 03:39

tgamblin added buildcache releases labels Jun 26, 2018

tgamblin mentioned this pull request Jun 26, 2018

add CombinatorialSpecSet class for taking cross-products of Specs. #8574

Closed

1 task

tgamblin requested changes Jul 3, 2018

View reviewed changes

scottwittenburg mentioned this pull request Jul 3, 2018

WIP: Package hash check binaries #8444

Closed

scottwittenburg force-pushed the buildcache-add-fullhash-and-yaml-index branch 2 times, most recently from 340b220 to 6caac36 Compare July 10, 2018 17:36

scottwittenburg force-pushed the buildcache-add-fullhash-and-yaml-index branch from 6caac36 to bc25fa1 Compare August 8, 2018 22:47

scottwittenburg changed the title ~~Add full_hash and yaml index to buildcache~~ Add full_hash and json index to buildcache Aug 8, 2018

scottwittenburg force-pushed the buildcache-add-fullhash-and-yaml-index branch 2 times, most recently from 03c9a4a to aab717c Compare August 9, 2018 16:36

tgamblin requested changes Aug 21, 2018

View reviewed changes

scottwittenburg force-pushed the buildcache-add-fullhash-and-yaml-index branch 2 times, most recently from 2cef5db to 2d1eb61 Compare August 23, 2018 15:35

scottwittenburg added 2 commits August 23, 2018 10:57

spec: Guard against using missing attribute

c6d2201

buildcache: Add full_hash to stored pkg yaml

1eea39c

Also create an index.json in addition to the normal index.html file.

scottwittenburg force-pushed the buildcache-add-fullhash-and-yaml-index branch from 2d1eb61 to 1eea39c Compare August 23, 2018 17:06

gartung approved these changes Oct 12, 2018

View reviewed changes

gartung mentioned this pull request Oct 22, 2018

Fix bug getting specs from build caches #9600

Merged

scottwittenburg mentioned this pull request Jan 7, 2019

Release workflow #10274

Merged

3 tasks

scottwittenburg closed this Jan 7, 2019


		variants_patches = self.variants['patches']

		if not hasattr(variants_patches, '_patches_in_order_of_appearance'):

Conversation

scottwittenburg commented Jun 12, 2018

Uh oh!

scottwittenburg commented Jun 12, 2018

Uh oh!

tgamblin 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!

scottwittenburg commented Aug 9, 2018

Uh oh!

scottwittenburg commented Aug 20, 2018

Uh oh!

tgamblin 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!

scottwittenburg commented Jan 7, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants