Hi everyone,
Here is my backport of the DEBUG macro refactoring patchset to sssd-1-11 branch.
Patches are mostly identical to master, with the exception of the changes generated by scripts (which were re-run), some commit messages and one commit being moved into a more logical place in history.
All patches that should work passed "make check" both on RHEL7 and Fedora 20 (verified by CI).
I've left the original Reviewed-by tags for the commits with unchanged content, but I can remove them, if necessary.
Sincerely, Nick
On (30/04/14 19:14), Nikolai Kondrashov wrote:
Hi everyone,
Here is my backport of the DEBUG macro refactoring patchset to sssd-1-11 branch.
Patches are mostly identical to master, with the exception of the changes generated by scripts (which were re-run), some commit messages and one commit being moved into a more logical place in history.
All patches that should work passed "make check" both on RHEL7 and Fedora 20 (verified by CI).
I've left the original Reviewed-by tags for the commits with unchanged content, but I can remove them, if necessary.
Sincerely, Nick
I realised that sssd cannot be built after applying the 5th and the 6th patch. 0005-Make-DEBUG-macro-definition-variadic.patch 0006-Make-DEBUG-macro-invocations-variadic.patch It is fixed by the 7th patch. 0007-Fixup-DEBUG-macro-invocations-update.patch
Now, it does not worth to modify patches, because the same problem is in master. I should have reviewed them. Just for the record. It is better to modify patches even if they are generated by script. It causes problem with bisecting. Sometimes problem can be solved easily. In my opinion, we should be able to compile each commit.
Jakub, please push these patches to 1-11 very soon. We do not want to rebase them like patchset for master.
ACK
LS
On 04/30/2014 11:02 PM, Lukas Slebodnik wrote:
I realised that sssd cannot be built after applying the 5th and the 6th patch. 0005-Make-DEBUG-macro-definition-variadic.patch 0006-Make-DEBUG-macro-invocations-variadic.patch It is fixed by the 7th patch. 0007-Fixup-DEBUG-macro-invocations-update.patch
Now, it does not worth to modify patches, because the same problem is in master. I should have reviewed them. Just for the record. It is better to modify patches even if they are generated by script. It causes problem with bisecting. Sometimes problem can be solved easily. In my opinion, we should be able to compile each commit.
I understand the benefit of "every patch builds" approach and would support a policy requiring that. Originally, I made them separate to simplify review and left the squashing decision to maintainers. Here, I simply cherry-picked them the way they were merged to (again) make it simpler.
Sincerely, Nick
On 04/30/2014 11:39 PM, Nikolai Kondrashov wrote:
On 04/30/2014 11:02 PM, Lukas Slebodnik wrote:
I realised that sssd cannot be built after applying the 5th and the 6th patch. 0005-Make-DEBUG-macro-definition-variadic.patch 0006-Make-DEBUG-macro-invocations-variadic.patch It is fixed by the 7th patch. 0007-Fixup-DEBUG-macro-invocations-update.patch
Now, it does not worth to modify patches, because the same problem is in master. I should have reviewed them. Just for the record. It is better to modify patches even if they are generated by script. It causes problem with bisecting. Sometimes problem can be solved easily. In my opinion, we should be able to compile each commit.
I understand the benefit of "every patch builds" approach and would support a policy requiring that. Originally, I made them separate to simplify review and left the squashing decision to maintainers. Here, I simply cherry-picked them the way they were merged to (again) make it simpler.
One thing, though: having the script-generated patches separate certainly made cherry-picking easier and faster.
Nick.
On (01/05/14 00:51), Nikolai Kondrashov wrote:
On 04/30/2014 11:39 PM, Nikolai Kondrashov wrote:
On 04/30/2014 11:02 PM, Lukas Slebodnik wrote:
I realised that sssd cannot be built after applying the 5th and the 6th patch. 0005-Make-DEBUG-macro-definition-variadic.patch 0006-Make-DEBUG-macro-invocations-variadic.patch It is fixed by the 7th patch. 0007-Fixup-DEBUG-macro-invocations-update.patch
Now, it does not worth to modify patches, because the same problem is in master. I should have reviewed them. Just for the record. It is better to modify patches even if they are generated by script. It causes problem with bisecting. Sometimes problem can be solved easily. In my opinion, we should be able to compile each commit.
I understand the benefit of "every patch builds" approach and would support a policy requiring that. Originally, I made them separate to simplify review and left the squashing decision to maintainers. Here, I simply cherry-picked them the way they were merged to (again) make it simpler.
One thing, though: having the script-generated patches separate certainly made cherry-picking easier and faster.
The patches were pushed, so we can continue with this discussion.
The only way how to make chery-picking easier is to have better merging algorithm in version control syste. The script generated patches solve problem with back porting changes to diverged branches.
I am not against script generated patches. I like scripting, it simplifies my life. My idea was to have script which will fix(convert) 99% of cases and the last 1% of corner cases can be solved easily by hand. Sometimes it is easier to fix the last 1% of problems by hand rather than spending days with creating a robust conversion script. But the main point is to include hand made changes to the generated patch and not to the separate patch. With this approach work is simplified with script and we don't lost benefit "every patch builds"
LS
On 05/05/2014 10:36 PM, Lukas Slebodnik wrote:
On (01/05/14 00:51), Nikolai Kondrashov wrote:
One thing, though: having the script-generated patches separate certainly made cherry-picking easier and faster.
The patches were pushed, so we can continue with this discussion.
The only way how to make chery-picking easier is to have better merging algorithm in version control syste. The script generated patches solve problem with back porting changes to diverged branches.
I am not against script generated patches. I like scripting, it simplifies my life. My idea was to have script which will fix(convert) 99% of cases and the last 1% of corner cases can be solved easily by hand. Sometimes it is easier to fix the last 1% of problems by hand rather than spending days with creating a robust conversion script. But the main point is to include hand made changes to the generated patch and not to the separate patch. With this approach work is simplified with script and we don't lost benefit "every patch builds"
The reason why I would sacrifice the "every patch builds" rule, originally, was to simplify backporting to older branches, which would likely be different and thus simply cherry-picking the monolithic (scripted+manual changes) commit would not be possible to do with confidence, and re-running the script would be necessary (which I did with this backport).
In that case, my idea was, keeping manual changes separate would allow us to simply cherry-pick them, without having to extract them from the monolithic commit by hand or to do them again.
However, now I realize that we could have simply ran the script on the commit before the original and made a diff to extract manual changes, which we could have then applied on the older tree, after running the script there.
So, yes, it is possible to have "every patch builds" and also to backport scripted+manual patches reliably. The script in question would better be included with such commits, though.
Sincerely, Nick
On (09/05/14 15:09), Nikolai Kondrashov wrote:
On 05/05/2014 10:36 PM, Lukas Slebodnik wrote:
On (01/05/14 00:51), Nikolai Kondrashov wrote:
One thing, though: having the script-generated patches separate certainly made cherry-picking easier and faster.
The patches were pushed, so we can continue with this discussion.
The only way how to make chery-picking easier is to have better merging algorithm in version control syste. The script generated patches solve problem with back porting changes to diverged branches.
I am not against script generated patches. I like scripting, it simplifies my life. My idea was to have script which will fix(convert) 99% of cases and the last 1% of corner cases can be solved easily by hand. Sometimes it is easier to fix the last 1% of problems by hand rather than spending days with creating a robust conversion script. But the main point is to include hand made changes to the generated patch and not to the separate patch. With this approach work is simplified with script and we don't lost benefit "every patch builds"
The reason why I would sacrifice the "every patch builds" rule, originally, was to simplify backporting to older branches, which would likely be different and thus simply cherry-picking the monolithic (scripted+manual changes) commit would not be possible to do with confidence, and re-running the script would be necessary (which I did with this backport).
In that case, my idea was, keeping manual changes separate would allow us to simply cherry-pick them, without having to extract them from the monolithic commit by hand or to do them again.
It was very bad assumption that patch which changed 215 files ( 7825 insertions, 7825 deletions) would be possible to cherry-pick to older branch without any problem. This is a reason why script was written.
However, now I realize that we could have simply ran the script on the commit before the original and made a diff to extract manual changes, which we could have then applied on the older tree, after running the script there.
Backporting much simpler patches can be difficult. I already wrote in previous mail how it should be done.
So, yes, it is possible to have "every patch builds" and also to backport scripted+manual patches reliably. The script in question would better be included with such commits, though.
The script was included in commit message. At least, you learnt something new and will not do the same mistake in future.
LS
On 05/09/2014 10:00 PM, Lukas Slebodnik wrote:
On (09/05/14 15:09), Nikolai Kondrashov wrote:
On 05/05/2014 10:36 PM, Lukas Slebodnik wrote:
On (01/05/14 00:51), Nikolai Kondrashov wrote:
One thing, though: having the script-generated patches separate certainly made cherry-picking easier and faster.
The patches were pushed, so we can continue with this discussion.
The only way how to make chery-picking easier is to have better merging algorithm in version control syste. The script generated patches solve problem with back porting changes to diverged branches.
I am not against script generated patches. I like scripting, it simplifies my life. My idea was to have script which will fix(convert) 99% of cases and the last 1% of corner cases can be solved easily by hand. Sometimes it is easier to fix the last 1% of problems by hand rather than spending days with creating a robust conversion script. But the main point is to include hand made changes to the generated patch and not to the separate patch. With this approach work is simplified with script and we don't lost benefit "every patch builds"
The reason why I would sacrifice the "every patch builds" rule, originally, was to simplify backporting to older branches, which would likely be different and thus simply cherry-picking the monolithic (scripted+manual changes) commit would not be possible to do with confidence, and re-running the script would be necessary (which I did with this backport).
In that case, my idea was, keeping manual changes separate would allow us to simply cherry-pick them, without having to extract them from the monolithic commit by hand or to do them again.
It was very bad assumption that patch which changed 215 files ( 7825 insertions, 7825 deletions) would be possible to cherry-pick to older branch without any problem. This is a reason why script was written.
Sure. There is some misunderstanding going on, apparently. I didn't make the assumption that we can just cherry-pick a huge script-generated commit, and it was me who wrote that script, partly to avoid that, before I joined the team :)
I'm just saying that, of course, I would have to re-run the script on the older branch and then I would have to fix up whatever the script missed.
However, my original assumption was that if we have a monolithic commit with scripted+manual changes, it wouldn't be easy to extract the fixup part from it and I would have to redo it by hand on the older branch.
As I say in the previous message, I now realize extracting the fixup part would be possible, provided the script works the same way.
So, yes, it is possible to have "every patch builds" and also to backport scripted+manual patches reliably. The script in question would better be included with such commits, though.
The script was included in commit message.
Sure, I put it there. I merely state a requirement for such system to work.
At least, you learnt something new and will not do the same mistake in future.
I wouldn't say it was learning, rather realizing there is a way. Once again, I support "every commit builds" idea. I'm implementing CI to make that achievable, actually.
Neither it was my mistake much. I urged the maintainers to squash commits as necessary and supported squashing manual changes into the scripted ones explicitly. They didn't, probably to speed up the push and I decided to go along the same way when backporting to make things simpler to track and review.
I should probably stop defending myself now :)
Sincerely, Nick
On (09/05/14 22:21), Nikolai Kondrashov wrote:
On 05/09/2014 10:00 PM, Lukas Slebodnik wrote:
On (09/05/14 15:09), Nikolai Kondrashov wrote:
On 05/05/2014 10:36 PM, Lukas Slebodnik wrote:
On (01/05/14 00:51), Nikolai Kondrashov wrote:
One thing, though: having the script-generated patches separate certainly made cherry-picking easier and faster.
The patches were pushed, so we can continue with this discussion.
The only way how to make chery-picking easier is to have better merging algorithm in version control syste. The script generated patches solve problem with back porting changes to diverged branches.
I am not against script generated patches. I like scripting, it simplifies my life. My idea was to have script which will fix(convert) 99% of cases and the last 1% of corner cases can be solved easily by hand. Sometimes it is easier to fix the last 1% of problems by hand rather than spending days with creating a robust conversion script. But the main point is to include hand made changes to the generated patch and not to the separate patch. With this approach work is simplified with script and we don't lost benefit "every patch builds"
The reason why I would sacrifice the "every patch builds" rule, originally, was to simplify backporting to older branches, which would likely be different and thus simply cherry-picking the monolithic (scripted+manual changes) commit would not be possible to do with confidence, and re-running the script would be necessary (which I did with this backport).
In that case, my idea was, keeping manual changes separate would allow us to simply cherry-pick them, without having to extract them from the monolithic commit by hand or to do them again.
It was very bad assumption that patch which changed 215 files ( 7825 insertions, 7825 deletions) would be possible to cherry-pick to older branch without any problem. This is a reason why script was written.
Sure. There is some misunderstanding going on, apparently. I didn't make the assumption that we can just cherry-pick a huge script-generated commit, and it was me who wrote that script, partly to avoid that, before I joined the team :)
It is not about team, It is about open source and contribution. You can do the same mistake in another project.
I'm just saying that, of course, I would have to re-run the script on the older branch and then I would have to fix up whatever the script missed.
However, my original assumption was that if we have a monolithic commit with scripted+manual changes, it wouldn't be easy to extract the fixup part from it and I would have to redo it by hand on the older branch.
As I already wrote, It was bad assumption. The script cabe perfect and convert 100% of messages on one branch, but there can be problems with older branches. Older branches are(can be) very diverged. TThe fixup part needn't be available in older branch and script can cause problem with totaly different debug messages.
As I say in the previous message, I now realize extracting the fixup part would be possible, provided the script works the same way.
So, yes, it is possible to have "every patch builds" and also to backport scripted+manual patches reliably. The script in question would better be included with such commits, though.
The script was included in commit message.
Sure, I put it there. I merely state a requirement for such system to work.
At least, you learnt something new and will not do the same mistake in future.
I wouldn't say it was learning, rather realizing there is a way. Once again, I support "every commit builds" idea. I'm implementing CI to make that achievable, actually.
Neither it was my mistake much. I urged the maintainers to squash commits as necessary and supported squashing manual changes into the scripted ones explicitly.
I cannot see a reason why they should be squased explicitly and before pushing. You should have done it before sending patches. We aren't used to modifying patches before pushing.
They didn't, probably to speed up the push and I decided to go along the same way when backporting to make things simpler to track and review.
I should probably stop defending myself now :)
Agree.
LS
On Fri, May 09, 2014 at 09:00:28PM +0200, Lukas Slebodnik wrote:
On (09/05/14 15:09), Nikolai Kondrashov wrote:
On 05/05/2014 10:36 PM, Lukas Slebodnik wrote:
On (01/05/14 00:51), Nikolai Kondrashov wrote:
One thing, though: having the script-generated patches separate certainly made cherry-picking easier and faster.
The patches were pushed, so we can continue with this discussion.
The only way how to make chery-picking easier is to have better merging algorithm in version control syste. The script generated patches solve problem with back porting changes to diverged branches.
I am not against script generated patches. I like scripting, it simplifies my life. My idea was to have script which will fix(convert) 99% of cases and the last 1% of corner cases can be solved easily by hand. Sometimes it is easier to fix the last 1% of problems by hand rather than spending days with creating a robust conversion script. But the main point is to include hand made changes to the generated patch and not to the separate patch. With this approach work is simplified with script and we don't lost benefit "every patch builds"
The reason why I would sacrifice the "every patch builds" rule, originally, was to simplify backporting to older branches, which would likely be different and thus simply cherry-picking the monolithic (scripted+manual changes) commit would not be possible to do with confidence, and re-running the script would be necessary (which I did with this backport).
In that case, my idea was, keeping manual changes separate would allow us to simply cherry-pick them, without having to extract them from the monolithic commit by hand or to do them again.
It was very bad assumption that patch which changed 215 files ( 7825 insertions, 7825 deletions) would be possible to cherry-pick to older branch without any problem. This is a reason why script was written.
However, now I realize that we could have simply ran the script on the commit before the original and made a diff to extract manual changes, which we could have then applied on the older tree, after running the script there.
Backporting much simpler patches can be difficult. I already wrote in previous mail how it should be done.
So, yes, it is possible to have "every patch builds" and also to backport scripted+manual patches reliably. The script in question would better be included with such commits, though.
The script was included in commit message. At least, you learnt something new and will not do the same mistake in future.
Please let's stay technical and considerate to one another. This discussion has took the route of bikeshedding already.
On the topic of every patch builds, yes, I prefer that too and I should have reviewed this aspect of the patchset. But it's not worth bringing up on the devel list in more context than something to keep in mind for next time.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On 05/12/2014 06:09 AM, Jakub Hrozek wrote:
On Fri, May 09, 2014 at 09:00:28PM +0200, Lukas Slebodnik wrote:
On (09/05/14 15:09), Nikolai Kondrashov wrote:
On 05/05/2014 10:36 PM, Lukas Slebodnik wrote:
On (01/05/14 00:51), Nikolai Kondrashov wrote:
One thing, though: having the script-generated patches separate certainly made cherry-picking easier and faster.
The patches were pushed, so we can continue with this discussion.
The only way how to make chery-picking easier is to have better merging algorithm in version control syste. The script generated patches solve problem with back porting changes to diverged branches.
I am not against script generated patches. I like scripting, it simplifies my life. My idea was to have script which will fix(convert) 99% of cases and the last 1% of corner cases can be solved easily by hand. Sometimes it is easier to fix the last 1% of problems by hand rather than spending days with creating a robust conversion script. But the main point is to include hand made changes to the generated patch and not to the separate patch. With this approach work is simplified with script and we don't lost benefit "every patch builds"
The reason why I would sacrifice the "every patch builds" rule, originally, was to simplify backporting to older branches, which would likely be different and thus simply cherry-picking the monolithic (scripted+manual changes) commit would not be possible to do with confidence, and re-running the script would be necessary (which I did with this backport).
In that case, my idea was, keeping manual changes separate would allow us to simply cherry-pick them, without having to extract them from the monolithic commit by hand or to do them again.
It was very bad assumption that patch which changed 215 files ( 7825 insertions, 7825 deletions) would be possible to cherry-pick to older branch without any problem. This is a reason why script was written.
However, now I realize that we could have simply ran the script on the commit before the original and made a diff to extract manual changes, which we could have then applied on the older tree, after running the script there.
Backporting much simpler patches can be difficult. I already wrote in previous mail how it should be done.
So, yes, it is possible to have "every patch builds" and also to backport scripted+manual patches reliably. The script in question would better be included with such commits, though.
The script was included in commit message. At least, you learnt something new and will not do the same mistake in future.
Please let's stay technical and considerate to one another. This discussion has took the route of bikeshedding already.
On the topic of every patch builds, yes, I prefer that too and I should have reviewed this aspect of the patchset. But it's not worth bringing up on the devel list in more context than something to keep in mind for next time.
Just to chime in here as well, we have never had a *strict* requirement that every patch builds individually. We strongly prefer it (for backporting purposes), but there are plenty of occasions where we have opted for (This patch-*set* compiles together, but we have broken the patches up along sensible lines to make review possible).
I personally think this patch-set is fully in keeping with that philosophy (that ability to review sanely trumps technical purity). As long as it's noted in the review request that "patches A, B and C must always be applied together to work properly".
On Wed, Apr 30, 2014 at 07:14:51PM +0300, Nikolai Kondrashov wrote:
Hi everyone,
Here is my backport of the DEBUG macro refactoring patchset to sssd-1-11 branch.
Patches are mostly identical to master, with the exception of the changes generated by scripts (which were re-run), some commit messages and one commit being moved into a more logical place in history.
All patches that should work passed "make check" both on RHEL7 and Fedora 20 (verified by CI).
I've left the original Reviewed-by tags for the commits with unchanged content, but I can remove them, if necessary.
Sincerely, Nick
Thanks for the quick turnaround, I pushed them all to sssd-1-11. The commits were: 4cb9844e898e8c1ddeaf1f8bb13916639d78df20 a59e6f8ee3a57f1161ccdf4d887db2ab69476f13 6e40f8ebc04dbd494abedaa6f14253da1e1cd31e 46c28ba3044c2121c2c0af4493ac655dcb41e0e6 dad68b3284228a38b4beb06946701e8795980966 b0d759c9a604add0573c04b1a4e88e1605e1fdc0 b9afd88065ecdb9c2cafb2ee767bd61585b31b8f 38e51a251fe4f76fd26f6f50948d57487bc5988d 7f3b9e09ff4664ef86b44ef0f89f40bbcaecfbce dab43bc463a25109f6d86d64722f5e9c60d465e5 0a3246795b7ec49d6a2de4bebd82810b9b0cd08e
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Lukas Slebodnik -
Nikolai Kondrashov -
Stephen Gallagher