1132107 - Update Windows Environment Path to include directories under C:\mozilla-build

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: Puppet, task)

Comment 1

[Mark Cornmesser [:markco]]

Attachment: BUG1132107.patch

This patch includes a module from Puppet Forge. 

https://forge.puppetlabs.com/basti1302/windows_path

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8564245 [details] [diff] [review]
BUG1132107.patch

Review of attachment 8564245 [details] [diff] [review]:
-----------------------------------------------------------------

::: modules/buildslave/manifests/install/version.pp
@@ +58,5 @@
> +                        # Append the virtual environment directory to the Windows path
> +                        windows_path {
> +                            "$virtualenv_path":
> +                                ensure => present;
> +                        }

Like!

::: modules/tweaks/manifests/append_windows_path.pp
@@ +16,5 @@
> +            ensure => present; 
> +    }
> +    windows_path {
> +        'C:\mozilla-build\python':
> +            ensure => present;

Hm, back and forward slashes are mixed in this file -- which one is right?

@@ +36,5 @@
> +            ensure => present;
> +    }
> +    windows_path {
> +        'C:\mozilla-build\info-zip':
> +            ensure => present;

I feel like each of these clauses belongs somewhere else.  Maybe in the package class?  It doesn't seem right to bunch them all together here, when they refer to all sorts of things which may or may not be installed.

::: modules/windows_path/Modulefile
@@ +1,4 @@
> +name 'basti1302-windows_path'
> +version '0.1.1'
> +summary 'Manage individual entries in the Windows PATH variable as resources, that is, add and remove directories to the PATH in an idempotent manner.'
> +project_page 'https://github.com/basti1302/puppet-windows-path'

Cool module!  Good find!

Comment 3

[Mark Cornmesser [:markco]]

> > +            ensure => present;
> > +    }
> > +    windows_path {
> > +        'C:\mozilla-build\info-zip':
> > +            ensure => present;
> 
> I feel like each of these clauses belongs somewhere else.  Maybe in the
> package class?  It doesn't seem right to bunch them all together here, when
> they refer to all sorts of things which may or may not be installed.

All of those directories are a products of the Mozilla build package. Do you think it would make more sense to put them in the  packages::mozilla::mozilla_build manifest?

Comment 4

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8564245 [details] [diff] [review]
BUG1132107.patch

Review of attachment 8564245 [details] [diff] [review]:
-----------------------------------------------------------------

Ah, good point.  Yes, packages::mozilla_build makes sense.

::: modules/tweaks/manifests/append_windows_path.pp
@@ +27,5 @@
> +        'C:\mozilla-build\msys\bin':
> +            ensure => present;
> +    }
> +    windows_path {
> +        'C:\mozilla-build\C:\mozilla-build\vim\vim72':

Oh, also, broken path here!

Comment 5

[Mark Cornmesser [:markco]]

Attachment: BUG1132107.patch

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8565534 [details] [diff] [review]
BUG1132107.patch

Review of attachment 8565534 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great and is good to land.

One worry I have is that when we upgrade buildbot, the old version will still be in PATH.  Does windows_path support ensure => absent, too?  If so, can you make a new patch to add that to the "absent:" clause at the bottom of modules/buildslave/manifests/install/version.pp?

Comment 7

[Mark Cornmesser [:markco]]

Comment on attachment 8565534 [details] [diff] [review]
BUG1132107.patch

Review of attachment 8565534 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/build/puppet/rev/f3a831c1d8b5