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.do...@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 _______________________________________________ Python-Dev mailing list Python-Dev@python.org https://mail.python.org/mailman/listinfo/python-dev Unsubscribe: https://mail.python.org/mailman/options/python-dev/archive%40mail-archive.com
