| Message ID | 20230923053515.535607-2-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | clang-tools support in tools | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> wrote: > > Builds in tools still use the cmd_ prefix in .cmd files, so don't > require the saved part. Name the groups in the line pattern match so Is that something that can be changed in the tools/ Makefiles? I'm fine with this change, just curious where the difference comes from precisely. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > that changing the regular expression is more robust and works with the > addition of a new match group. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- > scripts/clang-tools/gen_compile_commands.py | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py > index a84cc5737c2c..b43f9149893c 100755 > --- a/scripts/clang-tools/gen_compile_commands.py > +++ b/scripts/clang-tools/gen_compile_commands.py > @@ -19,7 +19,7 @@ _DEFAULT_OUTPUT = 'compile_commands.json' > _DEFAULT_LOG_LEVEL = 'WARNING' > > _FILENAME_PATTERN = r'^\..*\.cmd$' > -_LINE_PATTERN = r'^savedcmd_[^ ]*\.o := (.* )([^ ]*\.[cS]) *(;|$)' > +_LINE_PATTERN = r'^(saved)?cmd_[^ ]*\.o := (?P<command_prefix>.* )(?P<file_path>[^ ]*\.[cS]) *(;|$)' > _VALID_LOG_LEVELS = ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL'] > # The tools/ directory adopts a different build system, and produces .cmd > # files in a different format. Do not support it. > @@ -213,8 +213,8 @@ def main(): > result = line_matcher.match(f.readline()) > if result: > try: > - entry = process_line(directory, result.group(1), > - result.group(2)) > + entry = process_line(directory, result.group('command_prefix'), > + result.group('file_path')) > compile_commands.append(entry) > except ValueError as err: > logging.info('Could not add line from %s: %s', > -- > 2.42.0.515.g380fc7ccd1-goog >
On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers <ndesaulniers@google.com> wrote: > > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> wrote: > > > > Builds in tools still use the cmd_ prefix in .cmd files, so don't > > require the saved part. Name the groups in the line pattern match so > > Is that something that can be changed in the tools/ Makefiles? > > I'm fine with this change, just curious where the difference comes > from precisely. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> I agree. The savedcmd_ change came from Masahiro in: https://lore.kernel.org/lkml/20221229091501.916296-1-masahiroy@kernel.org/ I was reluctant to change the build logic in tools/ because of the potential to break things. Maybe Masahiro/Nicolas know of issues? Thanks, Ian > > > that changing the regular expression is more robust and works with the > > addition of a new match group. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > > scripts/clang-tools/gen_compile_commands.py | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py > > index a84cc5737c2c..b43f9149893c 100755 > > --- a/scripts/clang-tools/gen_compile_commands.py > > +++ b/scripts/clang-tools/gen_compile_commands.py > > @@ -19,7 +19,7 @@ _DEFAULT_OUTPUT = 'compile_commands.json' > > _DEFAULT_LOG_LEVEL = 'WARNING' > > > > _FILENAME_PATTERN = r'^\..*\.cmd$' > > -_LINE_PATTERN = r'^savedcmd_[^ ]*\.o := (.* )([^ ]*\.[cS]) *(;|$)' > > +_LINE_PATTERN = r'^(saved)?cmd_[^ ]*\.o := (?P<command_prefix>.* )(?P<file_path>[^ ]*\.[cS]) *(;|$)' > > _VALID_LOG_LEVELS = ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL'] > > # The tools/ directory adopts a different build system, and produces .cmd > > # files in a different format. Do not support it. > > @@ -213,8 +213,8 @@ def main(): > > result = line_matcher.match(f.readline()) > > if result: > > try: > > - entry = process_line(directory, result.group(1), > > - result.group(2)) > > + entry = process_line(directory, result.group('command_prefix'), > > + result.group('file_path')) > > compile_commands.append(entry) > > except ValueError as err: > > logging.info('Could not add line from %s: %s', > > -- > > 2.42.0.515.g380fc7ccd1-goog > > > > > -- > Thanks, > ~Nick Desaulniers
On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote: > On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers > <ndesaulniers@google.com> wrote: > > > > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> wrote: > > > > > > Builds in tools still use the cmd_ prefix in .cmd files, so don't > > > require the saved part. Name the groups in the line pattern match so > > > > Is that something that can be changed in the tools/ Makefiles? > > > > I'm fine with this change, just curious where the difference comes > > from precisely. > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > I agree. The savedcmd_ change came from Masahiro in: > https://lore.kernel.org/lkml/20221229091501.916296-1-masahiroy@kernel.org/ > I was reluctant to change the build logic in tools/ because of the > potential to break things. Maybe Masahiro/Nicolas know of issues? I haven't seen any issues related to the introduction of savedcmd_; and roughly searching through tools/ I cannot find a rule that matches the pattern Masahiro described in commit 92215e7a801d ("kbuild: rename cmd_$@ to savedcmd_$@ in *.cmd files", 2022-12-29). For consistency, I'd like to see the build rules in tools/ re-use the ones from scripts/ but as of now I don't see any necessity to introduce savedcmd in tools/, yet. Kind regards, Nicolas
On Thu, Sep 28, 2023 at 11:26 PM Nicolas Schier <nicolas@fjasle.eu> wrote: > > On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote: > > On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers > > <ndesaulniers@google.com> wrote: > > > > > > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> wrote: > > > > > > > > Builds in tools still use the cmd_ prefix in .cmd files, so don't > > > > require the saved part. Name the groups in the line pattern match so > > > > > > Is that something that can be changed in the tools/ Makefiles? > > > > > > I'm fine with this change, just curious where the difference comes > > > from precisely. > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > I agree. The savedcmd_ change came from Masahiro in: > > https://lore.kernel.org/lkml/20221229091501.916296-1-masahiroy@kernel.org/ > > I was reluctant to change the build logic in tools/ because of the > > potential to break things. Maybe Masahiro/Nicolas know of issues? > > I haven't seen any issues related to the introduction of savedcmd_; and > roughly searching through tools/ I cannot find a rule that matches the > pattern Masahiro described in commit 92215e7a801d ("kbuild: rename > cmd_$@ to savedcmd_$@ in *.cmd files", 2022-12-29). For consistency, > I'd like to see the build rules in tools/ re-use the ones from scripts/ > but as of now I don't see any necessity to introduce savedcmd in > tools/, yet. > > Kind regards, > Nicolas tools/build/Build.include mimics scripts/Kbuild.include That should be changed in the same way.
On Tue, Oct 3, 2023 at 7:31 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > On Thu, Sep 28, 2023 at 11:26 PM Nicolas Schier <nicolas@fjasle.eu> wrote: > > > > On Mon, 25 Sep 2023 09:06:11 -0700, Ian Rogers wrote: > > > On Mon, Sep 25, 2023 at 8:49 AM Nick Desaulniers > > > <ndesaulniers@google.com> wrote: > > > > > > > > On Fri, Sep 22, 2023 at 10:35 PM Ian Rogers <irogers@google.com> wrote: > > > > > > > > > > Builds in tools still use the cmd_ prefix in .cmd files, so don't > > > > > require the saved part. Name the groups in the line pattern match so > > > > > > > > Is that something that can be changed in the tools/ Makefiles? > > > > > > > > I'm fine with this change, just curious where the difference comes > > > > from precisely. > > > > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > > > > > I agree. The savedcmd_ change came from Masahiro in: > > > https://lore.kernel.org/lkml/20221229091501.916296-1-masahiroy@kernel.org/ > > > I was reluctant to change the build logic in tools/ because of the > > > potential to break things. Maybe Masahiro/Nicolas know of issues? > > > > I haven't seen any issues related to the introduction of savedcmd_; and > > roughly searching through tools/ I cannot find a rule that matches the > > pattern Masahiro described in commit 92215e7a801d ("kbuild: rename > > cmd_$@ to savedcmd_$@ in *.cmd files", 2022-12-29). For consistency, > > I'd like to see the build rules in tools/ re-use the ones from scripts/ > > but as of now I don't see any necessity to introduce savedcmd in > > tools/, yet. > > > > Kind regards, > > Nicolas > > > tools/build/Build.include mimics scripts/Kbuild.include > > That should be changed in the same way. Thanks Masahiro, I support that as a goal, I'm not sure I want to embrace it here. For example, in tools/build/Build.include there is: ``` # Echo command # Short version is used, if $(quiet) equals `quiet_', otherwise full one. echo-cmd = $(if $($(quiet)cmd_$(1)),\ echo ' $(call escsq,$($(quiet)cmd_$(1)))';) ``` This is relevant given the use of cmd_ and not savedcmd_. In scripts/Kbuild.include there is no equivalent, nor is there in scripts. So do I dig into the history of echo-cmd unfork that, and then go to the next thing? I find a lot of things like tools/selftests/bpf won't build for me. When I debug a failure is it because of a change or a pre-existing issue? Given the scope of the Build.include unification problem, and this 4 line change, I hope we can move forward with this and keep unification as a separate problem (I totally support solving that problem). Thanks, Ian > -- > Best Regards > Masahiro Yamada
diff --git a/scripts/clang-tools/gen_compile_commands.py b/scripts/clang-tools/gen_compile_commands.py index a84cc5737c2c..b43f9149893c 100755 --- a/scripts/clang-tools/gen_compile_commands.py +++ b/scripts/clang-tools/gen_compile_commands.py @@ -19,7 +19,7 @@ _DEFAULT_OUTPUT = 'compile_commands.json' _DEFAULT_LOG_LEVEL = 'WARNING' _FILENAME_PATTERN = r'^\..*\.cmd$' -_LINE_PATTERN = r'^savedcmd_[^ ]*\.o := (.* )([^ ]*\.[cS]) *(;|$)' +_LINE_PATTERN = r'^(saved)?cmd_[^ ]*\.o := (?P<command_prefix>.* )(?P<file_path>[^ ]*\.[cS]) *(;|$)' _VALID_LOG_LEVELS = ['DEBUG', 'INFO', 'WARNING', 'ERROR', 'CRITICAL'] # The tools/ directory adopts a different build system, and produces .cmd # files in a different format. Do not support it. @@ -213,8 +213,8 @@ def main(): result = line_matcher.match(f.readline()) if result: try: - entry = process_line(directory, result.group(1), - result.group(2)) + entry = process_line(directory, result.group('command_prefix'), + result.group('file_path')) compile_commands.append(entry) except ValueError as err: logging.info('Could not add line from %s: %s',
Builds in tools still use the cmd_ prefix in .cmd files, so don't require the saved part. Name the groups in the line pattern match so that changing the regular expression is more robust and works with the addition of a new match group. Signed-off-by: Ian Rogers <irogers@google.com> --- scripts/clang-tools/gen_compile_commands.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)