Closed Bug 1009004 Opened 5 years ago Closed 5 years ago

Re-implement INSTALL.sh script as a python script

Categories

(Testing Graveyard :: TPS, defect)

defect
Not set

Tracking

(firefox31 fixed, firefox32 fixed)

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed

People

(Reporter: cosmin-malutan, Assigned: cosmin-malutan)

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 11 obsolete files)

In order to ran TPS via CI on windows nodes we have to get rid of INSTALL.sh script and use python so it would be cross-platform.
It's not only that we can run it on Windows with simply Python installed, but also that it should be able to handle packaged tests. Please do not forget about this. Right now it fails to find the tests and extensions.
Summary: Replace INSTALL.sh script with a python script so it will work on windows nodes too → Re-implement INSTALL.sh script as a python script
Version: 31 Branch → Trunk
Attached patch patch_v1.0 (obsolete) — Splinter Review
This is the first patch here, I kept most of the logic from INSTALL.sh, and I had to change the cli.py  file so it will correctly detect the tests file if no argument is given.
Attachment #8421765 - Flags: review?(hskupin)
Comment on attachment 8421765 [details] [diff] [review]
patch_v1.0

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

::: testing/tps/configure.py
@@ +1,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +# This scripts sets up a virutalenv and installs TPS into it.

Please check for spelling mistakes. Also add the other half of the comment, and make it a global docstring.

@@ +25,5 @@
> +{NEWCONFIG}
> +
> +***********************************************************************
> +"""
> +def main():

Please add blank lines and run your patch through PEP8 to verify the style.

@@ +30,5 @@
> +    target = sys.argv[1]
> +
> +    # Create a virtual environment
> +    urllib.urlretrieve(virtualenv_url, 'virtualenv.py')
> +    run_process(['python', 'virtualenv.py', target])

I think it's fine to take the Python interpreter which is used to run this script. But I think we should also offer a command line option like the former script had to specify a specific version.

@@ +37,5 @@
> +        python_path = os.path.join(os.getcwd(), target, 'Scripts/python.exe')
> +        bin_name = 'Scripts/activate.bat'
> +    else:
> +        python_path = os.path.join(os.getcwd(), target, 'bin/python')
> +        bin_name = 'bin/activate'

Please see https://github.com/mozilla/mozmill-ci/pull/340/files#diff-e4092b51e0f0643e1d45640d1a2950a8R46 how to use Python itself to activate the environment.

@@ +42,5 @@
> +
> +    # Install TPS in environment
> +    run_process([python_path, 'setup.py', 'install'])
> +
> +    #clean up files created by setup.py

nit: missing blank after the #

@@ +45,5 @@
> +
> +    #clean up files created by setup.py
> +    shutil.rmtree('build')
> +    shutil.rmtree('dist')
> +    shutil.rmtree('tps.egg-info')

I think we should do that at the end, beside removing other temporary files created while this script was running. You don't do that right now.

@@ +51,5 @@
> +    # Get the path to tests and extensions directory
> +    testdir = 'tests' if os.path.exists('tests') \
> +        else '../../services/sync/tests/tps'
> +    extdir  = 'extensions' if os.path.exists('extensions') \
> +        else '../../services/sync/tps/extensions'

This will be broken on Windows. Paths always need the right delimiters. Also how does this work with packaged tests? I don't think it will find the right folder for the TPS tests.

Further please indent so the new line starts 4 chars to the right of the first ' appearance in the line before.

@@ +53,5 @@
> +        else '../../services/sync/tests/tps'
> +    extdir  = 'extensions' if os.path.exists('extensions') \
> +        else '../../services/sync/tps/extensions'
> +    assert(os.path.exists(testdir))
> +    assert(os.path.exists(extdir))

Please make sure we store the absolute path in that variables.

@@ +56,5 @@
> +    assert(os.path.exists(testdir))
> +    assert(os.path.exists(extdir))
> +
> +    # Update config file
> +    config = os.path.join(os.getcwd(), 'config/config.json.in')

Please use config_path here, and use join() when adding additional paths. Also use the current file location and not getcwd(), which will fail if you call the script from outside its dir.

@@ +58,5 @@
> +
> +    # Update config file
> +    config = os.path.join(os.getcwd(), 'config/config.json.in')
> +    replacements = {'__TESTDIR__':testdir, '__EXTENSIONDIR__':extdir}
> +    with open(config) as old_config_file, open(config[:-3], 'w') as config_file:

Please separate reading and writing. The updated config file should not stay in the same folder as the config.json.in.

@@ +76,5 @@
> +    print stdout, stderr
> +    retcode = process.returncode
> +    if not ignoreFailures:
> +        assert(retcode == 0)
> +    return retcode

Can't we simply use subprocess.check_call() here?

::: testing/tps/tps/cli.py
@@ +102,5 @@
>                  extensionDir = extensionDir.replace('/', '\\')
>  
> +    if not testDir or testDir == '__TESTDIR__':
> +        testDir = os.path.join(os.getcwd(), '..', '..',
> +                                    'services', 'sync', 'tests', 'tps')

Finding the right folder for tests and checking for __TESTDIR__ should should not be part of cli.py. Best is we default the test file option to 'all_tests.json' without any path. That way we get the path via the config and can join the manifest or test.

We may want to file a follow-up bug, so we can use the ManifestParser for TPS too. It offers us more flexibility.
Attachment #8421765 - Flags: review?(hskupin) → review-
Thanks for review Henrik, I still  need some direction before I upload the patch

(In reply to Henrik Skupin (:whimboo) from comment #3)
> @@ +51,5 @@
> > +    # Get the path to tests and extensions directory
> > +    testdir = 'tests' if os.path.exists('tests') \
> > +        else '../../services/sync/tests/tps'
> > +    extdir  = 'extensions' if os.path.exists('extensions') \
> > +        else '../../services/sync/tps/extensions'
> 
> This will be broken on Windows. Paths always need the right delimiters. Also
> how does this work with packaged tests? I don't think it will find the right
> folder for the TPS tests.
I tested this before and now again and it works, paths are handled by python here.

> @@ +58,5 @@
> > +
> > +    # Update config file
> > +    config = os.path.join(os.getcwd(), 'config/config.json.in')
> > +    replacements = {'__TESTDIR__':testdir, '__EXTENSIONDIR__':extdir}
> > +    with open(config) as old_config_file, open(config[:-3], 'w') as config_file:
> 
> Please separate reading and writing. The updated config file should not stay
> in the same folder as the config.json.in.
Right now the script dose it this way, and in that place it's is from where it takes the default config file if none is given.
If I change the target path here I have to change it cli.py too, if so, please let me know where should I put this file.

> > +    return retcode
> 
> Can't we simply use subprocess.check_call() here?
That was a great input many thanks :)

> ::: testing/tps/tps/cli.py
> @@ +102,5 @@
> >                  extensionDir = extensionDir.replace('/', '\\')
> >  
> > +    if not testDir or testDir == '__TESTDIR__':
> > +        testDir = os.path.join(os.getcwd(), '..', '..',
> > +                                    'services', 'sync', 'tests', 'tps')
> 
> Finding the right folder for tests and checking for __TESTDIR__ should
> should not be part of cli.py. Best is we default the test file option to
> 'all_tests.json' without any path. That way we get the path via the config
> and can join the manifest or test.

That's fine by me,  but what if user want's to give a file as an absolute path? we append it to path from manifest?
I would still default it to None then do this check:
>testDir = config.get('extensiondir')
>testfile = options.testfile if options.testfile else \
>            os.path.join(testDir, "all_tests.json")
Flags: needinfo?(hskupin)
(In reply to Cosmin Malutan from comment #4)
> > @@ +51,5 @@
> > > +    # Get the path to tests and extensions directory
> > > +    testdir = 'tests' if os.path.exists('tests') \
> > > +        else '../../services/sync/tests/tps'
> > > +    extdir  = 'extensions' if os.path.exists('extensions') \
> > > +        else '../../services/sync/tps/extensions'
> > 
> > This will be broken on Windows. Paths always need the right delimiters. Also
> > how does this work with packaged tests? I don't think it will find the right
> > folder for the TPS tests.
> I tested this before and now again and it works, paths are handled by python
> here.

Whatever it does for you, please update the code as instructed. Using hard-coded delimiters is not an option for us.

> > > +    # Update config file
> > > +    config = os.path.join(os.getcwd(), 'config/config.json.in')
> > > +    replacements = {'__TESTDIR__':testdir, '__EXTENSIONDIR__':extdir}
> > > +    with open(config) as old_config_file, open(config[:-3], 'w') as config_file:
> > 
> > Please separate reading and writing. The updated config file should not stay
> > in the same folder as the config.json.in.
> Right now the script dose it this way, and in that place it's is from where
> it takes the default config file if none is given.
> If I change the target path here I have to change it cli.py too, if so,
> please let me know where should I put this file.

No it doesn't do that. The INSTALL.sh is searching for config.json.in inside the created venv. You simply pick it from the config sub folder.

> > ::: testing/tps/tps/cli.py
> > @@ +102,5 @@
> > >                  extensionDir = extensionDir.replace('/', '\\')
> > >  
> > > +    if not testDir or testDir == '__TESTDIR__':
> > > +        testDir = os.path.join(os.getcwd(), '..', '..',
> > > +                                    'services', 'sync', 'tests', 'tps')
> > 
> > Finding the right folder for tests and checking for __TESTDIR__ should
> > should not be part of cli.py. Best is we default the test file option to
> > 'all_tests.json' without any path. That way we get the path via the config
> > and can join the manifest or test.
> 
> That's fine by me,  but what if user want's to give a file as an absolute
> path? we append it to path from manifest?
> I would still default it to None then do this check:
> >testDir = config.get('extensiondir')
> >testfile = options.testfile if options.testfile else \
> >            os.path.join(testDir, "all_tests.json")

I don't see why this should be an option to the user. When you run the install script, it should detect if we are in the repo or the packaged tests, and setup the paths correctly.
Flags: needinfo?(hskupin)
Attached patch patch_v2.0 (obsolete) — Splinter Review
Thanks for replay, I hope I got that right.
Attachment #8421765 - Attachment is obsolete: true
Attachment #8424712 - Flags: review?(hskupin)
Comment on attachment 8424712 [details] [diff] [review]
patch_v2.0

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

Looks already good. I haven't it tested yet, so please do the updates and with the next version I will give it a go.

::: testing/tps/configure.py
@@ +14,5 @@
> +import sys
> +import urllib
> +
> +virtualenv_url = 'https://raw.github.com/pypa/virtualenv/1.9.1/virtualenv.py'
> +final_log = """

I would call it usage_message

@@ +36,5 @@
> +    target = sys.argv[1]
> +    assert(target)
> +
> +    # Decide which python to use
> +    python = sys.argv[2] if len(sys.argv) == 3 else 'python'

Please use the optparse module as we do by default for handling options and arguments for scripts.

@@ +50,5 @@
> +    # Install TPS in environment
> +    subprocess.check_call(['python', 'setup.py', 'install'])
> +
> +    # Get the path to tests and extensions directory
> +    testdir = os.path.abspath('tests' if os.path.exists('tests')

If the script is not called from within the same folder it will fail. You will have to construct the correct path here. Also as usual please add comments, so here what this code is expected to do.

@@ +53,5 @@
> +    # Get the path to tests and extensions directory
> +    testdir = os.path.abspath('tests' if os.path.exists('tests')
> +                              else os.path.join('..', '..', 'services', 'sync',
> +                                                'tests', 'tps'))
> +    extdir = os.path.abspath('extensions' if os.path.exists('extensions')

Same here.

@@ +60,5 @@
> +    assert(os.path.exists(testdir))
> +    assert(os.path.exists(extdir))
> +
> +    # Update config file
> +    config_path = os.path.join('config', 'config.json.in')

I would call this config_in_path

::: testing/tps/tps/cli.py
@@ +73,5 @@
>  
>      configfile = options.configfile
>      if configfile is None:
>          if os.environ.get('VIRTUAL_ENV'):
> +            configfile = os.path.join(os.environ.get('VIRTUAL_ENV'), 'config.json')

Can you please test if that works with nested virtual environments? Means sourcing into another venv inside of a python script, while outside you are already sourced. What will this environment variable contain?

Maybe this logic should go as default in the optparse option?

Otherwise I like that we have the config on a way better path now.
Attachment #8424712 - Flags: review?(hskupin) → review-
Attached patch patch_v2.1 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #7)
> > +
> > +    # Get the path to tests and extensions directory
> > +    testdir = os.path.abspath('tests' if os.path.exists('tests')
> 
> If the script is not called from within the same folder it will fail. You
> will have to construct the correct path here. Also as usual please add
> comments, so here what this code is expected to do.
That's correct, most of the steps will fail if we are not in the same directory so I change the location to the scripts parent directory.

> > +            configfile = os.path.join(os.environ.get('VIRTUAL_ENV'), 'config.json')
> 
> Can you please test if that works with nested virtual environments? Means
> sourcing into another venv inside of a python script, while outside you are
> already sourced. What will this environment variable contain?
> 
> Maybe this logic should go as default in the optparse option?
> 
> Otherwise I like that we have the config on a way better path now.
I will get the path of the last activated environment, but when we activate the environment from inside a python script we don't set the environment variables.
ex:
># Activate tps environment
>tps_env = os.path.join(target, 'bin', 'activate_this.py')
>execfile(tps_env, dict(__file__=tps_env))
Right here we don't need it because we only install the package, but when running tps testruns from a python script we would need it.
Attachment #8424712 - Attachment is obsolete: true
Attachment #8427766 - Flags: review?(hskupin)
Comment on attachment 8427766 [details] [diff] [review]
patch_v2.1

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

Ok, mostly nits are remaining here. The next update might be the final one. Thanks Cosmin.

::: testing/tps/configure.py
@@ +34,5 @@
> +
> +
> +def main():
> +    parser = optparse.OptionParser()
> +    parser.add_option('--python',

Please also add '-p' here. Similar how virtualenv is doing it.

@@ +35,5 @@
> +
> +def main():
> +    parser = optparse.OptionParser()
> +    parser.add_option('--python',
> +                      action='store',

No need for that, but use the meta instead because this option needs a value.

@@ +38,5 @@
> +    parser.add_option('--python',
> +                      action='store',
> +                      type='string',
> +                      dest='python',
> +                      default='python',

I would default to None, and only use the python option for virtualenv if it has been specified. As of here it would have be required to be in the $PATH. And it doesn't have to be the case.

@@ +41,5 @@
> +                      dest='python',
> +                      default='python',
> +                      help='path to python interpreter')
> +    (options, args) = parser.parse_args(args=None, values=None)
> +    target = args[0]

nit: blank line above to separate the code

@@ +42,5 @@
> +                      default='python',
> +                      help='path to python interpreter')
> +    (options, args) = parser.parse_args(args=None, values=None)
> +    target = args[0]
> +    python = options.python

No need to do that. Keep using `options.python`.

@@ +45,5 @@
> +    target = args[0]
> +    python = options.python
> +    assert(target)
> +
> +    # As we handel file paths in this script we have to be sure we are in the

nit: handle

@@ +47,5 @@
> +    assert(target)
> +
> +    # As we handel file paths in this script we have to be sure we are in the
> +    # same directory with the script file
> +    os.chdir(os.path.abspath(os.path.dirname(__file__)))

I would prefer if you would save it off as the `here` variable at the top. Also please reverse the calls so it looks like:

os.path.dirname(os.path.abspath(__file__))

Further I don't see why we have to change the working directory here. Just use `here` as base whenever needed.

@@ +64,5 @@
> +    # Get the path to tests and extensions directory by checking check where the
> +    # tests and extensions directories are located
> +    testdir = os.path.abspath('tests' if os.path.exists('tests')
> +                              else os.path.join('..', '..', 'services', 'sync',
> +                                                'tests', 'tps'))

Use `here` as base as mentioned above.

::: testing/tps/tps/cli.py
@@ +73,5 @@
>  
>      configfile = options.configfile
>      if configfile is None:
>          if os.environ.get('VIRTUAL_ENV'):
> +            configfile = os.path.join(os.environ.get('VIRTUAL_ENV'), 'config.json')

Please save off the VIRTUAL_ENV value in a variable, so you only have to call it once.
Attachment #8427766 - Flags: review?(hskupin) → review-
Attached patch patch_v2.2 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #9)
> > +    parser.add_option('--python',
> > +                      action='store',
> > +                      type='string',
> > +                      dest='python',
> > +                      default='python',
> 
> I would default to None, and only use the python option for virtualenv if it
> has been specified. As of here it would have be required to be in the $PATH.
> And it doesn't have to be the case.
I defaulted it to None and if it's not given I will use sys.executable which is the python interpreter that called the current script so we are safe if we don't have python in $PATH.

Thanks for review Henrik.
Attachment #8427766 - Attachment is obsolete: true
Attachment #8428581 - Flags: review?(hskupin)
Comment on attachment 8428581 [details] [diff] [review]
patch_v2.2

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

When you have fixed those remaining issues, please also test how the script works via coversheet. The latter should also be setup to run inside an environment.

::: testing/tps/configure.py
@@ +38,5 @@
> +    parser = optparse.OptionParser()
> +    parser.add_option('-p', '--python',
> +                      type='string',
> +                      dest='python',
> +                      metavar='PYTHON_INTERPRETER',

Lets call it PYTHON_BIN

@@ +40,5 @@
> +                      type='string',
> +                      dest='python',
> +                      metavar='PYTHON_INTERPRETER',
> +                      default=None,
> +                      help='path to python interpreter')

"The Python interpreter to use"

@@ +49,5 @@
> +
> +    # Create a virtual environment
> +    urllib.urlretrieve(virtualenv_url, 'virtualenv.py')
> +    subprocess.check_call([options.python if options.python
> +                           else sys.executable, 'virtualenv.py', target])

Here you can always use `sys.executable` as first entry, but you should extend the array with ['-p', options.python] if it has been set.

@@ +57,5 @@
> +    execfile(tps_env, dict(__file__=tps_env))
> +
> +    # Install TPS in environment
> +    subprocess.check_call(['python',
> +                           os.path.join(here, 'setup.py'), 'install'])

nit: try to fill the first line as much as possible.

@@ +61,5 @@
> +                           os.path.join(here, 'setup.py'), 'install'])
> +
> +    # Get the path to tests and extensions directory by checking check where
> +    # the tests and extensions directories are located
> +    if os.path.exists(os.path.join(here, '..', '..', 'services', 'sync')):

I would save off `os.path.join(here, '..', '..', 'services', 'sync')` so you don't have to glue them together 3 times.

@@ +69,5 @@
> +                              'services', 'sync', 'tps', 'extensions')
> +    else:
> +        testdir = os.path.join(here, 'tests')
> +        extdir = os.path.join(here, 'extensions')
> +    assert(os.path.exists(testdir))

nit: please add a blank line above.

@@ +74,5 @@
> +    assert(os.path.exists(extdir))
> +
> +    # Update config file
> +    config_in_path = os.path.join(here, 'config', 'config.json.in')
> +    replacements = {'__TESTDIR__': testdir, '__EXTENSIONDIR__': extdir}

Please keep in mind that we want to store absolute paths here.
Attachment #8428581 - Flags: review?(hskupin) → review-
Attached patch patch_v2.2 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #11)
> When you have fixed those remaining issues, please also test how the script
> works via coversheet. The latter should also be setup to run inside an
> environment.
To make use of this script in actual version of coversheet we would have to make this change:
>diff --git a/coversheet/cli.py b/coversheet/cli.py
>index 17af897..36ebe32 100644
>--- a/coversheet/cli.py
>+++ b/coversheet/cli.py
>@@ -67,7 +67,7 @@ def main():
>                     help = "post results to Autolog")
>   parser.add_option("--testfile",
>                     action = "store", type = "string", dest = "testfile",
>-                    default = '../../services/sync/tests/tps/test_sync.js',
>+                    default = 'all_tests.json',
>                     help = "path to the test file to run "
>                            "[default: %default]")
>   parser.add_option("--logfile",
>diff --git a/coversheet/subproc.py b/coversheet/subproc.py
>index 3e99860..508c8a5 100644
>--- a/coversheet/subproc.py
>+++ b/coversheet/subproc.py
>@@ -139,8 +139,8 @@ class TPSSubproc():
>         if os.access(self.tpsenv, os.F_OK):
>             shutil.rmtree(self.tpsenv)
>         print "Installing tps in %s" % self.tpsenv
>-        self.run_process(["sh",
>-                          os.path.join(self.tpswd, "INSTALL.sh"),
>+        self.run_process(["python",
>+                          os.path.join(self.tpswd, "configure.py"),
>                           self.tpsenv],
>                           self.tpswd,
>                           ignoreFailures=True) 
The default testfile has to be changed because we add only the filename now. comment 5

> > +    # Update config file
> > +    config_in_path = os.path.join(here, 'config', 'config.json.in')
> > +    replacements = {'__TESTDIR__': testdir, '__EXTENSIONDIR__': extdir}
> 
> Please keep in mind that we want to store absolute paths here.
All are absolute paths because we join them with 'here' variable which is absolute path to the working directory.
Attachment #8428581 - Attachment is obsolete: true
Attachment #8429872 - Flags: review?(hskupin)
(In reply to Cosmin Malutan from comment #12)
> To make use of this script in actual version of coversheet we would have to
> make this change:

If we have to make changes you should file an issue and push a PR to the coversheet repository. Here it is clearly the wrong place for discussion. Also I was just asking you to test if coversheet and the new tps environment works together without causing problems.
Comment on attachment 8429872 [details] [diff] [review]
patch_v2.2

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

Two nits remain to be fixed. Can you do that today, so we can get it landed? r=me with those changes. Thanks.

::: testing/tps/configure.py
@@ +48,5 @@
> +    assert(target)
> +
> +    # Create a virtual environment
> +    urllib.urlretrieve(virtualenv_url, 'virtualenv.py')
> +    cmd_arg = [sys.executable, 'virtualenv.py', target]

nit: cmd_args given that it is an array with multiple elements inside.

@@ +50,5 @@
> +    # Create a virtual environment
> +    urllib.urlretrieve(virtualenv_url, 'virtualenv.py')
> +    cmd_arg = [sys.executable, 'virtualenv.py', target]
> +    if options.python:
> +        cmd_arg += ['-p', options.python]

cmd_arg.extend() please.
Attachment #8429872 - Flags: review?(hskupin) → review+
Attached patch patch_v2.4 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #14)
> Two nits remain to be fixed. Can you do that today, so we can get it landed?
> r=me with those changes. Thanks.
Sorry for returning to you for review, I know we discussed this before, but this patch is on mozilla-central and Andreea and Andrei can't land on it.
Attachment #8429872 - Attachment is obsolete: true
Attachment #8430097 - Flags: review?(hskupin)
(In reply to Cosmin Malutan from comment #15)
> Sorry for returning to you for review, I know we discussed this before, but
> this patch is on mozilla-central and Andreea and Andrei can't land on it.

This is not what r? is used for! Please check MDN for what the various flags standing for.
Comment on attachment 8430097 [details] [diff] [review]
patch_v2.4

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

So I finally gave it a test and here is what needs to be updated:

* When configure.py gets run without an argument it exists with "IndexError: list index out of range". It should show the help
* We might want to rename configure.py to create_venv.py so it's clear what it is used for
* As mentioned earlier, the testdir and extensiondir path are still not absolute. They contain '../../' in case of an in-tree venv

::: testing/tps/configure.py
@@ +26,5 @@
> +
> +See runtps --help for all options
> +
> +To change your TPS config, please edit the file:
> +{TARGET}/config.json

I missed that myself. So please move this up before the execution line. Also make sure about the usage of blank lines in the help message.
Attachment #8430097 - Flags: review?(hskupin) → review-
Attached patch patch_v3.0 (obsolete) — Splinter Review
(In reply to Henrik Skupin (:whimboo) from comment #17)
> * As mentioned earlier, the testdir and extensiondir path are still not
> absolute. They contain '../../' in case of an in-tree venv
I activated the environment while in coversheet env and it worked and I had absolute paths, here we join a "../" path to an absolute path so the result should be an absolute path, I tested this on windows too.
Attachment #8430097 - Attachment is obsolete: true
Attachment #8431460 - Flags: review?(hskupin)
Attached patch patch_v3.1 (obsolete) — Splinter Review
I had a typo mistake.
Attachment #8431460 - Attachment is obsolete: true
Attachment #8431460 - Flags: review?(hskupin)
Attachment #8431495 - Flags: review?(hskupin)
Comment on attachment 8431495 [details] [diff] [review]
patch_v3.1

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

::: testing/tps/create_venv.py
@@ +1,1 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public

Lets add a shebang and make the file executable.

@@ +19,5 @@
> +usage_message = """
> +***********************************************************************
> +
> +
> +To run TPS, activate the virtualenv using:

Why this additional blank line compared to the last patch?

@@ +29,5 @@
> +To execute tps using:
> +    runtps --binary=/path/to/firefox
> +
> +See runtps --help for all options
> +***********************************************************************

nit: Missing blank line above.

@@ +45,5 @@
> +                      help='The Python interpreter to use.')
> +    (options, args) = parser.parse_args(args=None, values=None)
> +
> +    if len(args) != 1:
> +         parser.error('Path to the target environment has to be specified')

nit: remove target. Also you might want to update the usage string, so it lists the env argument.

@@ +66,5 @@
> +                           'install'])
> +
> +    # Get the path to tests and extensions directory by checking check where
> +    # the tests and extensions directories are located
> +    sync_dir = os.path.join(here, '..', '..', 'services', 'sync')

I really don't want to repeat myself three times Cosmin! If something is unclear ask but do not claim twice that the result of this action is an absolute path. Whenever dots can be found, it's clearly relative. So please finally fix that.

What I have in my config.json is:
"/mozilla/code/firefox/gecko-dev/testing/tps/../../services/sync/tps/extensions"
Attachment #8431495 - Flags: review?(hskupin) → review-
Attached patch patch_v3.2 (obsolete) — Splinter Review
Fixed those, thanks for review.
Attachment #8431495 - Attachment is obsolete: true
Attachment #8432554 - Flags: review?(hskupin)
Comment on attachment 8432554 [details] [diff] [review]
patch_v3.2

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

::: testing/tps/create_venv.py
@@ +1,1 @@
> +#!/usr/bin/python

This is not portable and will fail on various systems. Also see: http://stackoverflow.com/questions/6908143/should-i-put-shebang-in-python-scripts

@@ +35,5 @@
> +"""
> +virtualenv_url = 'https://raw.github.com/pypa/virtualenv/1.9.1/virtualenv.py'
> +
> +if sys.platform == 'win32':
> +    bin_name = 'Scripts/activate.bat'

As mentioned a couple of days ago, please DO NOT make use of hard-coded delimiters, but always use os.path.join().

@@ +36,5 @@
> +virtualenv_url = 'https://raw.github.com/pypa/virtualenv/1.9.1/virtualenv.py'
> +
> +if sys.platform == 'win32':
> +    bin_name = 'Scripts/activate.bat'
> +    activate_env = 'Scripts/activate_this.py'

So that means you haven't tested this patch on Windows before given that you found this problem now? Please really make sure to test it on all supported platforms.
Attachment #8432554 - Flags: review?(hskupin) → review-
Attached patch patch_v4.0 (obsolete) — Splinter Review
I updated the patch and I tested it on all platforms.
Attachment #8432554 - Attachment is obsolete: true
Attachment #8433181 - Flags: review?(hskupin)
Comment on attachment 8433181 [details] [diff] [review]
patch_v4.0

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

The file mode still has not been updated. Please make the new script executable by the user. With that you get me r+.
Attachment #8433181 - Flags: review?(hskupin) → review+
The file mode is 100755 this is executable | http://stackoverflow.com/questions/22257640/git-diff-says-old-mode-is-100644-and-new-mode-is-100755-but-nops#comment-33805778
I set the mode again(chmod +x create_venv.py) but I had no difference in git.
I created issue https://github.com/jonallengriffin/coversheet/issues/19 for swiching to create_venv.py.
Hm, interesting. When I check the raw attachment the mode is indeed 755. Something might have gone wrong when applying the patch. I will check locally and get it landed on mozilla-central, after the changes to coversheet have been landed and deployed.
Comment on attachment 8433181 [details] [diff] [review]
patch_v4.0

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

::: testing/tps/create_venv.py
@@ +104,5 @@
> +    # Clean up files created by setup.py
> +    shutil.rmtree(os.path.join(here, 'build'))
> +    shutil.rmtree(os.path.join(here, 'dist'))
> +    shutil.rmtree(os.path.join(here, 'tps.egg-info'))
> +    os.remove(os.path.join(here, 'virtualenv.py'))

You have to remove `virtualenv.py*` here, given that the compiled `virtualenv.pyc` file will still be around. With the update you can carry over the r+.
Attached patch patch_v4.1Splinter Review
Thanks
Attachment #8433181 - Attachment is obsolete: true
Attachment #8433302 - Flags: review?(hskupin)
https://hg.mozilla.org/mozilla-central/rev/e6f580100975

Once we can see that the production CI is working fine, lets backport this patch to the other branches.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Attached patch followup.patch (obsolete) — Splinter Review
Today I tried to run tps testruns via coversheet on windows, and it looks like activating the environment in python script is not sufficient for windows, but we have to use the python executable from the environment for the module to be installed in environment and not globally.
With this changes I had to call coversheet with a pulsefile and aconfigfile argumens in order to work on windows, if we install it globally we have to call the covershet.exe, or we could install it in a MinGW environment.
Attachment #8434207 - Flags: review?(hskupin)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
This already missed Fx30, FWIW...
(In reply to Cosmin Malutan from comment #30)
> Today I tried to run tps testruns via coversheet on windows, and it looks

I already asked you in comment 22 if you have tested on all platforms. And you said you did. So what's going on now? I totally don't understand why it should fail again. I don't see that we want to fix it in this bug, given that the code has already been backported. File a new bug for this issue.
Attachment #8434207 - Attachment is obsolete: true
Attachment #8434207 - Flags: review?(hskupin)
(In reply to Henrik Skupin (:whimboo) from comment #33)
> I already asked you in comment 22 if you have tested on all platforms. And
> you said you did. So what's going on now? I totally don't understand why it
> should fail again. I don't see that we want to fix it in this bug, given
> that the code has already been backported. File a new bug for this issue.
I missed that it installs it globally when I tested it, I just checked that it creates the environment and doesn't throw exceptions on windows, sorry for that. I filed bug 1020880 as a follow up.
Target Milestone: mozilla32 → mozilla33
This landed for Firefox 32.0 on mozilla-central and not for 33.0. Restoring flags.
Target Milestone: mozilla33 → mozilla32
Product: Testing → Testing Graveyard
You need to log in before you can comment on or make changes to this bug.