osdir.com

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Python-Dev] I reverted "Add Windows App Store package" change


Hi,

I had to revert a change since it broke buildbots. Sadly, I don't have
the bandwidth to investigate the failures and try to fix the change
:-(


A large change has been pushed into the 3.7 and master branches to
"Add Windows App Store package":

"Release Windows Store app containing Python"
https://bugs.python.org/issue34977

These changes broke all Windows buildbots:

"Almost all Windows buildbots are failing to compile"
https://bugs.python.org/issue35437

So I reverted these two changes.

It's a large change which mostly add new files, but also make changes
in different files:
---
commit 468a15aaf9206448a744fc5eab3fc21f51966aad
Author: Steve Dower <steve.dower at microsoft.com>
Date:   Thu Dec 6 21:09:20 2018 -0800

    bpo-34977: Add Windows App Store package (GH-10245)

 .azure-pipelines/windows-appx-test.yml             |  65 +++
 .gitattributes                                     |   1 +
 Doc/make.bat                                       |   2 +
 Lib/test/test_pathlib.py                           |   2 +-
 Lib/test/test_venv.py                              |   1 +
 Lib/venv/__init__.py                               |  49 +-
 .../2018-10-30-13-39-17.bpo-34977.0l7_QV.rst       |   1 +
 PC/classicAppCompat.can.xml                        | Bin 0 -> 3978 bytes
 PC/classicAppCompat.cat                            | Bin 0 -> 10984 bytes
 PC/classicAppCompat.sccd                           | Bin 0 -> 18503 bytes
 PC/getpathp.c                                      |   8 +-
 PC/icons/pythonwx150.png                           | Bin 0 -> 8187 bytes
 PC/icons/pythonwx44.png                            | Bin 0 -> 2232 bytes
 PC/icons/pythonx150.png                            | Bin 0 -> 8271 bytes
 PC/icons/pythonx44.png                             | Bin 0 -> 2178 bytes
 PC/icons/pythonx50.png                             | Bin 0 -> 2190 bytes
 PC/launcher.c                                      | 220 +++++++-
 PC/layout/__init__.py                              |   0
 PC/layout/__main__.py                              |  14 +
 PC/layout/main.py                                  | 612 +++++++++++++++++++++
 PC/layout/support/__init__.py                      |   0
 PC/layout/support/appxmanifest.py                  | 487 ++++++++++++++++
 PC/layout/support/catalog.py                       |  44 ++
 PC/layout/support/constants.py                     |  28 +
 .../support/distutils.command.bdist_wininst.py     |  25 +
 PC/layout/support/filesets.py                      | 100 ++++
 PC/layout/support/logging.py                       |  93 ++++
 PC/layout/support/options.py                       | 122 ++++
 PC/layout/support/pip.py                           |  79 +++
 PC/layout/support/props.py                         | 110 ++++
 {Tools/nuget => PC/layout/support}/python.props    |   0
 PC/pylauncher.rc                                   |   6 +
 PC/python_uwp.cpp                                  | 226 ++++++++
 PC/store_info.txt                                  | 146 +++++
 PCbuild/_tkinter.vcxproj                           |   6 +
 PCbuild/find_msbuild.bat                           |  10 +
 PCbuild/pcbuild.proj                               |   3 +
 PCbuild/pcbuild.sln                                |  72 +++
 PCbuild/python.props                               |   3 +-
 PCbuild/python_uwp.vcxproj                         |  86 +++
 PCbuild/pythoncore.vcxproj                         |  15 +
 PCbuild/pythonw_uwp.vcxproj                        |  86 +++
 PCbuild/venvlauncher.vcxproj                       |  85 +++
 PCbuild/venvwlauncher.vcxproj                      |  85 +++
 Tools/msi/buildrelease.bat                         |  13 +-
 Tools/msi/make_appx.ps1                            |  71 +++
 Tools/msi/make_cat.ps1                             |  34 ++
 Tools/msi/make_zip.proj                            |   9 +-
 Tools/msi/make_zip.py                              | 250 ---------
 Tools/msi/sdktools.psm1                            |  43 ++
 Tools/msi/sign_build.ps1                           |  34 ++
 Tools/nuget/make_pkg.proj                          |  38 +-
 52 files changed, 3053 insertions(+), 331 deletions(-)
---

Note: the commit message is a single line.


Now I have questions :-)


It seems like the change is "experimental":
https://bugs.python.org/issue34977#msg331267

"""
Making the *release experimental* as part of the next 3.7 update was
approved by Ned (over email), so I merged the build. As soon as we
snap for the RC I'll kick off an update and make the store page
public, and hopefully can promote it enough to get eyes on it.

Unfortunately, I discovered as part of a test submission that the
minimum Windows version matters, and it's a version that hasn't been
rolled out fully yet *because of some bugs*, so there may not be that
many people who can use it this first time. But that will *improve
over time*, and I'm sure I can find enough people to flush out issues
before the next release (anyone on the Windows Insiders program should
be fine).
"""

If it's experimental, why pushing it *right now* to the 3.7 branch?
Can't we wait until it's stable enough? Even if it's stable, I'm not
sure sure that it should go into 3.7 which is a stable branch.

I guess that Steve would like to get this feature into 3.7.2. Ned
Deily (3.7 release changer) approved this change.

The pull request was merged one week after it has been created, I
don't see any review.
https://github.com/python/cpython/pull/10245

In general, I'm fine with merging changes without any kind of review,
when the change is small. I'm doing it *frequently* when I'm confident
that the change is small and safe enough. But here, the change is
quite large. Sadly, I know the problem: the lack of available
developers for review. There are very few developers who know Windows
and are available to provide a review. Sometimes, Zachary Ware or Eryk
Sun are around and can help.

I'm fine with iterating in the master branch, since the change seems
to be separated from CPython core: it mostly add new files. My concern
is more about changes on "Python itself": the venv and tests changes.

In the middle of the large change, there is small change on the venv module:

diff --git a/Lib/venv/__init__.py b/Lib/venv/__init__.py
index 043420897e..5438b0d4e5 100644
--- a/Lib/venv/__init__.py
+++ b/Lib/venv/__init__.py
@@ -64,10 +64,11 @@ class EnvBuilder:
         self.system_site_packages = False
         self.create_configuration(context)
         self.setup_python(context)
+        if not self.upgrade:
+            self.setup_scripts(context)
         if self.with_pip:
             self._setup_pip(context)
         if not self.upgrade:
-            self.setup_scripts(context)
             self.post_setup(context)
         if true_system_site_packages:

Moreover, the commit also changes tests:

* Lib/test/test_pathlib.py
* Lib/test/test_venv.py

The commit message is just "bpo-34977: Add Windows App Store package
(GH-10245)", it doesn't explain these changes.

I would prefer to see these changes extracted into separated commits.
Sadly, right now on the cpython project on GitHub, we are only allowed
to squash all commits into a single commit (note: the individual
commit in the PR doesn't explain these venv/tests changes neither). So
I suggest to create multiple PRs (at least one for venv+pathlib
changes and a second for the Windows AppStore).

I guess that these changes are bugfixes or enhancement. Having a
separated change should also ease to backport these changes up to 3.6
;-)

--

Jeremy Kloth just created:
"Correctly detect installed SDK versions"
https://bugs.python.org/issue35433

I'm not sure if it's related to the AppStore change?

Victor
-- 
Night gathers, and now my watch begins. It shall not end until my death.