osdir.com

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

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


As a slight aside, 8 out of 8 buildbot messages on the PR look like
false positives, and none of the true positives sent a message. What
happened there?

On 07Dec2018 0716, Steve Dower wrote:
> 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