| Message ID | 20240917141725.466514-9-masahiroy@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kbuild: support building external modules in a separate build directory | expand |
On Tue, Sep 17, 2024 at 11:16:36PM +0900, Masahiro Yamada wrote: > The current 'find' command does not prune the rust/test directory > itself, requiring an additional 'grep -Fv' command to exclude it. > This is cumbersome. > > The correct use of the -prune option can be seen in the 'make clean' > rule. > > [Current command] > > $ find . -type f -name '*.rs' -o -path ./rust/test -prune | wc > 70 70 1939 > $ find . -type f -name '*.rs' -o -path ./rust/test -prune | grep rust/test > ./rust/test > > [Improved command] > > $ find . -path ./rust/test -prune -o -type f -name '*.rs' -print | wc > 69 69 1927 > $ find . -path ./rust/test -prune -o -type f -name '*.rs' -print | grep rust/test > > With the improved 'find' command, the grep command is no longer needed. > > There is also no need to use the absolute path, so $(abs_srctree) can be > replaced with $(srctree). > > The pruned directory rust/test must be prefixed with $(srctree) instead > of $(objtree). Otherwise, 'make O=... rustfmt' would visit the stale > rust/test directory remaining in the source tree. > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > --- > > Makefile | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/Makefile b/Makefile > index 5b16e0605a77..4992b2895dd5 100644 > --- a/Makefile > +++ b/Makefile > @@ -1740,9 +1740,8 @@ PHONY += rustfmt rustfmtcheck > # when matching, which is a problem when e.g. `srctree` is `..`. > # We `grep` afterwards in order to remove the directory entry itself. > rustfmt: > - $(Q)find $(abs_srctree) -type f -name '*.rs' \ > - -o -path $(abs_objtree)/rust/test -prune \ > - | grep -Fv $(abs_objtree)/rust/test \ > + $(Q)find $(srctree) -path $(srctree)/rust/test -prune \ > + -o -type f -name '*.rs' -print \ > | grep -Fv generated \ Is there a reason for keeping the grep for generated instead of turning it also into a find prune argument? Reviewed-by: Nicolas Schier <n.schier@avm.de>
On Thu, Sep 19, 2024 at 10:54 PM Nicolas Schier <n.schier@avm.de> wrote: > > On Tue, Sep 17, 2024 at 11:16:36PM +0900, Masahiro Yamada wrote: > > The current 'find' command does not prune the rust/test directory > > itself, requiring an additional 'grep -Fv' command to exclude it. > > This is cumbersome. > > > > The correct use of the -prune option can be seen in the 'make clean' > > rule. > > > > [Current command] > > > > $ find . -type f -name '*.rs' -o -path ./rust/test -prune | wc > > 70 70 1939 > > $ find . -type f -name '*.rs' -o -path ./rust/test -prune | grep rust/test > > ./rust/test > > > > [Improved command] > > > > $ find . -path ./rust/test -prune -o -type f -name '*.rs' -print | wc > > 69 69 1927 > > $ find . -path ./rust/test -prune -o -type f -name '*.rs' -print | grep rust/test > > > > With the improved 'find' command, the grep command is no longer needed. > > > > There is also no need to use the absolute path, so $(abs_srctree) can be > > replaced with $(srctree). > > > > The pruned directory rust/test must be prefixed with $(srctree) instead > > of $(objtree). Otherwise, 'make O=... rustfmt' would visit the stale > > rust/test directory remaining in the source tree. > > > > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> > > --- > > > > Makefile | 5 ++--- > > 1 file changed, 2 insertions(+), 3 deletions(-) > > > > diff --git a/Makefile b/Makefile > > index 5b16e0605a77..4992b2895dd5 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -1740,9 +1740,8 @@ PHONY += rustfmt rustfmtcheck > > # when matching, which is a problem when e.g. `srctree` is `..`. > > # We `grep` afterwards in order to remove the directory entry itself. > > rustfmt: > > - $(Q)find $(abs_srctree) -type f -name '*.rs' \ > > - -o -path $(abs_objtree)/rust/test -prune \ > > - | grep -Fv $(abs_objtree)/rust/test \ > > + $(Q)find $(srctree) -path $(srctree)/rust/test -prune \ > > + -o -type f -name '*.rs' -print \ > > | grep -Fv generated \ > > Is there a reason for keeping the grep for generated instead of turning > it also into a find prune argument? This commit answers your question: https://github.com/Rust-for-Linux/linux/commit/73243a8a27a67
On Tue, Sep 17, 2024 at 4:17 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > The correct use of the -prune option can be seen in the 'make clean' > rule. Yeah, this `-prune` should not have been like that -- sorry about that. The comment above this recipe should be updated. I am not sure I understand the part of the commit message about the rust/test change. Do you mean that we should use `srctree` in case there is a stale one in the source tree from a previous non-completely-clean in-source-tree build? I think the original intention was to skip the objtree one if it were a subdir of srctree (and that is why the use of absolute paths). Although I think we can simplify further by just removing the logic about `rust/test`, since we don't generate `*.rs` files there anyway at the moment. Cheers, Miguel
On Tue, Oct 1, 2024 at 3:38 AM Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> wrote: > > On Tue, Sep 17, 2024 at 4:17 PM Masahiro Yamada <masahiroy@kernel.org> wrote: > > > > The correct use of the -prune option can be seen in the 'make clean' > > rule. > > Yeah, this `-prune` should not have been like that -- sorry about that. > > The comment above this recipe should be updated. > > I am not sure I understand the part of the commit message about the > rust/test change. Do you mean that we should use `srctree` in case > there is a stale one in the source tree from a previous > non-completely-clean in-source-tree build? Correct. > I think the original > intention was to skip the objtree one if it were a subdir of srctree > (and that is why the use of absolute paths). OK, understood. If you insist on the current logic, I will keep it as-is, but I need to replace $(abs_objtree) with $(CURDIR) because $(abs_objtree) will be removed by this series. > Although I think we can simplify further by just removing the logic > about `rust/test`, since we don't generate `*.rs` files there anyway > at the moment. OK, then we can remove -prune entirely. -- Best Regards Masahiro Yamada
On Tue, Oct 1, 2024 at 4:18 AM Masahiro Yamada <masahiroy@kernel.org> wrote: > > OK, then we can remove -prune entirely. Yeah, I think that is the simplest approach. With those changes, please feel free to add: Acked-by: Miguel Ojeda <ojeda@kernel.org> Thanks! Cheers, Miguel
diff --git a/Makefile b/Makefile index 5b16e0605a77..4992b2895dd5 100644 --- a/Makefile +++ b/Makefile @@ -1740,9 +1740,8 @@ PHONY += rustfmt rustfmtcheck # when matching, which is a problem when e.g. `srctree` is `..`. # We `grep` afterwards in order to remove the directory entry itself. rustfmt: - $(Q)find $(abs_srctree) -type f -name '*.rs' \ - -o -path $(abs_objtree)/rust/test -prune \ - | grep -Fv $(abs_objtree)/rust/test \ + $(Q)find $(srctree) -path $(srctree)/rust/test -prune \ + -o -type f -name '*.rs' -print \ | grep -Fv generated \ | xargs $(RUSTFMT) $(rustfmt_flags)
The current 'find' command does not prune the rust/test directory itself, requiring an additional 'grep -Fv' command to exclude it. This is cumbersome. The correct use of the -prune option can be seen in the 'make clean' rule. [Current command] $ find . -type f -name '*.rs' -o -path ./rust/test -prune | wc 70 70 1939 $ find . -type f -name '*.rs' -o -path ./rust/test -prune | grep rust/test ./rust/test [Improved command] $ find . -path ./rust/test -prune -o -type f -name '*.rs' -print | wc 69 69 1927 $ find . -path ./rust/test -prune -o -type f -name '*.rs' -print | grep rust/test With the improved 'find' command, the grep command is no longer needed. There is also no need to use the absolute path, so $(abs_srctree) can be replaced with $(srctree). The pruned directory rust/test must be prefixed with $(srctree) instead of $(objtree). Otherwise, 'make O=... rustfmt' would visit the stale rust/test directory remaining in the source tree. Signed-off-by: Masahiro Yamada <masahiroy@kernel.org> --- Makefile | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)