Standardize package names: lower-case, not Mixed_CASE #2475

citibeth · 2016-12-04T15:24:27Z

@adamjstewart
This PR converts all package names to lower case. Benefits:

Conforms to previously-discussed Spack standard.
Make it easier for users to "guess" the right name of a package. (Is it "Python" or "python"? "R" or "r"?)
Works better on macOS

adamjstewart · 2016-12-04T18:10:41Z

Can you elaborate on point 3: "Works better on macOS"?

citibeth · 2016-12-04T18:49:03Z

Can you elaborate on point 3: "Works better on macOS"?

Running things like git mv R r can produce strange results on macOS. Also, macOS will have problems if we were to create multiple packages that differ only in their case.

hartzell · 2016-12-04T20:20:37Z

The mac file system, by default, is "case-insensitive but case-preserving", it remembers how you created a file but doesn't use the info for anything. The Google has more info, e.g. here.

I learned this back in the day when I was working with a project in svn that had both a makefile and Makefile, checking them out on an OS X machine led to much hilarity.

adamjstewart · 2016-12-04T23:22:06Z

By the way, I made a few changes to how spack create chooses a default name in #2351 that should help make things more uniform. @citibeth If you haven't already, can you convert packages like the_silver_searcher to the-silver-searcher? It goes along the same lines of not knowing whether it is uppercase or lowercase.

alalazo · 2016-12-05T09:43:46Z

@glennpj @JavierCVilla Pinging the two guys that contributed most to r packaging.

adamjstewart

I think the change from R to r is the only thing particularly controversial about this PR, but I'm in favor of it.

citibeth · 2016-12-05T16:20:14Z

I concur. But remember, we use lapack not LAPACK. x not X. And so on for so many other acronym packages. The only difference is r is only one letter.

…

On Dec 5, 2016 11:13 AM, "Adam J. Stewart" ***@***.***> wrote: ***@***.**** approved this pull request. I think the change from R to r is the only thing particularly controversial about this PR, but I'm in favor of it. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2475 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cdzI4m2cTbIoXs6Jn5o8O2pY71HOIks5rFDglgaJpZM4LDnC7> .

tgamblin · 2016-12-05T16:26:36Z

How strongly do people feel about this? I would be ok with merging this right before the 0.10 release, as this just brings all the packages inline with the convention we're using for spack create. I wonder if this is a big disruption for R users, though.

@davydden @alalazo @glennpj

davydden · 2016-12-05T16:30:08Z

I wonder if this is a big disruption for R users, though.

i never used R, but i like the fact that everything is named consistently and there is no difference between py-foo and r-foo.

tgamblin · 2016-12-05T18:16:12Z

I like the consistency. Mostly looking for @glennpj's feedback as I believe he has R users.

glennpj · 2016-12-05T20:06:21Z

@tgamblin That change should be fine.

alalazo · 2016-12-05T20:08:45Z

@tgamblin also fine with me

tgamblin · 2016-12-05T21:40:14Z

@citibeth Ok people are ok with this. Let's get this merged quickly, as it is a lot of changes. Can you rebase on current develop and make sure there are no more mixed-case packages?

citibeth · 2016-12-05T23:32:55Z

OK, done.

tgamblin · 2016-12-06T00:06:16Z

I added a test to enforce the convention, so Travis will fail if new, nonconforming packages appear.

While I was doing that, I encountered an issue. The updates in this branch do NOT naturally take effect for Mac OS X users, because git doesn't correct the directory names. This causes tests like package_sanity to fail. It's similar to the issue mentioned above where mv works in funny ways on Mac OS X. This may be because I use GNU mv from coreutils and not the builtin Mac OS X mv but I don't know what git uses to change names on checkout.

How do we make this not immediately break for Mac OS X users? It would be nice if we could easily check the branch out and switch to a new one without getting case errors, but I dunno the best way to do it without calling git ls-tree and correcting the builtin package repo.

@davydden: can you check this branch out without getting test errors on your mac due to uncorrected directory names in var/spack/repos/builtin/packages?

citibeth · 2016-12-06T00:15:08Z

Yes you are right, and I don't know the solution. I discovered that issue early on when trying to APPLY these fixes on the Mac. To me that's just one more reason to try to get this merged.

…

On Mon, Dec 5, 2016 at 7:06 PM, Todd Gamblin ***@***.***> wrote: I added a test to enforce the convention, so Travis will fail if new, nonconforming packages appear. While I was doing that, I encountered an issue. The updates in this branch do NOT naturally take effect for Mac OS X users, because git doesn't correct the directory names. This causes tests like package_sanity to fail. It's similar to the issue mentioned above where mv works in funny ways on Mac OS X. This *may* be because I use GNU mv from coreutils and not the builtin Mac OS X mv but I don't know what git uses to change names on checkout. How do we make this not immediately break for Mac OS X users? It would be nice if we could easily check the branch out and switch to a new one without getting case errors, but I dunno the best way to do it without calling git ls-tree and correcting the builtin package repo. @davydden <https://github.com/davydden>: can you check this branch out without getting test errors on your mac due to uncorrected directory names in var/spack/repos/builtin/packages? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd9HI8i0YCg313b5XyOswTD7XPiWfks5rFKb5gaJpZM4LDnC7> .

JavierCVilla · 2016-12-06T14:46:23Z

@citibeth I'm not sure about this but just in case, shouldn't affect the change to lower-case also to these parts of the current R/package.py file?

    @property
    def r_lib_dir(self):
        return join_path('rlib', 'R', 'library')

        run_env.prepend_path('LIBRARY_PATH',
                             join_path(self.prefix, 'rlib', 'R', 'lib'))
        run_env.prepend_path('LD_LIBRARY_PATH',
                             join_path(self.prefix, 'rlib', 'R', 'lib'))
        run_env.prepend_path('CPATH',
                             join_path(self.prefix, 'rlib', 'R', 'include'))

        # R extension builds can have a global R executable function
        module.R = Executable(join_path(self.spec.prefix.bin, 'R'))

I mean in the join_path where R is still appended in upper-case.

citibeth · 2016-12-06T14:58:12Z

No, I don't think it changes that. The R build creates a directory in upper-case called 'R'. That's not something we can (easily) change, nor do I believe we should try.

…

On Tue, Dec 6, 2016 at 9:46 AM, Javier ***@***.***> wrote: @citibeth <https://github.com/citibeth> I'm not sure about this but just in case, shouldn't affect the change to lower-case also to these parts of the current R/package.py file? @Property def r_lib_dir(self): return join_path('rlib', 'R', 'library') run_env.prepend_path('LIBRARY_PATH', join_path(self.prefix, 'rlib', 'R', 'lib')) run_env.prepend_path('LD_LIBRARY_PATH', join_path(self.prefix, 'rlib', 'R', 'lib')) run_env.prepend_path('CPATH', join_path(self.prefix, 'rlib', 'R', 'include')) # R extension builds can have a global R executable function module.R = Executable(join_path(self.spec.prefix.bin, 'R')) I mean in the join_path where R is still appended in upper-case. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cdw0a_U9gOr5VI_RPofBH9bnrB-PRks5rFXVBgaJpZM4LDnC7> .

adamjstewart · 2016-12-06T15:42:01Z

@citibeth You'll also need to update this line so that spack create uses lowercase R.

adamjstewart · 2016-12-07T20:06:19Z

There's also a lot of stuff in lib/spack/spack/util/naming.py (and possibly elsewhere) that allows packages to contain _ and A-Z values. If we really want to be thorough, changing these would completely disallow future packages from containing these characters.

citibeth · 2016-12-07T20:56:15Z

That is a decent idea, but out of the purview of what I originally set out to do. We can also prevent these things from creeping in by asking for changes on PRs that have them.

…

On Wed, Dec 7, 2016 at 3:06 PM, Adam J. Stewart ***@***.***> wrote: There's also a lot of stuff in lib/spack/spack/util/naming.py (and possibly elsewhere) that allows packages to contain _ and A-Z values. If we really want to be thorough, changing these would completely disallow future packages from containing these characters. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd8OcY4N_vXPQgEqaas42JL5B6MCWks5rFxG8gaJpZM4LDnC7> .

adamjstewart · 2016-12-07T20:59:04Z

That just adds work for us to keep an eye out for that. At some point, a non-conforming package will slip in, and we will end up in the same mess of not being able to lowercase packages on macOS. Better to do it right the first time if you ask me.

citibeth · 2016-12-07T21:02:48Z

Can you do it?

…

On Wed, Dec 7, 2016 at 3:59 PM, Adam J. Stewart ***@***.***> wrote: That just adds work for us to keep an eye out for that. At some point, a non-conforming package will slip in, and we will end up in the same mess of not being able to lowercase packages on macOS. Better to do it right the first time if you ask me. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2475 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1cd99R-8njVNYDkKS7VMG-dhoBTu3_ks5rFx4ZgaJpZM4LDnC7> .

adamjstewart · 2016-12-07T21:04:59Z

Not at the moment (probably not any time soon either).

citibeth · 2016-12-30T23:04:48Z

@alalazo @tgamblin Are you able to help with the test failures I'm getting here? It's only failing on Python 2.6.

______________ PackageSanityTest.test_all_versions_are_lowercase _______________
self = <spack.test.package_sanity.PackageSanityTest testMethod=test_all_versions_are_lowercase>
    def test_all_versions_are_lowercase(self):
        """Spack package names must be lowercase, and use `-` instead of `_`.
            """
        errors = []
        for name in spack.repo.all_package_names():
            if re.search(r'[_A-Z]', name):
                errors.append(name)
    
>       self.assertEqual([], errors)
E       AssertionError: [] != ['direct_mpich', 'indirect_mpich', 'simple_inheritance', 'trivial_install_test_package']
../../../lib/spack/spack/test/package_sanity.py:70: AssertionError
=============== 1 failed, 481 passed, 1 xfailed in 75.12 seconds ===============

alalazo · 2016-12-31T05:04:45Z

@citibeth I don't know if I'll have time later in the day to check the causes, but it seems it's failing because it checks builtin.mock instead of builtin.

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

@adamjstewart I found the problem... unit tests were still using underscores in package names instead of dashes. But this yields a few questions:

The tests that tripped me up were checking for underscores and capital letters in the test repo. That's all fine, but... are they ALSO doing this check in the main repo?
It would still be nice to know why this is failing on Python 2.6 and Mac, but succeeding on other systems. My best guess is that in some cases, the unit test is checking the main repo; and in others, it is checking the test repo. That would seem to be wrong, are you able to look into that?

https://travis-ci.org/LLNL/spack/builds/187805917

adamjstewart

Just took another look through this and everything looks solid. There may be other references to capitalized packages in the documentation, but we can fix those whenever. Btw, when I updated the documentation on spack create in #2707, I updated a couple sections that made it sound like capitalized packages or packages with underscores were fine.

I think at some point, we should update Spack's internals that make sure a package name is valid. These still allow packages with uppercase letters and underscores.

Ping @tgamblin, can we get this merged? It affects a lot of packages and will conflict with #2707 and #2709. I also want to get started on an RPackage class, which would heavily conflict with this PR since all of the R packages extend R. Although it would be deleting the extends directive in these packages.

adamjstewart · 2017-01-03T16:24:33Z

lib/spack/docs/module_file_support.rst

@@ -367,14 +367,14 @@ and has similar effects on module file of dependees. Even in this case

  with the following snippet:

-  .. literalinclude:: ../../../var/spack/repos/builtin/packages/R/package.py
+  .. literalinclude:: ../../../var/spack/repos/builtin/packages/r/package.py
     :pyobject: R.setup_environment

  The ``R`` package also knows which environment variable should be modified


@citibeth This should be r. You might want to look through the documentation to make sure there aren't other capitalized packages being reference. The documentation tests aren't foolproof.

citibeth · 2017-01-03T16:50:16Z

OK, I just fixed up that section of the docs. I agree with @adamjstewart, let's get this merged and deal with docs later. I expect there will be significant editing/rewriting as we integrate build-system-specific `Package` superclasses into the docs, I think that's the time to really edit this stuff carefully.

adamjstewart · 2017-01-03T19:05:16Z

lib/spack/docs/module_file_support.rst

-  .. literalinclude:: ../../../var/spack/repos/builtin/packages/R/package.py
-     :pyobject: R.setup_environment
+  .. literalinclude:: ../../../var/spack/repos/builtin/packages/r/package.py
+     :pyobject: r.setup_environment


This isn't going to work. The class is still named R, just like python's class is named Python. You should remove this particular change.

Fix r->R for class R.

citibeth · 2017-01-05T01:46:15Z

@tgamblin Can we please get this merged? It's been approved and re-approved. I just rebased again...

citibeth added the refactoring label Dec 4, 2016

citibeth changed the title ~~Convert all package names to lower case~~ Standardize package names: lower-case, not Mixed_CASE Dec 5, 2016

davydden approved these changes Dec 5, 2016

View reviewed changes

adamjstewart approved these changes Dec 5, 2016

View reviewed changes

adamjstewart mentioned this pull request Dec 5, 2016

Allow spack create to handle packages with period in name #2351

Merged

citibeth force-pushed the 161204-efischer/DowncasePackages branch from 17bc1f6 to e76e9a3 Compare December 5, 2016 23:29

JavierCVilla mentioned this pull request Dec 6, 2016

New package - r-xgboost #2493

Merged

citibeth force-pushed the 161204-efischer/DowncasePackages branch from 4d22010 to f2bfe6f Compare December 30, 2016 22:55

citibeth mentioned this pull request Jan 1, 2017

Major improvements to spack create #2707

Merged

5 tasks

adamjstewart mentioned this pull request Jan 1, 2017

[WIP] Preliminary PythonPackage class #2709

Closed

adamjstewart approved these changes Jan 3, 2017

View reviewed changes

adamjstewart requested changes Jan 3, 2017

View reviewed changes

adamjstewart approved these changes Jan 4, 2017

View reviewed changes

Elizabeth Fischer and others added 15 commits January 4, 2017 20:45

Rename packages

49d0b35

Upcasing depends_on() in packages.

e4cbcd5

Downcased extends('r')

9fd9ff4

Fixed erroneously changed URL that had slipped through.

ea82a4b

Fixed typo

9c6677c

Fixed link from documentation into package source code.

67d3572

Fixed another doc problem.

222d734

Changed underscores to dashes in package names.

1dd77b2

Added test to enforce lowercase, no-underscore naming convention.

0ce2700

Fix r-xgboost

2115a9c

Downcase more instances of 'R' in package auto-creation.

0b93f1a

Fix test.

2bddcb1

Converted unit test packages to use dashes not underscores

81563bf

Downcase r in the docs.

8cf7e3f

Update module_file_support.rst

16efdda

Fix r->R for class R.

citibeth force-pushed the 161204-efischer/DowncasePackages branch from 976b364 to 16efdda Compare January 5, 2017 01:45

tgamblin merged commit 3dd4a01 into develop Jan 5, 2017

adamjstewart mentioned this pull request Jan 5, 2017

various python packages #2320

Merged

citibeth deleted the 161204-efischer/DowncasePackages branch January 5, 2017 17:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Standardize package names: lower-case, not Mixed_CASE #2475

Standardize package names: lower-case, not Mixed_CASE #2475

citibeth commented Dec 4, 2016

adamjstewart commented Dec 4, 2016

citibeth commented Dec 4, 2016

hartzell commented Dec 4, 2016

adamjstewart commented Dec 4, 2016

alalazo commented Dec 5, 2016

adamjstewart left a comment

citibeth commented Dec 5, 2016 via email

tgamblin commented Dec 5, 2016

davydden commented Dec 5, 2016

tgamblin commented Dec 5, 2016

glennpj commented Dec 5, 2016

alalazo commented Dec 5, 2016

tgamblin commented Dec 5, 2016

citibeth commented Dec 5, 2016

tgamblin commented Dec 6, 2016

citibeth commented Dec 6, 2016 via email

JavierCVilla commented Dec 6, 2016

citibeth commented Dec 6, 2016 via email

adamjstewart commented Dec 6, 2016

adamjstewart commented Dec 7, 2016

citibeth commented Dec 7, 2016 via email

adamjstewart commented Dec 7, 2016

citibeth commented Dec 7, 2016 via email

adamjstewart commented Dec 7, 2016

citibeth commented Dec 30, 2016 •

edited

Loading

alalazo commented Dec 31, 2016

citibeth commented Jan 1, 2017

adamjstewart left a comment •

edited

Loading

adamjstewart Jan 3, 2017

citibeth commented Jan 3, 2017 via email

adamjstewart Jan 3, 2017

citibeth commented Jan 5, 2017

Standardize package names: lower-case, not Mixed_CASE #2475

Standardize package names: lower-case, not Mixed_CASE #2475

Conversation

citibeth commented Dec 4, 2016

adamjstewart commented Dec 4, 2016

citibeth commented Dec 4, 2016

hartzell commented Dec 4, 2016

adamjstewart commented Dec 4, 2016

alalazo commented Dec 5, 2016

adamjstewart left a comment

Choose a reason for hiding this comment

citibeth commented Dec 5, 2016 via email

tgamblin commented Dec 5, 2016

davydden commented Dec 5, 2016

tgamblin commented Dec 5, 2016

glennpj commented Dec 5, 2016

alalazo commented Dec 5, 2016

tgamblin commented Dec 5, 2016

citibeth commented Dec 5, 2016

tgamblin commented Dec 6, 2016

citibeth commented Dec 6, 2016 via email

JavierCVilla commented Dec 6, 2016

citibeth commented Dec 6, 2016 via email

adamjstewart commented Dec 6, 2016

adamjstewart commented Dec 7, 2016

citibeth commented Dec 7, 2016 via email

adamjstewart commented Dec 7, 2016

citibeth commented Dec 7, 2016 via email

adamjstewart commented Dec 7, 2016

citibeth commented Dec 30, 2016 • edited Loading

alalazo commented Dec 31, 2016

citibeth commented Jan 1, 2017

adamjstewart left a comment • edited Loading

Choose a reason for hiding this comment

adamjstewart Jan 3, 2017

Choose a reason for hiding this comment

citibeth commented Jan 3, 2017 via email

adamjstewart Jan 3, 2017

Choose a reason for hiding this comment

citibeth commented Jan 5, 2017

citibeth commented Dec 30, 2016 •

edited

Loading

adamjstewart left a comment •

edited

Loading