| Message ID | 3554293.nIQqjdAT3l@polaris |
|---|---|
| State | New |
| Headers | show |
On 06/04/16 09:39 +0200, Eric Botcazou wrote: >Hi, > >we recently ran into build failures on Windows systems using a somewhat old >grep, coming from a syntax error in the libstdc++-symbols.ver version file: > ># Symbol versioning for shared libraries. >if ENABLE_SYMVERS >libstdc++-symbols.ver: ${glibcxx_srcdir}/$(SYMVER_FILE) \ > $(port_specific_symbol_files) > cp ${glibcxx_srcdir}/$(SYMVER_FILE) $@.tmp > chmod +w $@.tmp > if test "x$(port_specific_symbol_files)" != x; then \ > if grep '^# Appended to version file.' \ > $(port_specific_symbol_files) /dev/null > /dev/null 2>&1; then >\ > cat $(port_specific_symbol_files) >> $@.tmp; \ > else \ > sed -n '1,/DO NOT DELETE/p' $@.tmp > tmp.top; \ > sed -n '/DO NOT DELETE/,$$p' $@.tmp > tmp.bottom; \ > cat tmp.top $(port_specific_symbol_files) tmp.bottom > $@.tmp; \ > rm tmp.top tmp.bottom; \ > fi; \ > fi > >Note the double /dev/null on the grep command line. The first one causes the >grep to fail when the command is invoked on these systems. That's old code, >but it is now invoked for config/abi/pre/float128.ver on the mainline and 5 >branch and this breaks the build on these systems (4.9 builds fine). > >This first /dev/null doesn't serve any useful purpose and seems to be a typo, Doesn't it mean that if $port_specific_symbol_files contains only whitespace we don't hang waiting for input from stdin? The 'if' above it will be true when "x$port_specific_symbol_files" = "x " or similar. I don't see any way for that to happen in the FSF tree, so it should be safe. I'm a bit concerned about making that change this late in stage 4 though. There isn't much time to find out if it breaks an obscure target.
On Wed, Apr 06, 2016 at 09:50:48AM +0100, Jonathan Wakely wrote: > On 06/04/16 09:39 +0200, Eric Botcazou wrote: > >we recently ran into build failures on Windows systems using a somewhat old > >grep, coming from a syntax error in the libstdc++-symbols.ver version file: > > > ># Symbol versioning for shared libraries. > >if ENABLE_SYMVERS > >libstdc++-symbols.ver: ${glibcxx_srcdir}/$(SYMVER_FILE) \ > > $(port_specific_symbol_files) > > cp ${glibcxx_srcdir}/$(SYMVER_FILE) $@.tmp > > chmod +w $@.tmp > > if test "x$(port_specific_symbol_files)" != x; then \ > > if grep '^# Appended to version file.' \ > > $(port_specific_symbol_files) /dev/null > /dev/null 2>&1; then > >\ > > cat $(port_specific_symbol_files) >> $@.tmp; \ > > else \ > > sed -n '1,/DO NOT DELETE/p' $@.tmp > tmp.top; \ > > sed -n '/DO NOT DELETE/,$$p' $@.tmp > tmp.bottom; \ > > cat tmp.top $(port_specific_symbol_files) tmp.bottom > $@.tmp; \ > > rm tmp.top tmp.bottom; \ > > fi; \ > > fi > > > >Note the double /dev/null on the grep command line. The first one causes the > >grep to fail when the command is invoked on these systems. That's old code, > >but it is now invoked for config/abi/pre/float128.ver on the mainline and 5 > >branch and this breaks the build on these systems (4.9 builds fine). > > > >This first /dev/null doesn't serve any useful purpose and seems to be a typo, > > Doesn't it mean that if $port_specific_symbol_files contains only > whitespace we don't hang waiting for input from stdin? The 'if' above > it will be true when "x$port_specific_symbol_files" = "x " or similar. > > I don't see any way for that to happen in the FSF tree, so it should > be safe. I'm a bit concerned about making that change this late in > stage 4 though. There isn't much time to find out if it breaks an > obscure target. As it is a make variable, can't make be used to test this? So perhaps chmod +w $@.tmp ifneq ($(port_specific_symbol_files),) if grep '^# Appended to version file.' \ $(port_specific_symbol_files) /dev/null > /dev/null 2>&1; then \ cat $(port_specific_symbol_files) >> $@.tmp; \ else \ sed -n '1,/DO NOT DELETE/p' $@.tmp > tmp.top; \ sed -n '/DO NOT DELETE/,$$p' $@.tmp > tmp.bottom; \ cat tmp.top $(port_specific_symbol_files) tmp.bottom > $@.tmp; \ rm tmp.top tmp.bottom; \ fi; endif ? Though, I think the initial and trailing whitespace is removed during expansion (or already parsing of the vars), so even the test "x$(port_specific_symbol_files)" != x check should work right. Jakub
On 06/04/16 11:01 +0200, Jakub Jelinek wrote: >On Wed, Apr 06, 2016 at 09:50:48AM +0100, Jonathan Wakely wrote: >> On 06/04/16 09:39 +0200, Eric Botcazou wrote: >> >we recently ran into build failures on Windows systems using a somewhat old >> >grep, coming from a syntax error in the libstdc++-symbols.ver version file: >> > >> ># Symbol versioning for shared libraries. >> >if ENABLE_SYMVERS >> >libstdc++-symbols.ver: ${glibcxx_srcdir}/$(SYMVER_FILE) \ >> > $(port_specific_symbol_files) >> > cp ${glibcxx_srcdir}/$(SYMVER_FILE) $@.tmp >> > chmod +w $@.tmp >> > if test "x$(port_specific_symbol_files)" != x; then \ >> > if grep '^# Appended to version file.' \ >> > $(port_specific_symbol_files) /dev/null > /dev/null 2>&1; then >> >\ >> > cat $(port_specific_symbol_files) >> $@.tmp; \ >> > else \ >> > sed -n '1,/DO NOT DELETE/p' $@.tmp > tmp.top; \ >> > sed -n '/DO NOT DELETE/,$$p' $@.tmp > tmp.bottom; \ >> > cat tmp.top $(port_specific_symbol_files) tmp.bottom > $@.tmp; \ >> > rm tmp.top tmp.bottom; \ >> > fi; \ >> > fi >> > >> >Note the double /dev/null on the grep command line. The first one causes the >> >grep to fail when the command is invoked on these systems. That's old code, >> >but it is now invoked for config/abi/pre/float128.ver on the mainline and 5 >> >branch and this breaks the build on these systems (4.9 builds fine). >> > >> >This first /dev/null doesn't serve any useful purpose and seems to be a typo, >> >> Doesn't it mean that if $port_specific_symbol_files contains only >> whitespace we don't hang waiting for input from stdin? The 'if' above >> it will be true when "x$port_specific_symbol_files" = "x " or similar. >> >> I don't see any way for that to happen in the FSF tree, so it should >> be safe. I'm a bit concerned about making that change this late in >> stage 4 though. There isn't much time to find out if it breaks an >> obscure target. > >As it is a make variable, can't make be used to test this? >So perhaps > chmod +w $@.tmp >ifneq ($(port_specific_symbol_files),) > if grep '^# Appended to version file.' \ > $(port_specific_symbol_files) /dev/null > /dev/null 2>&1; then \ > cat $(port_specific_symbol_files) >> $@.tmp; \ > else \ > sed -n '1,/DO NOT DELETE/p' $@.tmp > tmp.top; \ > sed -n '/DO NOT DELETE/,$$p' $@.tmp > tmp.bottom; \ > cat tmp.top $(port_specific_symbol_files) tmp.bottom > $@.tmp; \ > rm tmp.top tmp.bottom; \ > fi; >endif >? Though, I think the initial and trailing whitespace is removed during >expansion (or already parsing of the vars), so even the >test "x$(port_specific_symbol_files)" != x >check should work right. OK, I have no objection to the original patch then.
On Wed, Apr 06, 2016 at 10:12:18AM +0100, Jonathan Wakely wrote: > >As it is a make variable, can't make be used to test this? > >So perhaps > > chmod +w $@.tmp > >ifneq ($(port_specific_symbol_files),) > > if grep '^# Appended to version file.' \ > > $(port_specific_symbol_files) /dev/null > /dev/null 2>&1; then \ > > cat $(port_specific_symbol_files) >> $@.tmp; \ > > else \ > > sed -n '1,/DO NOT DELETE/p' $@.tmp > tmp.top; \ > > sed -n '/DO NOT DELETE/,$$p' $@.tmp > tmp.bottom; \ > > cat tmp.top $(port_specific_symbol_files) tmp.bottom > $@.tmp; \ > > rm tmp.top tmp.bottom; \ > > fi; > >endif > >? Though, I think the initial and trailing whitespace is removed during > >expansion (or already parsing of the vars), so even the > >test "x$(port_specific_symbol_files)" != x > >check should work right. > > OK, I have no objection to the original patch then. To correct myself, only leading whitespace is removed, trailing is not, but when we do care about vars containing only whitespace, that means removing everything. Jakub
> OK, I have no objection to the original patch then.
Thanks, applied. FWIW I verified that the library still builds after the
change with an empty port_specific_symbol_files variable.
Index: src/Makefile.am =================================================================== --- src/Makefile.am (revision 234695) +++ src/Makefile.am (working copy) @@ -228,7 +228,7 @@ libstdc++-symbols.ver: ${glibcxx_srcdir chmod +w $@.tmp if test "x$(port_specific_symbol_files)" != x; then \ if grep '^# Appended to version file.' \ - $(port_specific_symbol_files) /dev/null > /dev/null 2>&1; then \ + $(port_specific_symbol_files) > /dev/null 2>&1; then \ cat $(port_specific_symbol_files) >> $@.tmp; \ else \ sed -n '1,/DO NOT DELETE/p' $@.tmp > tmp.top; \
Hi, we recently ran into build failures on Windows systems using a somewhat old grep, coming from a syntax error in the libstdc++-symbols.ver version file: # Symbol versioning for shared libraries. if ENABLE_SYMVERS libstdc++-symbols.ver: ${glibcxx_srcdir}/$(SYMVER_FILE) \ $(port_specific_symbol_files) cp ${glibcxx_srcdir}/$(SYMVER_FILE) $@.tmp chmod +w $@.tmp if test "x$(port_specific_symbol_files)" != x; then \ if grep '^# Appended to version file.' \ $(port_specific_symbol_files) /dev/null > /dev/null 2>&1; then \ cat $(port_specific_symbol_files) >> $@.tmp; \ else \ sed -n '1,/DO NOT DELETE/p' $@.tmp > tmp.top; \ sed -n '/DO NOT DELETE/,$$p' $@.tmp > tmp.bottom; \ cat tmp.top $(port_specific_symbol_files) tmp.bottom > $@.tmp; \ rm tmp.top tmp.bottom; \ fi; \ fi Note the double /dev/null on the grep command line. The first one causes the grep to fail when the command is invoked on these systems. That's old code, but it is now invoked for config/abi/pre/float128.ver on the mainline and 5 branch and this breaks the build on these systems (4.9 builds fine). This first /dev/null doesn't serve any useful purpose and seems to be a typo, so the attached patch gets rid of it. Tested on x86/Windows and x86-64/Linux, OK for mainline and 5 branch? 2016-04-06 Eric Botcazou <ebotcazou@adacore.com> libstdc++-v3/ * src/Makefile.am (libstdc++-symbols.ver): Remove useless /dev/null. * src/Makefile.in: Regenerate.