ExternalLibraries/PAPI does not build with gcc 4.8.2

Comments (13)

1. 

   Erik Schnetter

   • removed comment

   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'
   

   • 2014-09-29T09:38:33+00:00
2. 

   Tanja Bode reporter

   • removed comment

   Aha. I had noticed that malloc.h command. Adding that line by the malloc.h lines does indeed fix the problem.

   • 2014-09-29T09:50:35+00:00
3. 

   Roland Haas

   • removed comment

   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.

   • 2014-09-29T11:45:18+00:00
4. 

   Erik Schnetter

   • removed comment

   Note that the "potentially complex perl script" contains 12 (twelve) characters. Your call to patch is longer than this.

   Also -- how would you generate the patch? Would you (a) manually read all source files, or (b) use "grep" to look for "-Werror", or (c) wait until something breaks and then update the patch?

   (a) is not feasible. (b) is just what perl does, only more automatic. And (c) continues to work fine; we'll use perl until something breaks and then we'll update our method.

   • 2014-09-29T11:51:09+00:00
5. 

   Roland Haas

   • removed comment

   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_fopen_error.patch 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.

   • 2014-09-29T12:30:30+00:00
6. 

   Roland Haas

   • removed comment

   This should be addressed one way or the other before the release. gcc 4.8 and newer will be common on Linux machines. We have a proposed patch by Tanja, however disagreement if the patch is acceptable.

   • 2014-11-04T01:08:17+00:00
7. 

   Tanja Bode reporter

   • removed comment

   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.

   • 2014-11-04T04:51:35+00:00
8. 

   Roland Haas

   • removed comment

   Does the "find" line you pasted in fix the issue for you? If so: please apply (if you have commit rights).

   • 2014-11-04T07:01:05+00:00
9. 

   Roland Haas

   • changed status to open
   • removed comment

   • 2014-11-04T07:01:59+00:00
10. 

    Roland Haas

    • changed status to open
    • removed comment

    • 2014-11-04T07:02:06+00:00
11. 

    Tanja Bode reporter

    • removed comment

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

    • 2014-11-05T04:39:46+00:00
12. 

    Roland Haas

    • changed status to resolved
    • removed comment

    Thank you. Applied in rev 18 of PAPI.

    • 2014-11-05T05:02:04+00:00
13. 

    Roland Haas

    • edited description
    • changed status to closed

    • 2019-02-21T20:01:47+00:00
14. Log in to comment