[Python-Dev] I reverted "Add Windows App Store package" change
Steve Dower
steve.dower at python.org
Fri Dec 7 10:16:03 EST 2018
Thanks for fixing up the buildbots, but please be a little more thorough
before making publicly incorrect statements.
The change is 99% adding new files that are not part of Python, but
participate in the build for Windows (and are available and incredibly
useful for everyone). These are essentially zero-impact. In the PR I
pointed out exactly where to look for interesting changes and it didn't
help get any traction :)
The other changes are either in Windows-only files or tests. The one
exception is venv, where they are in "if os.name=='nt'" blocks. I also
pinged our venv expert a few times with no response.
The PR was put up two *months* ago, not one week. In that time, it's
been heavily tested by myself and a number of people who I *am* able to
share the output of this with. I've also been chatting with the release
manager for 3.7 about it and he's been on board (the words may have been
"on your own head", but that's close enough to "on board")
It didn't break all Windows buildbots.
That said, it's totally my fault for merging and then not watching. Also
for not submitting custom builds to all the buildbots (Can we still do
that? I'm not seeing any UI right now... I did run a number of test
release builds on the release machine, so I knew that was going to be okay.)
Now, to answer your questions: releasing in a new package with slightly
different semantics *has* to be somewhat experimental, because nobody
can test it until it's been released. This isn't about iterating on this
change in master, it's about getting broad public feedback on a release
mechanism that currently does not exist.
Historically, when changes to the part you point out have been extracted
out from their context, they've been rejected on the basis that they
aren't necessary. But now the original PR is broken (because the tests
don't pass), and so there will never be a need. This time I decided to
specifically point out numerous times where the interesting changes were
and was not able to get reviews from bpo/github (I did get many reviews
and some testing from others).
As it happens, I split out many changes to do with pathconfig that came
from this. You rejected them because they weren't necessary :)
Most of the code is a Python script to do "make install" on Windows. A
very common request is "how do I copy built files into the right place",
and the answer has always been "it's complicated". Now we can measure
how complicated it is in terms of lines of Python code, but at least the
answer is "run this script". Yes, it could go into its own PR, but that
runs into the context problem again. If I'm going to be forced to bypass
review on a dependency just to make it possible to merge a real change,
then they may as well go together.
(The new script is Black formatted, so probably I could get Lukasz to
approve it on its own? :) )
I hope that explains a bit better. People wait two months and more for
simpler reviews, but part of me being a core developer is to accelerate
that for Windows-targeted work. That's all I did here, and I'm happy
with my reasoning.
I've reposted the PRs at https://github.com/python/cpython/pull/11027
and https://github.com/python/cpython/pull/11028 with fixes for the one
issue that Victor couldn't investigate. If someone can get a Windows
buildbot to run against them that would be great (not you Zach - your
buildbots were fine :) ).
Cheers,
Steve
On 07Dec2018 0435, Victor Stinner wrote:
> 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
>
More information about the Python-Dev
mailing list