Re: [Patch] contrib/gcc-changelog: Check whether revert-commit exists
On Fri, Sep 08, 2023 at 10:25:42AM +0200, Christophe Lyon wrote: > Revert "libstdc++: Use GLIBCXX_CHECK_LINKER_FEATURES for cross-builds > (PR111238)" > > diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog > index 8f7b01e0563..0c60149d7f6 100644 > --- a/libstdc++-v3/ChangeLog > +++ b/libstdc++-v3/ChangeLog > @@ -4,6 +4,16 @@ > for freestanding. > * configure: Regenerate. > > +2023-09-04 Christophe Lyon > + > + Revert > + 2023-09-04 Christophe Lyon > + > + PR libstdc++/111238 > + * configure: Regenerate. > + * configure.ac: Call GLIBCXX_CHECK_LINKER_FEATURES in cross, > + non-Canadian builds. > + > 2023-09-04 Christophe Lyon > > PR libstdc++/111238 > > I inserted my "Revert" ChangeLog entry between the entry I want to declare > reverted and Jonathan's more recent patch about GLIBCXX_ENABLE_BACKTRACE. > Is that OK for the commit hooks? Yes, thanks. Jakub
Re: [Patch] contrib/gcc-changelog: Check whether revert-commit exists
On Thu, 7 Sept 2023 at 11:45, Jakub Jelinek wrote: > On Thu, Sep 07, 2023 at 10:20:19AM +0200, Christophe Lyon via Gcc-patches > wrote: > > On Tue, 5 Sept 2023 at 16:38, Tobias Burnus > wrote: > > > > > That's based on the fail > > > https://gcc.gnu.org/pipermail/gccadmin/2023q3/020349.html > > > and on the discussion on IRC. > > > > > > > Sorry I didn't notice the problem, nor the discussion on IRC, but I can > see > > that my commits created the problem, sorry for that. > > > > I'm not sure how your patch would have prevented me from doing this? > > What happened is that I had 3 patches on top of master > > - HEAD: the one I wanted to push > > - HEAD-1: revert of HEAD-2 > > git reset HEAD^ > instead of committing a revert would be better I think. > The patch I reverted locally was still under discussion, so I wanted to keep it around until we made a decision. But yeah. > > - HEAD-2: libstdc-Use-GLIBCXX_CHECK_LINKER_FEATURES-for.patch > > > > I had actually forgotten about HEAD-1 and HEAD-2, HEAD was unrelated to > > those, so when I pushed, I pushed 3 commits while I thought there was > only > > one. > > I did run contrib/gcc-changelog/git_check_commit.py (I realize I'm not > sure > > whether I used git_check_commit.py or git_commit.py), but only on HEAD > > since I had forgotten about the other two. > > Could you please remove your > 2023-09-04 Christophe Lyon > > PR libstdc++/111238 > * configure: Regenerate. > * configure.ac: Call GLIBCXX_CHECK_LINKER_FEATURES in cross, > non-Canadian builds. > libstdc++-v3/ChangeLog entry if that commit is indeed not in (or add > a Revert: entry for it right after it if you think it needs to be in)? > That is a part I haven't done, my/Arsen's hacks to make the version update > get through basically ignored that revert commit. > > ChangeLog files can be changed by commits which only touch ChangeLog files > and nothing else (ok, date stamp too, but please don't update that), no > ChangeLog in the message needs to be provided for such changes. > Like so: commit d2bb261dbf282bbb258e1e5f17c1b6230327e076 (HEAD -> master) Author: Christophe Lyon Date: Fri Sep 8 08:13:32 2023 + Revert "libstdc++: Use GLIBCXX_CHECK_LINKER_FEATURES for cross-builds (PR111238)" diff --git a/libstdc++-v3/ChangeLog b/libstdc++-v3/ChangeLog index 8f7b01e0563..0c60149d7f6 100644 --- a/libstdc++-v3/ChangeLog +++ b/libstdc++-v3/ChangeLog @@ -4,6 +4,16 @@ for freestanding. * configure: Regenerate. +2023-09-04 Christophe Lyon + + Revert + 2023-09-04 Christophe Lyon + + PR libstdc++/111238 + * configure: Regenerate. + * configure.ac: Call GLIBCXX_CHECK_LINKER_FEATURES in cross, + non-Canadian builds. + 2023-09-04 Christophe Lyon PR libstdc++/111238 I inserted my "Revert" ChangeLog entry between the entry I want to declare reverted and Jonathan's more recent patch about GLIBCXX_ENABLE_BACKTRACE. Is that OK for the commit hooks? Jakub > >
Re: [Patch] contrib/gcc-changelog: Check whether revert-commit exists
On Thu, Sep 07, 2023 at 10:20:19AM +0200, Christophe Lyon via Gcc-patches wrote: > On Tue, 5 Sept 2023 at 16:38, Tobias Burnus wrote: > > > That's based on the fail > > https://gcc.gnu.org/pipermail/gccadmin/2023q3/020349.html > > and on the discussion on IRC. > > > > Sorry I didn't notice the problem, nor the discussion on IRC, but I can see > that my commits created the problem, sorry for that. > > I'm not sure how your patch would have prevented me from doing this? > What happened is that I had 3 patches on top of master > - HEAD: the one I wanted to push > - HEAD-1: revert of HEAD-2 git reset HEAD^ instead of committing a revert would be better I think. > - HEAD-2: libstdc-Use-GLIBCXX_CHECK_LINKER_FEATURES-for.patch > > I had actually forgotten about HEAD-1 and HEAD-2, HEAD was unrelated to > those, so when I pushed, I pushed 3 commits while I thought there was only > one. > I did run contrib/gcc-changelog/git_check_commit.py (I realize I'm not sure > whether I used git_check_commit.py or git_commit.py), but only on HEAD > since I had forgotten about the other two. Could you please remove your 2023-09-04 Christophe Lyon PR libstdc++/111238 * configure: Regenerate. * configure.ac: Call GLIBCXX_CHECK_LINKER_FEATURES in cross, non-Canadian builds. libstdc++-v3/ChangeLog entry if that commit is indeed not in (or add a Revert: entry for it right after it if you think it needs to be in)? That is a part I haven't done, my/Arsen's hacks to make the version update get through basically ignored that revert commit. ChangeLog files can be changed by commits which only touch ChangeLog files and nothing else (ok, date stamp too, but please don't update that), no ChangeLog in the message needs to be provided for such changes. Jakub
Re: [Patch] contrib/gcc-changelog: Check whether revert-commit exists
On Thu, Sep 07, 2023 at 11:30:53AM +0200, Tobias Burnus wrote:
> contrib/gcc-changelog: Check whether revert-commit exists
>
> contrib/ChangeLog:
>
> * gcc-changelog/git_commit.py (GitCommit.__init__):
> Handle commit_to_info_hook = None; otherwise, if None,
> regard it as error.
> (to_changelog_entries): Handle commit_to_info_hook = None;
> if info is None, create a warning for it.
> * gcc-changelog/git_email.py (GitEmail.__init__):
> call super() with commit_to_info_hook=None instead
> of a lamda function.
>
> contrib/gcc-changelog/git_commit.py | 20 +++-
> contrib/gcc-changelog/git_email.py | 3 +--
> 2 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/contrib/gcc-changelog/git_commit.py
> b/contrib/gcc-changelog/git_commit.py
> index 4f3131021f2..4f1bd4d7293 100755
> --- a/contrib/gcc-changelog/git_commit.py
> +++ b/contrib/gcc-changelog/git_commit.py
> @@ -329,11 +329,15 @@ class GitCommit:
> self.revert_commit = m.group('hash')
> break
> if self.revert_commit:
> +# The following happens for get_email.py:
> +if not self.commit_to_info_hook:
> +self.warnings.append(f"Invoked script can technically not
> obtain info about "
> + f"reverted commits such as
> '{self.revert_commit}'")
I think not should precede technically (or should we just drop technically)?
> +self.warnings.append(f"Invoked script can
> technically not obtain info about "
> + f"cherry-picked commits such as
> '{self.revert_commit}'")
Likewise.
> timestamp = current_timestamp
> elif not timestamp or use_commit_ts:
> timestamp = current_timestamp
> diff --git a/contrib/gcc-changelog/git_email.py
> b/contrib/gcc-changelog/git_email.py
> index 49f41f2ec99..93808dfabb6 100755
> --- a/contrib/gcc-changelog/git_email.py
> +++ b/contrib/gcc-changelog/git_email.py
> @@ -89,8 +89,7 @@ class GitEmail(GitCommit):
> t = 'M'
> modified_files.append((target if t != 'D' else source, t))
> git_info = GitInfo(None, date, author, message, modified_files)
> -super().__init__(git_info,
> - commit_to_info_hook=lambda x: None)
> +super().__init__(git_info, commit_to_info_hook=None)
>
>
> def show_help():
Otherwise LGTM, but it would be good after committing it try to commit
reversion commit of some non-existent hash (willing to handle ChangeLog
manually again if it makes it through).
Jakub
Re: [Patch] contrib/gcc-changelog: Check whether revert-commit exists
First: Hi all,
attached is an updated patch (v3) where I changed the wording
of the warning to distinguish technical reasons for not using
the commit data (→ git_email.py) from not-found lookups (git_check_commit.py)
to avoid confusion. (See also below.)
Any remarks/suggestions - related to this (e.g. wording improvements or ...)
or to the patch in general?
Hi Christoph,
On 07.09.23 10:20, Christophe Lyon wrote:
On Tue, 5 Sept 2023 at 16:38, Tobias Burnus
wrote:
That's based on the fail
https://gcc.gnu.org/pipermail/gccadmin/2023q3/020349.html
and on the discussion on IRC.
Sorry I didn't notice the problem, nor the discussion on IRC, but I
can see that my commits created the problem, sorry for that.
Well, as the saying goes: you can't make an omelette without breaking eggs.
And there were three issues: The invalid reference, the missing check
for the former, and that it caused the nightly script to break.
While we cannot prevent human errors (nor KI ones) and have fixed/worked
around the latter, we can improve on the checking part :-)
I'm not sure how your patch would have prevented me from doing this?
There are two checking scripts:
One for manual running on a file produced by 'git format-patch', which is
./contrib/gcc-changelog/git_email.py
Before it crashed for the revert, now it prints OK with the warning:
Cannot obtain info about reverted commit
'46c2e94ca66ed9991c45a6ba6204ed02869efc39'
re-reading it, I think the wording is wrong. It should be:
Invoked script cannot obtain info about reverted commits such as
'46c2e94ca66ed9991c45a6ba6204ed02869efc39'
as that's a generic limitation - we simply do not know whether
that commit exists or not. (Changed in the attached patch.)
And the other operates on the git repository:
./contrib/gcc-changelog/git_check_commit.py
the latter now fails with:
ERR: Cannot find to-be-reverted commit:
"46c2e94ca66ed9991c45a6ba6204ed02869efc39"
That script can be run manually on any commit (e.g. before the commit;
consider using the '-v' and/or '-p' options; see '--help').
But that script is also run on the GCC server as pre-commit hook.
Therefore, the latter would have rejected your commit with the error message
during "git push", permitting to fix the commit (git commit --amend) before
trying again, similar to missing '\t' in the changelog or ...
Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
contrib/gcc-changelog: Check whether revert-commit exists
contrib/ChangeLog:
* gcc-changelog/git_commit.py (GitCommit.__init__):
Handle commit_to_info_hook = None; otherwise, if None,
regard it as error.
(to_changelog_entries): Handle commit_to_info_hook = None;
if info is None, create a warning for it.
* gcc-changelog/git_email.py (GitEmail.__init__):
call super() with commit_to_info_hook=None instead
of a lamda function.
contrib/gcc-changelog/git_commit.py | 20 +++-
contrib/gcc-changelog/git_email.py | 3 +--
2 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/contrib/gcc-changelog/git_commit.py b/contrib/gcc-changelog/git_commit.py
index 4f3131021f2..4f1bd4d7293 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -329,11 +329,15 @@ class GitCommit:
self.revert_commit = m.group('hash')
break
if self.revert_commit:
+# The following happens for get_email.py:
+if not self.commit_to_info_hook:
+self.warnings.append(f"Invoked script can technically not obtain info about "
+ f"reverted commits such as '{self.revert_commit}'")
+return
self.info = self.commit_to_info_hook(self.revert_commit)
-
-# The following happens for get_email.py:
-if not self.info:
-return
+if not self.info:
+self.errors.append(Error('Cannot find to-be-reverted commit', self.revert_commit))
+return
self.check_commit_email()
@@ -796,12 +800,18 @@ class GitCommit:
orig_date = self.original_info.date
current_timestamp = orig_date.strftime(DATE_FORMAT)
elif self.cherry_pick_commit:
-info = self.commit_to_info_hook(self.cherry_pick_commit)
+info = (self.commit_to_info_hook
+and self.commit_to_info_hook(self.cherry_pick_commit))
# it can happen that it is a cherry-pick for a different
# repository
if info:
timestamp = info.date.strftime(DATE_FORMAT)
else:
+if self.commit_to_info_hook:
+
Re: [Patch] contrib/gcc-changelog: Check whether revert-commit exists
Hi! On Tue, 5 Sept 2023 at 16:38, Tobias Burnus wrote: > That's based on the fail > https://gcc.gnu.org/pipermail/gccadmin/2023q3/020349.html > and on the discussion on IRC. > Sorry I didn't notice the problem, nor the discussion on IRC, but I can see that my commits created the problem, sorry for that. I'm not sure how your patch would have prevented me from doing this? What happened is that I had 3 patches on top of master - HEAD: the one I wanted to push - HEAD-1: revert of HEAD-2 - HEAD-2: libstdc-Use-GLIBCXX_CHECK_LINKER_FEATURES-for.patch I had actually forgotten about HEAD-1 and HEAD-2, HEAD was unrelated to those, so when I pushed, I pushed 3 commits while I thought there was only one. I did run contrib/gcc-changelog/git_check_commit.py (I realize I'm not sure whether I used git_check_commit.py or git_commit.py), but only on HEAD since I had forgotten about the other two. Are these scripts executed by the commit hooks too? (and thus they would now warn?) Thanks, Christophe > The problem in for the cron job was that > r14-3661-g084a7cf9fb2d9cb98dfbe7d91602c840ec50b002 > referenced a commit that did not exist. > > This was temporarily fixed by Jakub, but it makes sense to detect this > in the pre-commit hook. > > > With the patch, >contrib/gcc-changelog/git_email.py > 0001-Revert-libstdc-Use-GLIBCXX_CHECK_LINKER_FEATURES-for.patch > now prints: > OK > Warnings: > Cannot obtain info about reverted commit > '46c2e94ca66ed9991c45a6ba6204ed02869efc39' > > while >contrib/gcc-changelog/git_check_commit.py > 084a7cf9fb2d9cb98dfbe7d91602c840ec50b002 > now fails with: >Checking 084a7cf9fb2d9cb98dfbe7d91602c840ec50b002: FAILED >ERR: Cannot find to-be-reverted commit: > "46c2e94ca66ed9991c45a6ba6204ed02869efc39" > > (check_email.py always shows the warning, git_check_commit.py only with > '-v') > > Notes: > - I am not sure whether a sensible testcase can be added - and, hence, I > have not added one. > - I have run "pytest-3' but my python is too old and thus might miss some > checks which >newer pytest/flake8 will find. But at least it did pass here. > - I have not tested the cherry-pick + commit does not exist case, >which now creates a warning as I did not quickly find a testcase. > > Comments, remarks, suggestions, approval? > > Tobias > - > Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, > 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: > Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; > Registergericht München, HRB 106955 >
Re: [Patch] contrib/gcc-changelog: Check whether revert-commit exists
Tobias Burnus writes: > Attached an old patch. See attached patch for the current one. > > Difference is one line: the warning that is shown in the example output > below. Python-wise, the changes seem fine. Unsure if it does the right thing, though, since I'm not familiar with the full script. -- Arsen Arsenović signature.asc Description: PGP signature
Re: [Patch] contrib/gcc-changelog: Check whether revert-commit exists
Attached an old patch. See attached patch for the current one.
Difference is one line: the warning that is shown in the example output
below.
On 05.09.23 16:37, Tobias Burnus wrote:
That's based on the fail
https://gcc.gnu.org/pipermail/gccadmin/2023q3/020349.html
and on the discussion on IRC.
The problem in for the cron job was that
r14-3661-g084a7cf9fb2d9cb98dfbe7d91602c840ec50b002
referenced a commit that did not exist.
This was temporarily fixed by Jakub, but it makes sense to detect this
in the pre-commit hook.
With the patch,
contrib/gcc-changelog/git_email.py
0001-Revert-libstdc-Use-GLIBCXX_CHECK_LINKER_FEATURES-for.patch
now prints:
OK
Warnings:
Cannot obtain info about reverted commit
'46c2e94ca66ed9991c45a6ba6204ed02869efc39'
while
contrib/gcc-changelog/git_check_commit.py
084a7cf9fb2d9cb98dfbe7d91602c840ec50b002
now fails with:
Checking 084a7cf9fb2d9cb98dfbe7d91602c840ec50b002: FAILED
ERR: Cannot find to-be-reverted commit:
"46c2e94ca66ed9991c45a6ba6204ed02869efc39"
(check_email.py always shows the warning, git_check_commit.py only
with '-v')
Notes:
- I am not sure whether a sensible testcase can be added - and, hence,
I have not added one.
- I have run "pytest-3' but my python is too old and thus might miss
some checks which
newer pytest/flake8 will find. But at least it did pass here.
- I have not tested the cherry-pick + commit does not exist case,
which now creates a warning as I did not quickly find a testcase.
Comments, remarks, suggestions, approval?
Tobias
-
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634
München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas
Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht
München, HRB 106955
contrib/gcc-changelog: Check whether revert-commit exists
contrib/ChangeLog:
* gcc-changelog/git_commit.py (GitCommit.__init__):
Handle commit_to_info_hook = None; otherwise, if None,
regard it as error.
(to_changelog_entries): Handle commit_to_info_hook = None;
if info is None, create a warning for it.
* gcc-changelog/git_email.py (GitEmail.__init__):
call super() with commit_to_info_hook=None instead
of a lamda function.
contrib/gcc-changelog/git_commit.py | 14 +-
contrib/gcc-changelog/git_email.py | 3 +--
2 files changed, 10 insertions(+), 7 deletions(-)
diff --git a/contrib/gcc-changelog/git_commit.py b/contrib/gcc-changelog/git_commit.py
index 4f3131021f2..99e40c3c0d4 100755
--- a/contrib/gcc-changelog/git_commit.py
+++ b/contrib/gcc-changelog/git_commit.py
@@ -329,11 +329,14 @@ class GitCommit:
self.revert_commit = m.group('hash')
break
if self.revert_commit:
+# The following happens for get_email.py:
+if not self.commit_to_info_hook:
+self.warnings.append(f"Cannot obtain info about reverted commit '{self.revert_commit}'")
+return
self.info = self.commit_to_info_hook(self.revert_commit)
-
-# The following happens for get_email.py:
-if not self.info:
-return
+if not self.info:
+self.errors.append(Error('Cannot find to-be-reverted commit', self.revert_commit))
+return
self.check_commit_email()
@@ -796,12 +799,14 @@ class GitCommit:
orig_date = self.original_info.date
current_timestamp = orig_date.strftime(DATE_FORMAT)
elif self.cherry_pick_commit:
-info = self.commit_to_info_hook(self.cherry_pick_commit)
+info = (self.commit_to_info_hook
+and self.commit_to_info_hook(self.cherry_pick_commit))
# it can happen that it is a cherry-pick for a different
# repository
if info:
timestamp = info.date.strftime(DATE_FORMAT)
else:
+self.warnings.append(f"Cherry-picked commit not found: '{self.cherry_pick_commit}'")
timestamp = current_timestamp
elif not timestamp or use_commit_ts:
timestamp = current_timestamp
diff --git a/contrib/gcc-changelog/git_email.py b/contrib/gcc-changelog/git_email.py
index 49f41f2ec99..93808dfabb6 100755
--- a/contrib/gcc-changelog/git_email.py
+++ b/contrib/gcc-changelog/git_email.py
@@ -89,8 +89,7 @@ class GitEmail(GitCommit):
t = 'M'
modified_files.append((target if t != 'D' else source, t))
git_info = GitInfo(None, date, author, message, modified_files)
-super().__init__(git_info,
- commit_to_info_hook=lambda x: None)
+super().__init__(git_info, commit_to_info_hook=None)
def show_help():
