| Message ID | 20210212204849.1556406-1-mmayer@broadcom.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | include/buildrules: substitute ".o" for ".lo" only at the very end | expand |
> To prevent issues when the ".o" extension appears in a directory path, > ensure that the ".o" -> ".lo" substitution is only performed for the > final file extension. If the subject should be "[PATCH] xfsprogs: ...", please let me know. Regards, -Markus
On 2/12/21 2:51 PM, Markus Mayer wrote: >> To prevent issues when the ".o" extension appears in a directory path, >> ensure that the ".o" -> ".lo" substitution is only performed for the >> final file extension. > > If the subject should be "[PATCH] xfsprogs: ...", please let me know. Nah, that's fine, I noticed it. So did you have a path component that had ".o" in it that got substituted? Is that what the bugfix is? Thanks, -Eric > Regards, > -Markus >
On Fri, 12 Feb 2021 at 13:29, Eric Sandeen <sandeen@sandeen.net> wrote: > > On 2/12/21 2:51 PM, Markus Mayer wrote: > >> To prevent issues when the ".o" extension appears in a directory path, > >> ensure that the ".o" -> ".lo" substitution is only performed for the > >> final file extension. > > > > If the subject should be "[PATCH] xfsprogs: ...", please let me know. > > Nah, that's fine, I noticed it. > > So did you have a path component that had ".o" in it that got substituted? > Is that what the bugfix is? Yes and yes. Specifically, I was asked to name the build directory in our build system "workspace.o" (or something else ending in .o) because that causes the automated backup to skip backing up temporary build directories, which is what we want. There is an existing exclusion pattern that skips .o files during backup runs, and they didn't want to create specialized rules for different projects. Hence the request for the oddly named directory to make it match the existing pattern. We also have a symlink without the ".o" extension (workspace -> workspace.o) which is commonly used to access the work space, but symlinks frequently get expanded when scripts run. In the end, the xfsprogs build system saw the full path without the symlink (".../workspace.o/.../xfsprogs-5.8.0/...") and started substituting workspace.o with workspace.lo. And then the build died. Like this: >>> xfsprogs 5.8.0 Building PATH="/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/host/bin:/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/host/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" /usr/bin/make -j33 -C /local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/ [HEADERS] include [HEADERS] libxfs Building include [LN] disk make[3]: Nothing to be done for 'include'. Building libfrog [CC] gen_crc32table [GENERATE] crc32table.h make[4]: *** No rule to make target '/local/users/jenkins/workspace.lo/buildroot_linux-5.4_llvm/output/arm64/target/usr/include/uuid/uuid.h', needed by 'bitmap.lo'. Stop. make[4]: *** Waiting for unfinished jobs.... [CC] avl64.lo include/buildrules:35: recipe for target 'libfrog' failed make[3]: *** [libfrog] Error 2 Makefile:91: recipe for target 'default' failed make[2]: *** [default] Error 2 package/pkg-generic.mk:247: recipe for target '/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/.stamp_built' failed make[1]: *** [/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/.stamp_built] Error 2 Regards, -Markus
On 2/12/21 3:55 PM, Markus Mayer wrote: > On Fri, 12 Feb 2021 at 13:29, Eric Sandeen <sandeen@sandeen.net> wrote: >> >> On 2/12/21 2:51 PM, Markus Mayer wrote: >>>> To prevent issues when the ".o" extension appears in a directory path, >>>> ensure that the ".o" -> ".lo" substitution is only performed for the >>>> final file extension. >>> >>> If the subject should be "[PATCH] xfsprogs: ...", please let me know. >> >> Nah, that's fine, I noticed it. >> >> So did you have a path component that had ".o" in it that got substituted? >> Is that what the bugfix is? > > Yes and yes. > > Specifically, I was asked to name the build directory in our build > system "workspace.o" (or something else ending in .o) because that > causes the automated backup to skip backing up temporary build > directories, which is what we want. There is an existing exclusion > pattern that skips .o files during backup runs, and they didn't want > to create specialized rules for different projects. Hence the request > for the oddly named directory to make it match the existing pattern. > > We also have a symlink without the ".o" extension (workspace -> > workspace.o) which is commonly used to access the work space, but > symlinks frequently get expanded when scripts run. In the end, the > xfsprogs build system saw the full path without the symlink > (".../workspace.o/.../xfsprogs-5.8.0/...") and started substituting > workspace.o with workspace.lo. And then the build died. haha, no comment on the strategy ;) But I agree that we should not be substituting anything but the root filename suffix, so the patch is fine by me. -Eric > Like this: > >>>> xfsprogs 5.8.0 Building > PATH="/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/host/bin:/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/host/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" > /usr/bin/make -j33 -C > /local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/ > [HEADERS] include > [HEADERS] libxfs > Building include > [LN] disk > make[3]: Nothing to be done for 'include'. > Building libfrog > [CC] gen_crc32table > [GENERATE] crc32table.h > make[4]: *** No rule to make target > '/local/users/jenkins/workspace.lo/buildroot_linux-5.4_llvm/output/arm64/target/usr/include/uuid/uuid.h', > needed by 'bitmap.lo'. Stop. > make[4]: *** Waiting for unfinished jobs.... > [CC] avl64.lo > include/buildrules:35: recipe for target 'libfrog' failed > make[3]: *** [libfrog] Error 2 > Makefile:91: recipe for target 'default' failed > make[2]: *** [default] Error 2 > package/pkg-generic.mk:247: recipe for target > '/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/.stamp_built' > failed > make[1]: *** [/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/.stamp_built] > Error 2 > > Regards, > -Markus >
On 2/12/21 2:48 PM, Markus Mayer wrote: > To prevent issues when the ".o" extension appears in a directory path, > ensure that the ".o" -> ".lo" substitution is only performed for the > final file extension. > > Signed-off-by: Markus Mayer <mmayer@broadcom.com> Reviewed-by: Eric Sandeen <sandeen@redhat.com> > --- > > I ran into a build issue with our setup due to the original regex below. > It was mangling the path to header files by substituting ".o" with ".lo" > one too many times. > > This patch resolves the issue. Also, it seems like the right thing to do > to limit substitutions to the final file extension in a path. > > include/buildrules | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/buildrules b/include/buildrules > index 7a139ff07de8..357e0a18504f 100644 > --- a/include/buildrules > +++ b/include/buildrules > @@ -133,7 +133,7 @@ rmltdep: > $(Q)rm -f .ltdep > > .ltdep: $(CFILES) $(HFILES) > - $(Q)$(MAKEDEP) $(CFILES) | $(SED) -e 's,^\([^:]*\)\.o,\1.lo,' > .ltdep > + $(Q)$(MAKEDEP) $(CFILES) | $(SED) -e 's,^\([^:]*\)\.o$$,\1.lo,' > .ltdep > > depend: rmdep .dep > >
On Fri, Feb 12, 2021 at 01:55:06PM -0800, Markus Mayer wrote: > On Fri, 12 Feb 2021 at 13:29, Eric Sandeen <sandeen@sandeen.net> wrote: > > > > On 2/12/21 2:51 PM, Markus Mayer wrote: > > >> To prevent issues when the ".o" extension appears in a directory path, > > >> ensure that the ".o" -> ".lo" substitution is only performed for the > > >> final file extension. > > > > > > If the subject should be "[PATCH] xfsprogs: ...", please let me know. > > > > Nah, that's fine, I noticed it. > > > > So did you have a path component that had ".o" in it that got substituted? > > Is that what the bugfix is? > > Yes and yes. > > Specifically, I was asked to name the build directory in our build > system "workspace.o" (or something else ending in .o) because that > causes the automated backup to skip backing up temporary build Does the backup not know about the NODUMP flag, aka chattr +d ? --D > directories, which is what we want. There is an existing exclusion > pattern that skips .o files during backup runs, and they didn't want > to create specialized rules for different projects. Hence the request > for the oddly named directory to make it match the existing pattern. > > We also have a symlink without the ".o" extension (workspace -> > workspace.o) which is commonly used to access the work space, but > symlinks frequently get expanded when scripts run. In the end, the > xfsprogs build system saw the full path without the symlink > (".../workspace.o/.../xfsprogs-5.8.0/...") and started substituting > workspace.o with workspace.lo. And then the build died. > > Like this: > > >>> xfsprogs 5.8.0 Building > PATH="/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/host/bin:/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/host/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" > /usr/bin/make -j33 -C > /local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/ > [HEADERS] include > [HEADERS] libxfs > Building include > [LN] disk > make[3]: Nothing to be done for 'include'. > Building libfrog > [CC] gen_crc32table > [GENERATE] crc32table.h > make[4]: *** No rule to make target > '/local/users/jenkins/workspace.lo/buildroot_linux-5.4_llvm/output/arm64/target/usr/include/uuid/uuid.h', > needed by 'bitmap.lo'. Stop. > make[4]: *** Waiting for unfinished jobs.... > [CC] avl64.lo > include/buildrules:35: recipe for target 'libfrog' failed > make[3]: *** [libfrog] Error 2 > Makefile:91: recipe for target 'default' failed > make[2]: *** [default] Error 2 > package/pkg-generic.mk:247: recipe for target > '/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/.stamp_built' > failed > make[1]: *** [/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/.stamp_built] > Error 2 > > Regards, > -Markus
On Fri, 12 Feb 2021 at 14:15, Eric Sandeen <sandeen@sandeen.net> wrote: > > On 2/12/21 3:55 PM, Markus Mayer wrote: > > On Fri, 12 Feb 2021 at 13:29, Eric Sandeen <sandeen@sandeen.net> wrote: > >> > >> On 2/12/21 2:51 PM, Markus Mayer wrote: > >>>> To prevent issues when the ".o" extension appears in a directory path, > >>>> ensure that the ".o" -> ".lo" substitution is only performed for the > >>>> final file extension. > >>> > >>> If the subject should be "[PATCH] xfsprogs: ...", please let me know. > >> > >> Nah, that's fine, I noticed it. > >> > >> So did you have a path component that had ".o" in it that got substituted? > >> Is that what the bugfix is? > > > > Yes and yes. > > > > Specifically, I was asked to name the build directory in our build > > system "workspace.o" (or something else ending in .o) because that > > causes the automated backup to skip backing up temporary build > > directories, which is what we want. There is an existing exclusion > > pattern that skips .o files during backup runs, and they didn't want > > to create specialized rules for different projects. Hence the request > > for the oddly named directory to make it match the existing pattern. > > > > We also have a symlink without the ".o" extension (workspace -> > > workspace.o) which is commonly used to access the work space, but > > symlinks frequently get expanded when scripts run. In the end, the > > xfsprogs build system saw the full path without the symlink > > (".../workspace.o/.../xfsprogs-5.8.0/...") and started substituting > > workspace.o with workspace.lo. And then the build died. > > haha, no comment on the strategy ;) You won't hear an argument from me. *LOL* I had a sneaking suspicion that the "workspace.o" strategy might trigger some fallout. Turns out I wasn't wrong. > But I agree that we should not be substituting anything but the root filename > suffix, so the patch is fine by me. Thanks, -Markus > -Eric
On Fri, 12 Feb 2021 at 14:21, Darrick J. Wong <djwong@kernel.org> wrote: > > On Fri, Feb 12, 2021 at 01:55:06PM -0800, Markus Mayer wrote: > > On Fri, 12 Feb 2021 at 13:29, Eric Sandeen <sandeen@sandeen.net> wrote: > > > > > > On 2/12/21 2:51 PM, Markus Mayer wrote: > > > >> To prevent issues when the ".o" extension appears in a directory path, > > > >> ensure that the ".o" -> ".lo" substitution is only performed for the > > > >> final file extension. > > > > > > > > If the subject should be "[PATCH] xfsprogs: ...", please let me know. > > > > > > Nah, that's fine, I noticed it. > > > > > > So did you have a path component that had ".o" in it that got substituted? > > > Is that what the bugfix is? > > > > Yes and yes. > > > > Specifically, I was asked to name the build directory in our build > > system "workspace.o" (or something else ending in .o) because that > > causes the automated backup to skip backing up temporary build > > Does the backup not know about the NODUMP flag, aka chattr +d ? I don't know. I'll follow up with the team that handles the infrastructure. I believe there's some file access over NFS involved here, and I am not sure if the NODUMP flag would be available via NFS. Regards, -Markus
On Fri, Feb 12, 2021 at 01:55:06PM -0800, Markus Mayer wrote: > On Fri, 12 Feb 2021 at 13:29, Eric Sandeen <sandeen@sandeen.net> wrote: > > > > On 2/12/21 2:51 PM, Markus Mayer wrote: > > >> To prevent issues when the ".o" extension appears in a directory path, > > >> ensure that the ".o" -> ".lo" substitution is only performed for the > > >> final file extension. > > > > > > If the subject should be "[PATCH] xfsprogs: ...", please let me know. > > > > Nah, that's fine, I noticed it. > > > > So did you have a path component that had ".o" in it that got substituted? > > Is that what the bugfix is? > > Yes and yes. > > Specifically, I was asked to name the build directory in our build > system "workspace.o" (or something else ending in .o) because that > causes the automated backup to skip backing up temporary build > directories, which is what we want. There is an existing exclusion > pattern that skips .o files during backup runs, and they didn't want > to create specialized rules for different projects. Hence the request > for the oddly named directory to make it match the existing pattern. > > We also have a symlink without the ".o" extension (workspace -> > workspace.o) which is commonly used to access the work space, but > symlinks frequently get expanded when scripts run. In the end, the > xfsprogs build system saw the full path without the symlink > (".../workspace.o/.../xfsprogs-5.8.0/...") and started substituting > workspace.o with workspace.lo. And then the build died. > > Like this: > > >>> xfsprogs 5.8.0 Building > PATH="/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/host/bin:/local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/host/sbin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin" > /usr/bin/make -j33 -C > /local/users/jenkins/workspace.o/buildroot_linux-5.4_llvm/output/arm64/build/xfsprogs-5.8.0/ > [HEADERS] include > [HEADERS] libxfs > Building include > [LN] disk > make[3]: Nothing to be done for 'include'. > Building libfrog > [CC] gen_crc32table > [GENERATE] crc32table.h > make[4]: *** No rule to make target > '/local/users/jenkins/workspace.lo/buildroot_linux-5.4_llvm/output/arm64/target/usr/include/uuid/uuid.h', > needed by 'bitmap.lo'. Stop. So there's a rule like this generated: bitmap.lo: bitmap.c ../include/xfs.h ../include/xfs/linux.h \ ../include/xfs/xfs_types.h ../include/xfs/xfs_fs_compat.h \ ../include/xfs/xfs_fs.h ../include/platform_defs.h avl64.h \ ../include/list.h bitmap.h The sed expression is: $(SED) -e 's,^\([^:]*\)\.o,\1.lo,' Which means it is supposed to match from the start of line to a ".o", and store everything up to the ".o" in register 1. And the problem is that it is matching a ".o" in the middle of a string. So existing behaviour: $ echo "foo.o/bitmap.o: bitmap.c ../include/xfs.h ../include/xfs/linux.h" | sed -e 's,^\([^:]*\)\.o,\1.lo,' foo.o/bitmap.lo: bitmap.c ../include/xfs.h ../include/xfs/linux.h $ Looks correct until the dependency line is split and the first entry in the split line is something like "foo.o/uuid.h" Your modified version: $ echo "bitmap.o: bitmap.c ../include/xfs.h ../include/xfs/linux.h" | sed -e 's,^\([^:]*\)\.o$$,\1.lo,' bitmap.o: bitmap.c ../include/xfs.h ../include/xfs/linux.h $ Doesn't work for the case we actually need the substitution for. So, really, I think we need to match against the full target specification rather than just ".o". Something like $SED -e 's,^\([^:]*\)\.o: ,\1.lo: ,' $ echo "foo.o/bitmap: bitmap.c ../include/xfs.h ../include/xfs/linux.h" | sed -e 's,^\([^:]*\)\.o: ,\1.lo: ,' foo.o/bitmap: bitmap.c ../include/xfs.h ../include/xfs/linux.h $ echo "foo.o/bitmap.o: bitmap.c ../include/xfs.h ../include/xfs/linux.h" | sed -e 's,^\([^:]*\)\.o: ,\1.lo: ,' foo.o/bitmap.lo: bitmap.c ../include/xfs.h ../include/xfs/linux.h Cheers, Dave.
On Fri, 12 Feb 2021 at 14:45, Dave Chinner <david@fromorbit.com> wrote: > > So there's a rule like this generated: > > bitmap.lo: bitmap.c ../include/xfs.h ../include/xfs/linux.h \ > ../include/xfs/xfs_types.h ../include/xfs/xfs_fs_compat.h \ > ../include/xfs/xfs_fs.h ../include/platform_defs.h avl64.h \ > ../include/list.h bitmap.h > > The sed expression is: > > $(SED) -e 's,^\([^:]*\)\.o,\1.lo,' > > Which means it is supposed to match from the start of line to a ".o", > and store everything up to the ".o" in register 1. > > And the problem is that it is matching a ".o" in the middle of a > string. > > So existing behaviour: > > $ echo "foo.o/bitmap.o: bitmap.c ../include/xfs.h ../include/xfs/linux.h" | sed -e 's,^\([^:]*\)\.o,\1.lo,' > foo.o/bitmap.lo: bitmap.c ../include/xfs.h ../include/xfs/linux.h > $ > > Looks correct until the dependency line is split and the first entry > in the split line is something like "foo.o/uuid.h" > > Your modified version: > > $ echo "bitmap.o: bitmap.c ../include/xfs.h ../include/xfs/linux.h" | sed -e 's,^\([^:]*\)\.o$$,\1.lo,' > bitmap.o: bitmap.c ../include/xfs.h ../include/xfs/linux.h > $ > > Doesn't work for the case we actually need the substitution for. > > So, really, I think we need to match against the full target > specification rather than just ".o". > > Something like > > $SED -e 's,^\([^:]*\)\.o: ,\1.lo: ,' > > $ echo "foo.o/bitmap: bitmap.c ../include/xfs.h ../include/xfs/linux.h" | sed -e 's,^\([^:]*\)\.o: ,\1.lo: ,' > foo.o/bitmap: bitmap.c ../include/xfs.h ../include/xfs/linux.h > $ echo "foo.o/bitmap.o: bitmap.c ../include/xfs.h ../include/xfs/linux.h" | sed -e 's,^\([^:]*\)\.o: ,\1.lo: ,' > foo.o/bitmap.lo: bitmap.c ../include/xfs.h ../include/xfs/linux.h Thanks. I just tried your suggestion. Looks like it's working for my situation. I'll submit a v2 of the patch. Regards, -Markus
diff --git a/include/buildrules b/include/buildrules index 7a139ff07de8..357e0a18504f 100644 --- a/include/buildrules +++ b/include/buildrules @@ -133,7 +133,7 @@ rmltdep: $(Q)rm -f .ltdep .ltdep: $(CFILES) $(HFILES) - $(Q)$(MAKEDEP) $(CFILES) | $(SED) -e 's,^\([^:]*\)\.o,\1.lo,' > .ltdep + $(Q)$(MAKEDEP) $(CFILES) | $(SED) -e 's,^\([^:]*\)\.o$$,\1.lo,' > .ltdep depend: rmdep .dep
To prevent issues when the ".o" extension appears in a directory path, ensure that the ".o" -> ".lo" substitution is only performed for the final file extension. Signed-off-by: Markus Mayer <mmayer@broadcom.com> --- I ran into a build issue with our setup due to the original regex below. It was mangling the path to header files by substituting ".o" with ".lo" one too many times. This patch resolves the issue. Also, it seems like the right thing to do to limit substitutions to the final file extension in a path. include/buildrules | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)