| Message ID | 20191207003333.3228-2-yang.zhao@skyboxlabs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | git-p4: python3 compatibility | expand |
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 >
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.
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
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.
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.
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 --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
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(+)