diff mbox series

[01/13] ci: also run linux-gcc pipeline with python-3.7 environment

Message ID 20191207003333.3228-2-yang.zhao@skyboxlabs.com
State New, archived
Headers show
Series git-p4: python3 compatibility | expand

Commit Message

Yang Zhao Dec. 7, 2019, 12:33 a.m. UTC
git-p4.py includes support for python-3, but this was not previously
validated in CI. Lets actually do that.

There is no tangible benefit to repeating python-3 tests for all
environments, so only limit it to linux-gcc for now.

Signed-off-by: Yang Zhao <yang.zhao@skyboxlabs.com>
---

I assert that we don't need to run python3 tests on more platforms,
but is this actually reasonable?

 azure-pipelines.yml | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

SZEDER Gábor Dec. 10, 2019, 10:30 a.m. UTC | #1
On Fri, Dec 06, 2019 at 04:33:19PM -0800, Yang Zhao wrote:
> git-p4.py includes support for python-3, but this was not previously
> validated in CI. Lets actually do that.
> 
> There is no tangible benefit to repeating python-3 tests for all
> environments, so only limit it to linux-gcc for now.

In the subject line and the commit message body you speak about CI in
general, without sinling out a particular CI system ...

>  azure-pipelines.yml | 11 +++++++++++
>  1 file changed, 11 insertions(+)

... but the patch only modifies 'azure-pipelines.yml', and not
'.travis.yml'.

> diff --git a/azure-pipelines.yml b/azure-pipelines.yml
> index 37ed7e06c6..d5f9413248 100644
> --- a/azure-pipelines.yml
> +++ b/azure-pipelines.yml
> @@ -331,7 +331,18 @@ jobs:
>    displayName: linux-gcc
>    condition: succeeded()
>    pool: Hosted Ubuntu 1604
> +  strategy:
> +    matrix:
> +      python27:
> +        python.version: '2.7'
> +      python37:
> +        python.version: '3.7'
>    steps:
> +  - task: UsePythonVersion@0
> +    inputs:
> +      versionSpec: '$(python.version)'
> +  - bash: |
> +      echo "##vso[task.setvariable variable=python_path]$(which python)"

I don't speak 'azure-pipelines.yml', so question: will this build Git
and run the whole test suite twice, once with Python 2.7 and once with
3.7?  I'm asking because 'git-p4' is the one and only Python script we
have, with no plans for more, so running the whole test suite with a
different Python version for a second time instead of running only the
'git-p4'-specific tests (t98*) seems to be quite wasteful.

Furthermore, this is the first patch of the series, with all the
Python3 fixes in subsequent commits, so the Azure Pipelines build with
Python 3.7 would fail with only this patch, wouldn't it?  I think this
patch should be the last in the series, after all the Python 2 vs 3
issues are sorted out.

>    - bash: |
>         test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1
>  
> -- 
> 2.21.0.windows.1
>
Yang Zhao Dec. 10, 2019, 7:11 p.m. UTC | #2
On Tue, Dec 10, 2019 at 2:30 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> On Fri, Dec 06, 2019 at 04:33:19PM -0800, Yang Zhao wrote:
> > diff --git a/azure-pipelines.yml b/azure-pipelines.yml
> > index 37ed7e06c6..d5f9413248 100644
> > --- a/azure-pipelines.yml
> > +++ b/azure-pipelines.yml
> > @@ -331,7 +331,18 @@ jobs:
> >    displayName: linux-gcc
> >    condition: succeeded()
> >    pool: Hosted Ubuntu 1604
> > +  strategy:
> > +    matrix:
> > +      python27:
> > +        python.version: '2.7'
> > +      python37:
> > +        python.version: '3.7'
> >    steps:
> > +  - task: UsePythonVersion@0
> > +    inputs:
> > +      versionSpec: '$(python.version)'
> > +  - bash: |
> > +      echo "##vso[task.setvariable variable=python_path]$(which python)"
>
> I don't speak 'azure-pipelines.yml', so question: will this build Git
> and run the whole test suite twice, once with Python 2.7 and once with
> 3.7?  I'm asking because 'git-p4' is the one and only Python script we
> have, with no plans for more, so running the whole test suite with a
> different Python version for a second time instead of running only the
> 'git-p4'-specific tests (t98*) seems to be quite wasteful.

The CI scripts as it is currently does not separate compiling and testing for
non-Windows builds. I don't see a good way to only run a specific set of tests
given a particular environment without re-architecturing the CI pipeline.

Furthermore, there's a step in the build that hard-codes the
environment's python
path into the installed version of the script. This complicates being
able to even create
a `git-p4` that runs under different python environments in Azure
Pipelines due to how
`UsePythonVersion@0` pulls python into version-specific directories.
I haven't dug into
why this hardcoding is done in the first place.

So, the question is if it's worth doing this work now when the desire
seems to be dropping
python-2.7 completely in the (near?) future.
SZEDER Gábor Dec. 12, 2019, 2:13 p.m. UTC | #3
On Tue, Dec 10, 2019 at 11:11:09AM -0800, Yang Zhao wrote:
> On Tue, Dec 10, 2019 at 2:30 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> > On Fri, Dec 06, 2019 at 04:33:19PM -0800, Yang Zhao wrote:
> > > diff --git a/azure-pipelines.yml b/azure-pipelines.yml
> > > index 37ed7e06c6..d5f9413248 100644
> > > --- a/azure-pipelines.yml
> > > +++ b/azure-pipelines.yml
> > > @@ -331,7 +331,18 @@ jobs:
> > >    displayName: linux-gcc
> > >    condition: succeeded()
> > >    pool: Hosted Ubuntu 1604
> > > +  strategy:
> > > +    matrix:
> > > +      python27:
> > > +        python.version: '2.7'
> > > +      python37:
> > > +        python.version: '3.7'
> > >    steps:
> > > +  - task: UsePythonVersion@0
> > > +    inputs:
> > > +      versionSpec: '$(python.version)'
> > > +  - bash: |
> > > +      echo "##vso[task.setvariable variable=python_path]$(which python)"
> >
> > I don't speak 'azure-pipelines.yml', so question: will this build Git
> > and run the whole test suite twice, once with Python 2.7 and once with
> > 3.7?  I'm asking because 'git-p4' is the one and only Python script we
> > have, with no plans for more, so running the whole test suite with a
> > different Python version for a second time instead of running only the
> > 'git-p4'-specific tests (t98*) seems to be quite wasteful.
> 
> The CI scripts as it is currently does not separate compiling and testing for
> non-Windows builds. I don't see a good way to only run a specific set of tests
> given a particular environment without re-architecturing the CI pipeline.

Building git and running the test suite is encapsulated in the
'ci/run-build-and-tests.sh' script, while installing dependencies is
encapsulated in 'ci/install-dependencies.sh', just in case Azure
Pipelines Linux images don't contain both Python 2 and 3 (Travis CI
images contain 2.7 and 3.5)  So I don't think it's necessary to touch
'azure-pipelines.yml' or '.travis.yml' at all.

> Furthermore, there's a step in the build that hard-codes the
> environment's python
> path into the installed version of the script. This complicates being
> able to even create
> a `git-p4` that runs under different python environments in Azure
> Pipelines due to how
> `UsePythonVersion@0` pulls python into version-specific directories.

The PYTHON_PATH that we build 'git p4' with can be a symbolink link,
and then choosing which Python version to use is only a matter of
pointing that symbolic link to the python binary of the desired
version.

In fact our default PYTHON_PATH is '/usr/bin/python', which is a
symbolic link pointing to 'python2.7' on Ubuntu 16.04, including the
Travis CI's images that we use.

> I haven't dug into
> why this hardcoding is done in the first place.
> 
> So, the question is if it's worth doing this work now when the desire
> seems to be dropping
> python-2.7 completely in the (near?) future.
> 
> -- 
> Yang
Yang Zhao Dec. 12, 2019, 5:04 p.m. UTC | #4
On Thu, Dec 12, 2019 at 6:13 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> > The CI scripts as it is currently does not separate compiling and testing for
> > non-Windows builds. I don't see a good way to only run a specific set of tests
> > given a particular environment without re-architecturing the CI pipeline.
>
> Building git and running the test suite is encapsulated in the
> 'ci/run-build-and-tests.sh' script, while installing dependencies is
> encapsulated in 'ci/install-dependencies.sh', just in case Azure
> Pipelines Linux images don't contain both Python 2 and 3 (Travis CI
> images contain 2.7 and 3.5)  So I don't think it's necessary to touch
> 'azure-pipelines.yml' or '.travis.yml' at all.

Yes, and this is implemented as a single step as far as the CI
pipeline is concerned. It does not produce a build artifact that can
then be loaded into multiple environments for running tests.

Unless there's a very good reason to _not_ use Azure Pipeline's
built-in Python version selection support, I believe it's more
desirable in the long-run to leverage the feature rather than maintain
some custom solution.
SZEDER Gábor Dec. 12, 2019, 5:15 p.m. UTC | #5
On Thu, Dec 12, 2019 at 09:04:24AM -0800, Yang Zhao wrote:
> On Thu, Dec 12, 2019 at 6:13 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > > The CI scripts as it is currently does not separate compiling and testing for
> > > non-Windows builds. I don't see a good way to only run a specific set of tests
> > > given a particular environment without re-architecturing the CI pipeline.
> >
> > Building git and running the test suite is encapsulated in the
> > 'ci/run-build-and-tests.sh' script, while installing dependencies is
> > encapsulated in 'ci/install-dependencies.sh', just in case Azure
> > Pipelines Linux images don't contain both Python 2 and 3 (Travis CI
> > images contain 2.7 and 3.5)  So I don't think it's necessary to touch
> > 'azure-pipelines.yml' or '.travis.yml' at all.
> 
> Yes, and this is implemented as a single step as far as the CI
> pipeline is concerned. It does not produce a build artifact that can
> then be loaded into multiple environments for running tests.

I don't understand what artifact should be loaded into what
environments...

> Unless there's a very good reason to _not_ use Azure Pipeline's
> built-in Python version selection support, I believe it's more
> desirable in the long-run to leverage the feature rather than maintain
> some custom solution.

Azure Pipelines's built-in Python version selection support only works
on Azure Pipelines, therefore it's more desirable to have a general
solution.
Yang Zhao Dec. 12, 2019, 7:02 p.m. UTC | #6
On Thu, Dec 12, 2019 at 9:15 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Thu, Dec 12, 2019 at 09:04:24AM -0800, Yang Zhao wrote:
> > Unless there's a very good reason to _not_ use Azure Pipeline's
> > built-in Python version selection support, I believe it's more
> > desirable in the long-run to leverage the feature rather than maintain
> > some custom solution.
>
> Azure Pipelines's built-in Python version selection support only works
> on Azure Pipelines, therefore it's more desirable to have a general
> solution.

That's fair. However, if we actually want to have something unified
that works for Linux and macOS (t90** isn't run on Windows afaict)
then I won't have the bandwidth for it in the near term. I'd be more
inclined to drop the CI changes from the series if we don't want a
stop-gap in the meantime.
diff mbox series

Patch

diff --git a/azure-pipelines.yml b/azure-pipelines.yml
index 37ed7e06c6..d5f9413248 100644
--- a/azure-pipelines.yml
+++ b/azure-pipelines.yml
@@ -331,7 +331,18 @@  jobs:
   displayName: linux-gcc
   condition: succeeded()
   pool: Hosted Ubuntu 1604
+  strategy:
+    matrix:
+      python27:
+        python.version: '2.7'
+      python37:
+        python.version: '3.7'
   steps:
+  - task: UsePythonVersion@0
+    inputs:
+      versionSpec: '$(python.version)'
+  - bash: |
+      echo "##vso[task.setvariable variable=python_path]$(which python)"
   - bash: |
        test "$GITFILESHAREPWD" = '$(gitfileshare.pwd)' || ci/mount-fileshare.sh //gitfileshare.file.core.windows.net/test-cache gitfileshare "$GITFILESHAREPWD" "$HOME/test-cache" || exit 1