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
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
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
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
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
On 07Dec2018 0716, Steve Dower wrote:
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.)
As to the breakage on my buildbot (https://buildbot.python.org/all/#/workers/12), it seems to be caused by not having VS2017 (my guess due to missing C++ headers). Unless something has changed recently, the guides still state the building with VS2015 is OK and I would like to keep my buildbot testing at the *minimum* required components for building (hence the VS9.0 builder) to keep us honest with any changes. I have no problem adding VS2017 for newer Python versions, but I think is build scripts always chose the latest version of the build tools thus making testing with older toolsets impossible. -- Jeremy Kloth
On Fri, Dec 7, 2018 at 9:17 AM Steve Dower
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.)
The UX is not great, but you can trigger a custom builder build by pushing to the `buildbot-custom` branch of python/cpython. Eventually, time will materialize to make it possible for core devs to trigger builds from branches on their personal fork since we do have proper auth now. -- Zach
Le ven. 7 déc. 2018 à 16:42, Steve Dower
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?
I don't know why the PR didn't get notifications about the regression. I got something more than 20 emails in my buildbot mail box :-) Let me have a look: https://github.com/python/cpython/pull/10245#issuecomment-445184041 There are multiple messages related to the 'custom' build: Pablo's created a custom build ("buildbot-custom" Git branch) which reverted your PR in master. It seems like Pablo's buildbot which sends notifications to PRs decided that his commit is attached to your PR. You can ignore all notifications from the "custom" build. The GitHub notifications are still experimental. If someone is interested, see how you can enhance these notifications ;-) s390x Debian 3.7 failed on "git clone". Well, that's a random network issue. We are working on trying to make this specific issue quiet: https://github.com/python/buildmaster-config/pull/69 Anyone is welcome to help to enhance the Python CIs, there are plenty of things to enhance ;-) Buildbots are complex beast, but the situation should be better nowadays than 2 years ago. The number of random failure is quite low. Maybe random failures are more visible today because notifications are sent on state change (basically when the color changes). Two years ago, all buildbots were always red (fail), so we never get any notification :-) I'm still fixing random failures every week: test_eintr, test_socket, etc. Victor
Le ven. 7 déc. 2018 à 16:16, Steve Dower
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.
Yeah, the lack of review is an issue in Python, I'm well aware of that... But I guess that it would be easier to get a review on a change modifying a single file (venv) than one PR modifying 52 files? Usually, I'm scared by giant PRs and I just skip them :-)
The PR was put up two *months* ago, not one week.
Oh, wait... I read Oct 30 and I counted 7 days... sorry :-) November becomes December, not October. Sorry about that.
That said, it's totally my fault for merging and then not watching.
That's fine. A revert doesn't mean that your change is wrong. The only intent is to repair the CI as soon as possible to spot other regressions, and give us more time to design the proper fix: https://pythondev.readthedocs.io/ci.html#revert-on-fail
(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.)
The current process is described at: https://devguide.python.org/buildbots/#custom-builders
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.
I don't think that there is a general rule. It's usually decided on a case by case basis, and I know that each core dev has a different opinion on this question :-) I do prefer multiple small commits. I would be simpler if it would be possible to have a "patch serie": list of pull requests, or a single PR with multiple commits but don't squash them.
As it happens, I split out many changes to do with pathconfig that came from this. You rejected them because they weren't necessary :)
I'm sorry, I don't recall, which changes?
I hope that explains a bit better.
It does, thanks :-) Victor
On 12/7/2018 11:31 AM, Victor Stinner wrote:
I would be simpler if it would be possible to have a "patch serie": list of pull requests,
One can make an 'index issue' with multiple dependencies, each with a PR. I do this for multiple independent changes to a module or related modules.
or a single PR with multiple commits but don't squash them.
Already possible on Github. If one clicks [View Changes], multiple
commits in a PR are combined for display purposes into a net PR diff.
But the individual commits can be examined and reviewed individually and
are not squashed into one by github (and should not be by authors) until
the merge into cpython.
Typically, authors effectively 'squash' all initial changes into one
commit, make a PR, and only add additional commits in review. But
authors *can* split the initial editing into multiple commits with an
eye to easing review -- and record information for future maintainers.
Simple bugfix example:
On 07Dec2018 1340, Terry Reedy wrote:
Simple bugfix example:
Add test to test_mod that fails with TwinkleError. Posted to issue by Joe Blow. Make new test pass using the 'underhand' strategy. The split above is not really necessary, but PR 10245 squashed changes to 52 files of 15 file types into one initial commit.
https://github.com/python/cpython/pull/10245/commits/17ba155a7b794926ce705ee...
View Files displays them alphabetically. Jump to ... lists them in the same order, but includes the functions changed, so it is hundreds of lines.
I think this PR would have benefited from having perhaps 10 or more initial commits, each with a comment about the group of files included. In icon commit could have said something about the source and purpose of the added icon files. One commit could have included the venv and test changes that you want to review. An added message, as long as needed, could have explained these particular changes.
This is great in theory, but if you look back at the original PR it would have been 100+ commits (I was occasionally squashing and force pushing). What you're really proposing is: * do all the work using the git workflow (stream-of-consiousness commits) * squash all the commits at the end * re-expand the single commit into logical groupings of files and re-commit them So it's easy to sit back and imagine that I had all the perfect changes ready to go and deliberately chose to make it harder to review, but that's not the case at all. Making it sound like this is how development works is really disparaging to people who find themselves not producing perfect commit histories. And let's be honest, there's no good tooling for turning a series of interdependent commits into a smaller set of sensible ones. Squashing at least gets rid of the changes that were reverted as part of the entire PR, and if you then just want to split by file you're really just asking for extra work. If someone had bothered to say "I'll review the parts of it that are relevant to me if you can split them out" then I would have, but nobody (in public) showed any interest in reviewing the changes at all - I had some private reviews done by colleagues at work, who weren't so demanding about it. I review as many PRs as I send these days (maybe more? I'm not counting), and I always try to make an effort to have mercy towards people to save them having to work extra hard just to make my life a little bit easier. It grates to have had an incremental change visible in public for over two months, have it be totally ignored the entire time, and then have to defend something that I already did. Luckily I get paid work time for doing this; really can't see why I'd want to suffer through this for free...
On Sat, 8 Dec 2018 at 09:10, Steve Dower
And let's be honest, there's no good tooling for turning a series of interdependent commits into a smaller set of sensible ones. Squashing at least gets rid of the changes that were reverted as part of the entire PR, and if you then just want to split by file you're really just asking for extra work.
Whether the UX counts as "good" or not is open to debate (I consider it pretty good for the complexity of the task it handles), but if you ever want to revise the history of a complex patch series to make it easier for reviewers to follow: 1. Use "git rebase --interactive" to squash all the ad hoc commits into a single commit 2. Use "git reset HEAD^" to unstage that squashed monolithic commit 3. Use "git add --patch" to compose a new commit series that takes a reviewer through a logical set of changes, rather than the messy reality of what actually happened (I have no idea if there are any GUI tools which expose this level of commit series editing power, but it exists on the command line, so presumably there are graphical equivalents) Most of the time this isn't worth the hassle, with either reviewing the PR as a whole being good enough, or else there being subsets of the changes which can be made into separate PRs that can be reviewed and accepted independently. However, on those occassions where you're needing to tell the reviewers a story not only about what you changed, but also why you changed it, then an hour or two spent up front creating a more coherent commit history may save multiple hours of discussion later. Cheers, Nick. P.S. Gerrit's entire review model is based around this idea of dependent patch series, but code review UX turned out to be a situation where optimising for the simple, common case (i.e. GitHub's approach) proved to be a much better idea than focusing on the less common complex cases. -- Nick Coghlan | ncoghlan@gmail.com | Brisbane, Australia
On 12/8/2018 11:32 AM, Nick Coghlan wrote:
Whether the UX counts as "good" or not is open to debate (I consider it pretty good for the complexity of the task it handles), but if you ever want to revise the history of a complex patch series to make it easier for reviewers to follow:
1. Use "git rebase --interactive" to squash all the ad hoc commits into a single commit 2. Use "git reset HEAD^" to unstage that squashed monolithic commit 3. Use "git add --patch" to compose a new commit series that takes a reviewer through a logical set of changes, rather than the messy reality of what actually happened
Thank you for the information. I am sure I will use it.
(I have no idea if there are any GUI tools which expose this level of commit series editing power, but it exists on the command line, so presumably there are graphical equivalents)
On Windows, I use git gui routinely for making commits. It lists files with workspace changes in Unstaged and Staged boxes. ^T (sTage) and ^U (Unstage) moves a hightlighted file to the other one. A third box shows the diff for a highlighted file. One can also revise the 'last' commit (never done this yet). A fourth box is for entering and editing commit messages. -- Terry Jan Reedy
participants (6)
-
Jeremy Kloth
-
Nick Coghlan
-
Steve Dower
-
Terry Reedy
-
Victor Stinner
-
Zachary Ware