| Message ID | f046d7d4-7dd4-28f2-10b9-6d266e3c0896@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
| Series | contrib/gcc-changelog: Check whether revert-commit exists | expand |
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
Tobias Burnus <tobias@codesourcery.com> 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.
Hi! On Tue, 5 Sept 2023 at 16:38, Tobias Burnus <tobias@codesourcery.com> 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 >
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 <tobias@codesourcery.com> > 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
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
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 <tobias@codesourcery.com> 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 <christophe.lyon@linaro.org> 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
On Thu, 7 Sept 2023 at 11:45, Jakub Jelinek <jakub@redhat.com> 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 <tobias@codesourcery.com> > 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 <christophe.lyon@linaro.org> > > 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 <christophe.lyon@linaro.org> Date: Fri Sep 8 08:13:32 2023 +0000 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 <christophe.lyon@linaro.org> + + Revert + 2023-09-04 Christophe Lyon <christophe.lyon@linaro.org> + + PR libstdc++/111238 + * configure: Regenerate. + * configure.ac: Call GLIBCXX_CHECK_LINKER_FEATURES in cross, + non-Canadian builds. + 2023-09-04 Christophe Lyon <christophe.lyon@linaro.org> 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 > >
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 <christophe.lyon@linaro.org> > + > + Revert > + 2023-09-04 Christophe Lyon <christophe.lyon@linaro.org> > + > + PR libstdc++/111238 > + * configure: Regenerate. > + * configure.ac: Call GLIBCXX_CHECK_LINKER_FEATURES in cross, > + non-Canadian builds. > + > 2023-09-04 Christophe Lyon <christophe.lyon@linaro.org> > > 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
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..4d024026f2b 100755 --- a/contrib/gcc-changelog/git_commit.py +++ b/contrib/gcc-changelog/git_commit.py @@ -329,11 +329,13 @@ 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: + 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 +798,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():