Validate scope name components.

Review Request #3893 — Created May 16, 2016 and submitted

benjyw
pants
pants-reviews
jsirois, stuhood

Previously we had a scope (gopkg.in) containing a dot. However dots are
used to nest scopes, so this was potentially ambiguous and a bug waiting
to happen (e.g., this only worked because we created an implicit 'gopkg'
parent scope behind the scenes, which isn't what was intended).

Now we forbid dots, and for uniformity we also require lower-case letters
and dashes instead of underscores.

Invalid scope names are still allowed, but deprecated. In 1.2 they will
become errors.

This change deprecates gopkg.in and, while we're at it, a couple of other
go-related scopes.

Unfortunately we can't do this check in one central place, because by the
time that would be possible we already have composite scope names (foo.bar)
which would fail the check. We have to check the individual components before
they get munged together, so it's a little less elegant than I would like.
Not terrible though.

Note that we currently get warnings about check_published_deps, because of
how its deprecation was implemented. But that hack can go away in the next
release, so this is a very temporary warning.

CI passes: http://jenkins.pantsbuild.org/job/pantsbuild/job/pants/branch/PR-3448/

  • 0
  • 0
  • 2
  • 0
  • 2
Description From Last Updated
NH
  1. Thanks. Yay explicit constraints!

  2. src/python/pants/option/options.py (Diff revision 1)
     
     

    This looks like it's fixing construction of intermediate scopes for deprecated scopes, which by inspection was not happening correctly earlier. If so, could you add a regression test for that?

    1. Ah, it's obviously not clear from the code (because I neglected to add a comment), but this was only necessary because gopkg.in has a dot in it, and that only worked because we created an intermediate 'gopkg' scope. Once the validation is enforced everywhere, we can revert this. I added the following comment above this code, to clarify:

          # TODO: Once scope name validation is enforced (so there can be no dots in scope name
          # components) we can replace this line with `for si in scope_infos:`, because it will
          # not be possible for a deprecated_scope to introduce any new intermediate scopes.
      
  3. I think this should live in test_optionable.py since it's testing a method on Optionable.

    1. Good idea. I forget we had a test_optionable.py.

  4. 
      
ST
  1. 
      
  2. Are these actually go specific? My impression was that John thought they might be applied to recursive fetches of other things as well.

    1. I talked to John and he concurred re changing the scope names.

      These options, and the code, are really go-specific in practice. If we were to generalize this it would be radically different code.

  3. src/python/pants/option/optionable.py (Diff revision 1)
     
     

    Should be able to put [a-z0-9] in the same character class:

    r'^[a-z0-9]+(?:-[a-z0-9]+)*$'
    
    1. Ah yes, good call.

  4. 
      
BE
NH
  1. Ship It!
  2. 
      
BE
Review request changed

Status: Closed (submitted)

Change Summary:

e0d88dc6b7d25a7a4ee188419bff7e0ff367561b

BE
  1. Submitted. Thanks Stu and Nick!

  2. 
      
Loading...