Major improvements to spack create by adamjstewart · Pull Request #2707 · spack/spack

adamjstewart · 2016-12-31T23:18:38Z

This is a first stab at fulfilling #1108. spack create previously required a URL. If one was not present, or could not be parsed or fetched, it was useless. This resulted in many users being forced to use spack edit -f or manually copy packages by hand. I wanted to draw a line between the two commands. spack create is used to create new packages, spack edit is used to edit existing packages. In order to remove spack edit -f, I needed to allow spack create to work even if a URL does not exist.

The following commands demonstrate the new abilities of spack create:

$ spack create
# create an example package

$ spack create --name foobar
# create a package called foobar

$ spack create --template cmake
# create an example CMake package

$ spack create --force --template python --name numpy
$ spack create --force --template python --name py-numpy
# create a python package, both commands create the same package

$ spack create --template autotools <url>
# convenient when the tarball doesn't contain a configure script and autoreconf needs to be run

I noticed a lot of duplicated code between spack create and spack checksum. For example, all of the logic that searches for available versions, displays them and asks the user how many they would like to checksum, and formats the output versions was duplicated. I un-duplicated them.

Remove duplicated logic in spack create and spack checksum
Add --template flag to spack create
Allow spack create to work even if a URL is not specified
Document new features of spack create
Remove spack edit --force

We can change the UI from spack create --name <name> <url> to spack create --url <url> <name> in another PR, as that seemed like a more controversial change.

adamjstewart · 2016-12-31T23:30:13Z

lib/spack/spack/cmd/checksum.py

+    version_lines = "\n".join([
+        "    version('{0}', {1}'{2}')".format(
+            v, ' ' * (max_len - len(str(v))), h) for v, h in version_hashes
+    ])


Previously, spack checksum tau would result in:

version('2.26', '2af91f02ad26d5bf0954146c56a8cdfa') version('2.25.2', 'f5e542d41eb4a7daa6241e5472f49fd7') version('2.25.1.1', 'f2baae27c5c024937566f33339826d7c') version('2.25.1', 'b9783f9bbe2862254bfdd208735241b6') version('2.25', '46cd48fa3f3c4ce0197017b3158a2b43')

while

$ spack create --force https://www.cs.uoregon.edu/research/tau/tau_releases/tau-2.25.tar.gz

would result in:

version('2.26' , '2af91f02ad26d5bf0954146c56a8cdfa') version('2.25.2' , 'f5e542d41eb4a7daa6241e5472f49fd7') version('2.25.1.1', 'f2baae27c5c024937566f33339826d7c') version('2.25.1' , 'b9783f9bbe2862254bfdd208735241b6') version('2.25' , '46cd48fa3f3c4ce0197017b3158a2b43')

The latter isn't even PEP 8 compliant, and would raise problems during the flake8 tests. Now, both result in:

version('2.26', '2af91f02ad26d5bf0954146c56a8cdfa') version('2.25.2', 'f5e542d41eb4a7daa6241e5472f49fd7') version('2.25.1.1', 'f2baae27c5c024937566f33339826d7c') version('2.25.1', 'b9783f9bbe2862254bfdd208735241b6') version('2.25', '46cd48fa3f3c4ce0197017b3158a2b43')

adamjstewart · 2016-12-31T23:30:39Z

lib/spack/spack/cmd/checksum.py

+    # Make sure the user provided a package and not a URL
+    if not valid_fully_qualified_module_name(args.package):
+        tty.die("`spack checksum` accepts package names, not URLs. "
+                "Use `spack md5 <url>` instead.")


I accidentally do this all the time.

If you know what the user wants, why not just do it instead of quitting with an error message? Do we need spack md5 at all, or can its functionality be folded into spack checksum?

Does it make sense to rename spack checksum? We will not always use md5, and it would be nice if spack checksum gave you whatever the current recommended checksum of a file/url was. Should spack checksum be called spack find-new-versions or spack-spider-versions or something?

You mean rename spack md5 or rename spack checksum? I don't have any particular preference. Let's leave that for another PR.

I mean:

spack checksum -> spack find-new-versions or some better name.

spack md5 -> spack checksum (and it might spit out a sha256)

Another PR sounds good.

Yeah, there's a lot of overlap between spack versions/checksum/md5. We can work on that during the switch to sha256 or whatever we decide on.

adamjstewart · 2016-12-31T23:31:53Z

lib/spack/spack/cmd/create.py

-        format = "    version(%%-%ds, '%%s')" % (max_len + 2)
-        return '\n'.join(
-            format % ("'%s'" % v, h) for v, h in self.version_hash_tuples
-        )


This is now done in spack checksum. No need to duplicate the effort.

adamjstewart · 2016-12-31T23:33:15Z

lib/spack/spack/cmd/create.py

-        '-f', '--force', action='store_true', dest='force',
-        help="Overwrite any existing package file with the same name.")
-
-    setup_parser.subparser = subparser


I don't think this line is necessary? I don't see it in any other cmd.

adamjstewart · 2016-12-31T23:33:53Z

lib/spack/spack/cmd/create.py

        '-N', '--namespace',
-        help="Specify a namespace for the package. Must be the namespace of "
-        "a repository registered with Spack.")
+        help="namespace for the package")


Can someone test --repo and --namespace for me? I don't use either, so I can't really test whether or not the behavior has changed.

adamjstewart · 2016-12-31T23:35:57Z

lib/spack/spack/cmd/create.py

-
-    sorted_versions = sorted(versions.keys(), reverse=True)
-    sorted_urls = [versions[v] for v in sorted_versions]
-    return sorted_versions[:archives_to_fetch], sorted_urls[:archives_to_fetch]


This is all duplicated in spack checksum, so I un-duplicated it.

adamjstewart · 2016-12-31T23:41:29Z

lib/spack/spack/cmd/create.py

        # Determine the build system based on the files contained
        # in the archive.
-        build_system = 'unknown'
+        build_system = 'default'


Thoughts on unknown vs. default?

How about 'generic' for the regular old Package?

So the name is only presented to the user in 2 scenarios. One, as a help option for spack create --template, and two, when it prints out "This package looks like it uses the {0} build system". I think --template generic makes more sense, but I also think the generic build system doesn't make much sense. I could reword it of course.

I like the idea of just special casing the method to something like "couldn't detect a build system -- using generic package", since that gives the user more information anyway.

citibeth

I would suggest a grammar of spack create [options] <package>, since that is consistent with other Spack commands. The demo you shared would then become:

$ spack create
# create an example package

$ spack create foobar
# create a package called foobar

$ spack create --template cmake
# create an example CMake package

$ spack create --force --template python numpy
$ spack create --force --template python py-numpy
# create a python package, both commands create the same package

$ spack create --template autotools <url>
# convenient when the tarball doesn't contain a configure script and autoreconf needs to be run

Where was the code deduplicated, and where is it now?

citibeth · 2017-01-01T02:25:01Z

lib/spack/spack/cmd/checksum.py

+    # Make sure the user provided a package and not a URL
+    if not valid_fully_qualified_module_name(args.package):
+        tty.die("`spack checksum` accepts package names, not URLs. "
+                "Use `spack md5 <url>` instead.")


If you know what the user wants, why not just do it instead of quitting with an error message? Do we need spack md5 at all, or can its functionality be folded into spack checksum?

citibeth · 2017-01-01T02:27:13Z

lib/spack/spack/cmd/create.py

-        'scons': SconsGuess,
-        'bazel': BazelGuess,
-        'python': PythonGuess,
-        'R': RGuess,


Let's please merge #2475 before doing this.

adamjstewart · 2017-01-02T02:02:36Z

lib/spack/spack/__init__.py


 # User's editor from the environment
-editor = Executable(os.environ.get("EDITOR", "vi"))
+editor = Executable(os.environ.get("EDITOR", "vim"))


vi is not present on newer distributions like Fedora, and when both are present, vi generally enables legacy mode. How common are distributions that only come with vi?

I don't know. But maybe some embedded or stripped down ones (e.g. for containers)? What about:

if "EDITOR" in os.environ: editor = Executable(os.environ.get("EDITOR")) else: editor = which('vim') if not editor: editor = Executable('vi')

I'm an emacs user -- I just picked vi as the default because it's more likely to be installed, so I guess we should do best effort here.

Let me back out this change and make it in a different PR, just to keep things simple.

adamjstewart · 2017-01-02T02:06:54Z

lib/spack/spack/cmd/edit.py

    subparser.add_argument(
        '-f', '--force', dest='force', action='store_true',
-        help="Open a new file in $EDITOR even if package doesn't exist.")
+        help='deprecated, use `spack create` instead')


Is this deprecated enough for now? Honestly, I would be fine with ripping it out completely in this PR, but I can wait.

I think taking it out is fine if the docs reflect it.

adamjstewart · 2017-01-02T02:09:33Z

I think this is good to go now. We should merge #2475 before this though. I'll rebase on that after it's merged.

Let me know if there are any other places in the documentation or code that reference spack edit --force. I wasn't expecting those calls in spack diy and spack setup.

adamjstewart · 2017-01-03T15:33:28Z

I would suggest a grammar of spack create [options] , since that is consistent with other Spack commands.

That's the plan. But in a separate PR, as that change was more controversial and I don't want it to hold up these changes.

Where was the code deduplicated, and where is it now?

Here and here. It is in spack checksum now where it belongs.

adamjstewart · 2017-01-05T18:31:54Z

@tgamblin Can you look this over at some point? I have a PythonPackage PR and will soon have an RPackage PR that will also modify spack create.

adamjstewart · 2017-01-09T22:47:20Z

Ping @tgamblin

adamjstewart · 2017-01-15T13:49:46Z

Ping ping @tgamblin

tgamblin

@adamjstewart: mostly minor questions/comments. This looks really good to me. See below.

Comments/questions:

Now that the build system guessers are getting complex, does it make sense to make a subpackage for the different kinds, like build_systems, cmd, etc.?
We'll need to figure out whether to merge this or #2780 first.

tgamblin · 2017-01-15T23:45:29Z

lib/spack/docs/packaging_guide.rst

+     5.0.0   https://gmplib.org/download/gmp/gmp-5.0.0.tar.bz2

-   Include how many checksums in the package file? (default is 5, q to abort)
+   How many would you like to checksum? (default is 1, q to abort)


Oh good! I keep thinking to change this to 1.

tgamblin · 2017-01-15T23:47:38Z

lib/spack/docs/packaging_guide.rst


-       # FIXME: Add dependencies if this package requires them.
-       # depends_on("foo")
+       # FIXME: Add dependencies if required.


Can we add a note here for autotools packages, that most autotools packages will not need m4, autoconf, automake, libtool, etc.? I think it was confusing for at least one person. Also does m4 need to be here? It's brought in transitively through the other three.

Not sure about the m4 question. One thing I was considering doing was to create 2 separate build system guessers, one for Autotools with a configure script and one for Autotools without. I presume there are pretty tell-tale filenames that should make it obvious that the package uses Autotools even if it doesn't have a configure script?

Autotools without configure would automatically create an empty autoreconf method and add those 3-4 deps. Autotools with configure wouldn't add those deps, so no one would get confused.

If you can give me a list of tell-tale filenames that would indicate a package uses Autotools without a configure script, I can do this.

I'm kind of tempted to say we should just not bother at that point. I think it'll be confusing to have two autotools packages, and the existing one already has an autoreconf phase. Also, I think it's complicated to do this generically. Some packages use automake but not autoconf, some do it the other way around, and some may or may not use libtool. The particular dependencies you want are going to depend on that stuff, so I don't think it's unreasonable to require the packager to straighten this stuff out for packages that don't ship with a proper configure script.

I think we should be mindful of making the common case easy and the special cases possible. This accomplishes that very well, and we go a step further by automating a number of important build systems. I think helping people beyond that is outside the scope of what we should have to maintain in Spack.

I didn't mean creating a second AutotoolsPackage, just a second AutotoolsPackageTemplate. Both would subclass AutotoolsPackage, but one would have additional dependencies and an additional method to override in the body. BuildSystemGuesser would choose between the two based on whether or not a configure script is present.

But if we don't want to do that, I think I'll just remove these dependencies. I agree that it is confusing. Also, the only way we can detect that it uses the Autotools build system is if there is a configure script, and if there is a configure script, we definitely won't need these.

tgamblin · 2017-01-15T23:48:12Z

lib/spack/docs/packaging_guide.rst

+       # depends_on('autoconf', type='build')
+       # depends_on('automake', type='build')
+       # depends_on('libtool', type='build')
+       # depends_on('foo')


maybe separate this from the autotools-related build deps and add a comment to the template saying to add library/other dependencies here?

tgamblin · 2017-01-15T23:48:48Z

lib/spack/docs/packaging_guide.rst

-           make("install")
+       def configure_args(self):
+           # FIXME: Add arguments other than --prefix
+           # FIXME: If not needed delete the function


"this function"

tgamblin · 2017-01-15T23:56:36Z

lib/spack/docs/packaging_guide.rst

+   and installed before this one. See :ref:`dependencies`.

-#. Get the ``install()`` method working.
+#. Get the installation working.


This is gong to conflict with @alalazo's #2780. But I guess I have the same question. Is the new title clearer? Can we bring out the install method somehow? Maybe this can wait for some other PR.

I can remove this particular line from my changes and we can discuss it in #2780.

Actually, I don't think @alalazo edits this particular line. I think we'll end up needing to review the Packaging Guide as a whole after this and #2780 are merged, so I'll leave it in for now.

I think it will be good at some point to review the documentation as a whole, maybe before the next major release. The information is there, but rearranging it to make the important bits more prominent or rewording parts to make the style more homogeneous across sections may be worth it.

tgamblin · 2017-01-16T00:03:35Z

lib/spack/spack/__init__.py


 # User's editor from the environment
-editor = Executable(os.environ.get("EDITOR", "vi"))
+editor = Executable(os.environ.get("EDITOR", "vim"))


I don't know. But maybe some embedded or stripped down ones (e.g. for containers)? What about:

if "EDITOR" in os.environ: editor = Executable(os.environ.get("EDITOR")) else: editor = which('vim') if not editor: editor = Executable('vi')

I'm an emacs user -- I just picked vi as the default because it's more likely to be installed, so I guess we should do best effort here.

tgamblin · 2017-01-16T00:13:08Z

lib/spack/spack/cmd/checksum.py

+    # Make sure the user provided a package and not a URL
+    if not valid_fully_qualified_module_name(args.package):
+        tty.die("`spack checksum` accepts package names, not URLs. "
+                "Use `spack md5 <url>` instead.")


Does it make sense to rename spack checksum? We will not always use md5, and it would be nice if spack checksum gave you whatever the current recommended checksum of a file/url was. Should spack checksum be called spack find-new-versions or spack-spider-versions or something?

tgamblin · 2017-01-16T00:15:24Z

lib/spack/spack/cmd/create.py

        # Determine the build system based on the files contained
        # in the archive.
-        build_system = 'unknown'
+        build_system = 'default'


How about 'generic' for the regular old Package?

tgamblin · 2017-01-16T00:17:27Z

lib/spack/spack/cmd/edit.py

    subparser.add_argument(
        '-f', '--force', dest='force', action='store_true',
-        help="Open a new file in $EDITOR even if package doesn't exist.")
+        help='deprecated, use `spack create` instead')


I think taking it out is fine if the docs reflect it.

adamjstewart · 2017-01-16T00:54:04Z

Now that the build system guessers are getting complex, does it make sense to make a subpackage for the different kinds, like build_systems, cmd, etc.?

I think it would make sense to move a lot of this logic to build_systems. We would just need to add build systems for scons, octave, and bazel. Now that we have ones for autotools, cmake, python, and r, it shouldn't be too hard to do. I can get started on this if there is interest.

We'll need to figure out whether to merge this or documentation: build-system phases + build-time tests #2780 first.

I actually think our PRs don't overlap that much. My changes to the Packaging Guide mostly affect the beginning, where we introduce spack create and spack edit. @alalazo's changes mostly affect "Implementing the install method", farther down. So either would be fine.

adamjstewart · 2017-01-16T16:54:20Z

@tgamblin I believe this is ready for a second review now.

tgamblin · 2017-01-17T01:13:08Z

@adamjstewart: some stuff is still missing but we can add it later.

adamjstewart · 2017-01-17T13:54:46Z

Thanks for getting this into 0.10! Let me know what is still missing and I'll add it to a new PR.

adamjstewart added UI WIP labels Dec 31, 2016

adamjstewart requested review from DavidPoliakoff, alalazo, citibeth, tgamblin and trws December 31, 2016 23:18

adamjstewart commented Dec 31, 2016

View reviewed changes

citibeth reviewed Jan 1, 2017

View reviewed changes

adamjstewart removed the WIP label Jan 2, 2017

adamjstewart changed the title ~~[WIP] Major improvements to spack create~~ Major improvements to spack create Jan 2, 2017

adamjstewart commented Jan 2, 2017

View reviewed changes

adamjstewart added 9 commits January 5, 2017 10:18

Initial changes to spack create command

9112b7c

Get 'spack create <url>' working again

212afd0

Simplify call to BuildSystemGuesser

20a9b96

More verbose output of spack create

56b5e05

Remove duplicated code from spack create and spack checksum

aee4bfb

Add better documentation to spack create docstrings

512c480

Fix pluralization bug

2024463

Flake8

6739622

Update documentation on spack create and deprecate spack edit --force

aa884c8

adamjstewart added 2 commits January 5, 2017 10:21

Make it more obvious when we are renaming a package

f522446

Further deprecate spack edit --force

6d8a824

adamjstewart force-pushed the features/create branch from 5dee821 to 6d8a824 Compare January 5, 2017 16:22

Fix unit tests

e5397da

adamjstewart mentioned this pull request Jan 6, 2017

Preliminary RPackage class #2761

Merged

adamjstewart mentioned this pull request Jan 12, 2017

Spack cannot parse URLs with '?' characters #2817

Closed

tgamblin requested changes Jan 16, 2017

View reviewed changes

adamjstewart added 5 commits January 16, 2017 09:46

Rename default template to generic template

bebc48e

Don't add automake/autoconf deps to Autotools packages

10b8881

Remove changes to default $EDITOR

f0152bd

Completely remove all traces of spack edit --force

9a4b5b0

Remove grammar changes to make the PR easier to review

1e2ee79

tgamblin approved these changes Jan 17, 2017

View reviewed changes

tgamblin merged commit 1f49493 into spack:develop Jan 17, 2017

adamjstewart deleted the features/create branch January 17, 2017 15:28

This was referenced Jan 17, 2017

Change spack create UI to default to name instead of URL #2843

Closed

spack edit -f => spack create #1108

Closed

[WIP] Allow spack create to handle already downloaded tarballs #2898

Closed

adamjstewart mentioned this pull request Jan 23, 2018

Fix error message typo in 'spack create' cmd #7042

Merged

Conversation

adamjstewart commented Dec 31, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

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!

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

adamjstewart commented Jan 2, 2017

Uh oh!

adamjstewart commented Jan 3, 2017

Uh oh!

adamjstewart commented Jan 5, 2017

Uh oh!

adamjstewart commented Jan 9, 2017

Uh oh!

adamjstewart commented Jan 15, 2017

Uh oh!

tgamblin left a comment • 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!

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

adamjstewart commented Dec 31, 2016 •

edited

Loading

tgamblin left a comment •

edited

Loading

alalazo Jan 16, 2017 •

edited

Loading