#1661: ExternalLibraries/PAPI does not build with gcc 4.8.2 ------------------------------------+--------------------------------------- Reporter: tbode | Owner: tbode Type: defect | Status: new Priority: major | Milestone: ET_2014_11 Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: PAPI ------------------------------------+---------------------------------------

Comment (by eschnett):

Removing -Werror via a patch is cumbersome and brittle. Instead, they should be replaced by a perl command via search-and-replace. (This is most likely how the patch was generated in the first place.)

Unfortunately, this perl command seems to have gotten lost. A similar perl command for handling "malloc.h" is still there.

This perl command would look something like {{{ find . -type f -print | xargs perl -pi -e 's/-Werror//g' }}}

#1661: ExternalLibraries/PAPI does not build with gcc 4.8.2 ------------------------------------+--------------------------------------- Reporter: tbode | Owner: tbode Type: defect | Status: new Priority: major | Milestone: ET_2014_11 Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: PAPI ------------------------------------+---------------------------------------

Comment (by rhaas):

I'm not sure if using a hand-written search-and-replace is the best solution to this. It seems to me that patches are fragile only because we tended to include in them the name of the output directory (so papi-5.3 or so appears in the patch file). However that can be avoided if instead of {{{ patch -p0 <somepatch.patch }}} we use {{{ pushd ${NAME} patch -p1 <somepatch.patch }}} as Tanja's proposed fix does.

The major disadvantage with using perl to me seems that it is actually too flexible. Patch files have a fairly standard syntax and it is straightforward to see what is changed. With a potentially complex perl script it is much harder to see what is changed (and in which files). It also requires that one reads all of configure.sh to see what changes are done.

There's also the usual issue that {{{find -print | xargs foo}}} does not deal with file names containing spaces (or more exotic things like newlines) but given the "portable" solutions mentioned on http://www.unix.com/man-page/POSIX/0/xargs/ I am inclined to wait for it to actually cause problems before even attempting to implement those solutions.

Please note: I fully agree that the patches as currently used are fragile and require lots of work maintaining (and Erik shoulders most of the burden of maintaining most of the ExternalLibraries) and that the perl script proposed will very likely be more robust against trivial changes in the source files.

#1661: ExternalLibraries/PAPI does not build with gcc 4.8.2 ------------------------------------+--------------------------------------- Reporter: tbode | Owner: tbode Type: defect | Status: new Priority: major | Milestone: ET_2014_11 Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: PAPI ------------------------------------+---------------------------------------

Comment (by rhaas):

Well I did not want to step on anyone's toes. :-).

I don't do (a) of course. I tend to iterate between compiling, fixing the compiler reported errors and compile again until the code compiles (ie the same procedure as when fixing code of my own). Then I do a diff between the original version and the fixed one. So it's more like (b) but with a manual fix rather than constructing a perl regex which I have to test and verify by doing a diff to see what it actually changed.

I agree that for this change (removing -Werror) the perl script is shorter and not more complex than the patch. Pelr "wins" when the number of affected files is large and the replacement is simple. Perl **may** affect innocent files (eg changelogs) but no one should (have to) look at those files. It also means that there are potentially multiple locations to check for "patches" namely the configure script for the perl call and the dist directory for more complex changes.

Just as long we don't try to do what http://svn.cactuscode.org/projects/ExternalLibraries/LORENE/trunk/dist/check... does in perl, I am happy.

For many tools, but in particular with perl I would rather not want use shortness of the script as a measure for its simplicity :-). For example the perl scrip used {{{ perl -pi -e 's/malloc.h/stdlib.h/' }}} contains a (most likely never triggered) bug since {{{.}}} is not properly escape inside of the regex. It should be {{{ perl -pi -e 's/malloc.h/stdlib.h/' }}} or any amount of extra complication if there'd ever by something like "mymalloc.h" if one would want to be pedantic.

So: the perl solution is fine with me, but I do reserver the right to complain again if the perl expressions become truly complex.

#1661: ExternalLibraries/PAPI does not build with gcc 4.8.2 ------------------------------------+--------------------------------------- Reporter: tbode | Owner: tbode Type: defect | Status: new Priority: major | Milestone: ET_2014_11 Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: PAPI ------------------------------------+---------------------------------------

Comment (by tbode):

Upon retrospect the perl method, while more dangerous in general, is more elegant and useful in this case. For this ''specific case'' of removing/modifying a compiler flag, the danger is minimal and could be decreased by only applying the perl script on files where this issue could possibly come up (config.mk files in this case's scheme as I see it):

{{{find . -name config.mk -print | xargs perl -pi -e 's/-Werror//g'}}}

This would leave any changelogs alone (though comments in the mk files could be modified) and be more likely to not require modification between PAPI version updates. I would make more of an issue if the change were more complicated, but I don't think this is particularly worth the effort for this patch. Whether you want to continue the discussion on the other perl-script based patch is beyond this ticket.

#1661: ExternalLibraries/PAPI does not build with gcc 4.8.2 ------------------------------------+--------------------------------------- Reporter: tbode | Owner: tbode Type: defect | Status: review Priority: major | Milestone: ET_2014_11 Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: PAPI ------------------------------------+--------------------------------------- Changes (by rhaas):

* status: new => review

#1661: ExternalLibraries/PAPI does not build with gcc 4.8.2 ------------------------------------+--------------------------------------- Reporter: tbode | Owner: tbode Type: defect | Status: reviewed_ok Priority: major | Milestone: ET_2014_11 Component: EinsteinToolkit thorn | Version: development version Resolution: | Keywords: PAPI ------------------------------------+---------------------------------------

Comment (by tbode):

The line I posted above works for me, but I do not have commit rights to that repository.