[vpython] Add conditional packages based on PEP425

[vpython] Add conditional packages based on PEP425

Allow packages to specify inclusion pending PEP425 tag matching.
Packages that specify match filters will only be included in the
VirtualEnv if one of their match tags matches a system PEP425 tag.

BUG=None
TEST=local
R=iannucci@chromium.org

Review-Url: https://codereview.chromium.org/2921133002
Committed: https://github.com/luci/luci-go/commit/288831e8f27616792594ba30104b6266ba9af015

[dnj, 2017-06-03 02:49:30]

dnj@chromium.org changed reviewers:
+ qyearsley@chromium.org

[iannucci, 2017-06-05 19:02:47]

lgtm

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go
File vpython/spec/match_test.go (right):

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go#...
vpython/spec/match_test.go:50: mkTag("fooV", "fooABI", "fooArch"),
it would be nice to use more realistic looking things here

[qyearsley, 2017-06-05 20:20:08]

So, if I understand correctly, after this CL, I should be to list wheels in
vpython env config files like this:

wheel: <
  name: "infra/python/wheels/pywin32-win_amd64"
  version: "version:219"
  match_tag: py27-none-win_amd64
>

wheel: <
  name: "infra/python/wheels/pywin32-win32"
  version: "version:219"
  match_tag: py27-none-win32
>

(Assuming that there is no applicable ABI string and the "architecture" or
"platform" strings are "win32" and "win_amd64".)

https://codereview.chromium.org/2921133002/diff/1/vpython/application/subcomm...
File vpython/application/subcommand_verify.go (right):

https://codereview.chromium.org/2921133002/diff/1/vpython/application/subcomm...
vpython/application/subcommand_verify.go:55: // normalizse a clone.
Nit: spelling

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go
File vpython/spec/match.go (right):

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go#newco...
vpython/spec/match.go:12: // package match the supplied PEP425 tags.
... match any of the supplied PEP425 tags.

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go#newco...
vpython/spec/match.go:35: func PEP425Matches(match *vpython.Pep425Tag, tags
[]*vpython.Pep425Tag) bool {
For consistency, this could be called Pep425Matches (since the type is called
Pep425Tag).

Also, I'm new to Go -- should the types of the arguments in this type
declaration be interpreted as "pointer to vpython.Pep425Tag" and "pointer to a
slice of vpython.Pep425Tag"?

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go#newco...
vpython/spec/match.go:49: continue
Another Go question:

Is this equivalent to:


		if match.Version != "" && tag.Version != match.Version {
			continue
		}
		if match.Abi != "" && tag.Abi != match.Abi {
			continue
		}
		if match.Arch != "" && tag.Arch != match.Arch {
			continue
		}

I'm not sure what's conventional, but it might also make sense to use variable
names like `ver`, `abi`, `arch` instead of `v` for all of them.

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go
File vpython/spec/match_test.go (right):

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go#...
vpython/spec/match_test.go:21: Arch:    arch,
Note, I guess Pep425Tag is defined elsewhere(?) but it might be worth noting
that in the PEP425 docs these are called "Python", "ABI" and "Platform" rather
than "Version", "Abi", and "Arch" (I think)

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go#...
vpython/spec/match_test.go:84: Convey(fmt.Sprintf(`With system tags: %s`,
tagString(tc.tags)), func() {
General question:
If the test passes then normally when the test is run, is anything printed?

[dnj, 2017-06-06 13:59:10]

> So, if I understand correctly, after this CL, I should be to list wheels in
> vpython env config files like this:
> 
> wheel: <
>   name: "infra/python/wheels/pywin32-win_amd64"
>   version: "version:219"
>   match_tag: py27-none-win_amd64
> >
> 
> wheel: <
>   name: "infra/python/wheels/pywin32-win32"
>   version: "version:219"
>   match_tag: py27-none-win32
> >
> 
> (Assuming that there is no applicable ABI string and the "architecture" or
> "platform" strings are "win32" and "win_amd64".)

Almost. All you'd need is:
match_tag: <
  arch: "win_amd64"
>
match_tag: <
  arch: "win_386"
>

https://codereview.chromium.org/2921133002/diff/1/vpython/application/subcomm...
File vpython/application/subcommand_verify.go (right):

https://codereview.chromium.org/2921133002/diff/1/vpython/application/subcomm...
vpython/application/subcommand_verify.go:55: // normalizse a clone.
On 2017/06/05 20:20:08, qyearsley wrote:
> Nit: spelling

Done.

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go
File vpython/spec/match.go (right):

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go#newco...
vpython/spec/match.go:12: // package match the supplied PEP425 tags.
On 2017/06/05 20:20:08, qyearsley wrote:
> ... match any of the supplied PEP425 tags.

Done.

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go#newco...
vpython/spec/match.go:35: func PEP425Matches(match *vpython.Pep425Tag, tags
[]*vpython.Pep425Tag) bool {
On 2017/06/05 20:20:08, qyearsley wrote:
> For consistency, this could be called Pep425Matches (since the type is called
> Pep425Tag).
> 
> Also, I'm new to Go -- should the types of the arguments in this type
> declaration be interpreted as "pointer to vpython.Pep425Tag" and "pointer to a
> slice of vpython.Pep425Tag"?

The type is called "Pep425Tag" b/c it is derived from a protobuf message, and
protobuf naming practices dictate not uppercasing acronyms. However, the
preferred way to express it in Go naming is to keep acronyms capitalized, so I'm
doing that where possible in the Go code.

[]*vpython.Pep425Tag is "Slice of pointers to Pep425Tag".

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go#newco...
vpython/spec/match.go:49: continue
On 2017/06/05 20:20:08, qyearsley wrote:
> Another Go question:
> 
> Is this equivalent to:
> 
> 
> 		if match.Version != "" && tag.Version != match.Version {
> 			continue
> 		}
> 		if match.Abi != "" && tag.Abi != match.Abi {
> 			continue
> 		}
> 		if match.Arch != "" && tag.Arch != match.Arch {
> 			continue
> 		}
> 
> I'm not sure what's conventional, but it might also make sense to use variable
> names like `ver`, `abi`, `arch` instead of `v` for all of them.

Yes. Idiomatic Go is pretty cool with abbreviated names. Since it's scoped to a
single line, it's concise enough that I actually prefer the consistency in
naming ("v" everywhere) over matching fields.

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match.go#newco...
vpython/spec/match.go:49: continue
On 2017/06/05 20:20:08, qyearsley wrote:
> Another Go question:
> 
> Is this equivalent to:
> 
> 
> 		if match.Version != "" && tag.Version != match.Version {
> 			continue
> 		}
> 		if match.Abi != "" && tag.Abi != match.Abi {
> 			continue
> 		}
> 		if match.Arch != "" && tag.Arch != match.Arch {
> 			continue
> 		}
> 
> I'm not sure what's conventional, but it might also make sense to use variable
> names like `ver`, `abi`, `arch` instead of `v` for all of them.

Yes. Idiomatic Go is pretty cool with abbreviated names. Since it's scoped to a
single line, it's concise enough that I actually prefer the consistency in
naming ("v" everywhere) over matching fields.

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go
File vpython/spec/match_test.go (right):

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go#...
vpython/spec/match_test.go:21: Arch:    arch,
On 2017/06/05 20:20:08, qyearsley wrote:
> Note, I guess Pep425Tag is defined elsewhere(?) but it might be worth noting
> that in the PEP425 docs these are called "Python", "ABI" and "Platform" rather
> than "Version", "Abi", and "Arch" (I think)

Yeah, unfortunately fixing these field names would break existing text protobuf
naming, so it's not trivial. Also beyond the scope of the CL. Probably worth
spending the time to do though.

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go#...
vpython/spec/match_test.go:84: Convey(fmt.Sprintf(`With system tags: %s`,
tagString(tc.tags)), func() {
On 2017/06/05 20:20:08, qyearsley wrote:
> General question:
> If the test passes then normally when the test is run, is anything printed?

If you run with "-v" for verbose, then the test case names are printed.

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go#...
vpython/spec/match_test.go:84: Convey(fmt.Sprintf(`With system tags: %s`,
tagString(tc.tags)), func() {
On 2017/06/05 20:20:08, qyearsley wrote:
> General question:
> If the test passes then normally when the test is run, is anything printed?

If you run with "-v" for verbose, then the test case names are printed.

[qyearsley, 2017-06-06 14:26:33]

Alright, LGTM :-)

[dnj, 2017-06-06 14:29:34]

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go
File vpython/spec/match_test.go (right):

https://codereview.chromium.org/2921133002/diff/1/vpython/spec/match_test.go#...
vpython/spec/match_test.go:21: Arch:    arch,
On 2017/06/06 13:59:10, dnj wrote:
> On 2017/06/05 20:20:08, qyearsley wrote:
> > Note, I guess Pep425Tag is defined elsewhere(?) but it might be worth noting
> > that in the PEP425 docs these are called "Python", "ABI" and "Platform"
rather
> > than "Version", "Abi", and "Arch" (I think)
> 
> Yeah, unfortunately fixing these field names would break existing text
protobuf
> naming, so it's not trivial. Also beyond the scope of the CL. Probably worth
> spending the time to do though.

FYI, filed this to fix naming: https://codereview.chromium.org/2927573002/

[dnj, 2017-06-06 19:32:42]

iannucci@, ping? :)

[dnj (Google), 2017-06-07 18:51:39]

rebase

[dnj, 2017-06-07 18:54:36]

The CQ bit was checked by dnj@chromium.org

[dnj, 2017-06-07 18:54:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
iannucci@chromium.org, qyearsley@chromium.org
Link to the patchset: https://codereview.chromium.org/2921133002/#ps40001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-07 18:54:42]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-07 19:01:13]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496861676043460,
"parent_rev": "3815930c1099d1c3060531cb66f2274dc5c977eb", "commit_rev":
"288831e8f27616792594ba30104b6266ba9af015"}

[commit-bot: I haz the power, 2017-06-07 19:01:16]

Description was changed from

==========
[vpython] Add conditional packages based on PEP425

Allow packages to specify inclusion pending PEP425 tag matching.
Packages that specify match filters will only be included in the
VirtualEnv if one of their match tags matches a system PEP425 tag.

BUG=None
TEST=local
R=iannucci@chromium.org
==========

to

==========
[vpython] Add conditional packages based on PEP425

Allow packages to specify inclusion pending PEP425 tag matching.
Packages that specify match filters will only be included in the
VirtualEnv if one of their match tags matches a system PEP425 tag.

BUG=None
TEST=local
R=iannucci@chromium.org

Review-Url: https://codereview.chromium.org/2921133002
Committed:
https://github.com/luci/luci-go/commit/288831e8f27616792594ba30104b6266ba9af015
==========

[commit-bot: I haz the power, 2017-06-07 19:01:17]

Committed patchset #3 (id:40001) as
https://github.com/luci/luci-go/commit/288831e8f27616792594ba30104b6266ba9af015