- Add -pgmreadelf option
- Auto configure path and name of "readelf"
- Add settings readelf command
- During build of GHC name of tool can be changed with ./configure READELF=myreadelf
I don't know why, but recomp011 is now failing. See https://phabricator.haskell.org/harbormaster/build/6753/?l=50:
Stderr: 993 : createProcess: runInteractiveProcess: exec: does not exist (No such file or directory) 994 make: *** [recomp011] Error 1 995 996 *** unexpected failure for recomp011(normal)
That's a good question. Since there is no access to config.log I can't figure this out.
I tried to check how can it happen, but have no idea. One of the way is that --target were provided on platform where readelf have no target-prefix. But it should fail during configuration.
It looks like after running configure in settings there is:
("readelf command", ""),
("readelf command", "/usr/host/bin/x86_64-pc-linux-gnu-readelf"),
I wish I were able to re-produce similar environment either in virtual machine or chroot or whatever.
Use AC_CHECK_TARGET_TOOL and keep only path to readelf
Looks like on test machines readProcessEnvWithExitCode "/usr/bin/readelf"
fails by some unknown reason. Let's use only name of tool.
To achive that there is no FP_-prefixed macro yet. But standard
AC_CHECK_TARGET_TOOL fulfils our needs completely.
There is no reason for adding configure argument --with-readelf since such
tools usually configured in a way ./configure READELF=myreadelf.
Note there is no good reason to compile-in full path for tools like
as/nm/ld/objdump/readelf. Moreover this is less flexible since you cannot use
icecc/distcc-like systems which inject their tools by changing PATH
(pulls hair out of his head)
Same test driver/recomp011 failed and passed in a same run according to logs:
12 ================== Testsuite summary ================== 13 14 Unexpected results from: 15 TEST="recomp011" 16 17 OVERALL SUMMARY for test run started at Sat Oct 17 03:29:52 2015 UTC 18 0:06:09 spent to go through 19 4747 total tests, which gave rise to 20 15151 test cases, of which 21 10355 were skipped 22 23 71 had missing libraries 24 4621 expected passes 25 103 expected failures 26 27 0 caused framework failures 28 0 unexpected passes 29 1 unexpected failures 30 0 unexpected stat failures 31 32 Unexpected failures: 33 driver/recomp011 recomp011 [bad exit code] (normal)
51 Beginning test run at Sat Oct 17 03:36:15 2015 UTC 52 =====> recomp011(normal) 1 of 1 [0, 0, 0] 53 cd ./driver/recomp011 && $MAKE -s --no-print-directory recomp011 </dev/null > recomp011.run.stdout 2> recomp011.run.stderr 54 55 OVERALL SUMMARY for test run started at Sat Oct 17 03:36:15 2015 UTC 56 0:00:06 spent to go through 57 1 total tests, which gave rise to 58 1 test cases, of which 59 0 were skipped 60 61 0 had missing libraries 62 1 expected passes 63 0 expected failures 64 65 0 caused framework failures 66 0 unexpected passes 67 0 unexpected failures 68 0 unexpected stat failures 69 70 make: Leaving directory `/srv/builds/patches/rGHC/B6797-D1326-4526/testsuite/tests' 71 make: Leaving directory `/srv/builds/patches/rGHC/B6797-D1326-4526/testsuite'
It looks like it runs ./validate without --no-clean flag. But from previous run I saw that settings actually had readelf command associated with value /usr/bin/readelf.
I don't think this change introduce:
992 rts/posix/Signals.c:536:1: error: 993 error: ‘backtrace_handler’ defined but not used [-Werror=unused-function] 994 backtrace_handler(int sig STG_UNUSED) 995 ^ 996 cc1: all warnings being treated as errors
Same that happens to ar/nm/ld/ranlib. They aren't used.
Were able to reproduce on my machine.
By some reason I have:
--- settings 2015-10-20 06:45:38.248921502 +0300 +++ "bindisttest/install dir/lib/ghc-7.11.20151019/settings" 2015-10-20 07:38:58.943009601 +0300 @@ -17,7 +17,7 @@ ("dllwrap command", "/bin/false"), ("windres command", "/bin/false"), ("libtool command", "libtool"), - ("readelf command", "readelf"), + ("readelf command", ""), ("perl command", "/usr/host/bin/perl"), ("cross compiling", "NO"), ("target os", "OSLinux"),
Have to figure out what exactly that bindist means and why it generated with empty value for "readelf command".
Update: there is second configure script (distrib/configure.ac.in)
Great that you found the reason it wasn't validating before. Someone else ran into this problem some time ago, but I didn't think of it this time, sorry.
Why does this now call AC_CHECK_TARGET_TOOL instead of FP_ARG_WITH_PATH_GNU_PROG (as it did in your initial version). Is that important? If so, please add a comment. Someone might be tempted to refactor this later, and perhaps introduce a regression. If it isn't important, please make the code as similar as possible to the way the other tools are found (ar/nm/ld/ranlib). Then future readers don't have to go and lookup the documentation for AC_CHECK_TARGET_TOOL to figure out why this one might be different.
No problem. Kinda quest.
That change (to autoconf alternative) was as an attempt to minimize effect of this change for already supported platforms.
Yep. And I'm the one of those who is tempting to do this. I believe we shouldn't compile/configure-in full path to binutils. We should use autoconf as much as possible if we base our build infrastructure on it.
Right now I had to go through code of FP_ARG_WITH_PATH_GNU_PROG to understand what it does before using. And I still don't understand:
This looks pretty good to me.
I must say I'm pretty sympathetic to this view. These macros have certainly added cognitive overhead in my experience.
I still think it would be better if readelf were treated in the same way as (ar/nm/ld/ranlib). Then change them all to use AC_CHECK_TARGET_TOOL later if that's better (and create a ticket for it).
If you disagree, then there should be a comment (or Note) explaining the situation.
If this needs to go in quickly, because of the ghc-7.10.3 release, that's also fine. I don't care that much either.
Someone adds me as reviewer to this, so I think I will just comment: honestly I do not like the way configuration is handled. I definitely think we should stay with what we have today with --with-<tool>=<tool path>. I see author does not agree with this but such is my feeling that it's better to use kind of common way, otherwise it will be inconvenient for GHC builders/advanced-users just to remember exactly this and this tool needs different way. Anyway, I still think that --with-<tool> also supports authors way of configuration: AR=gar NM=nm ./configure.... -- so I think this more universal...
Otherwise the patch looks OK, at least what I see in it...
In general, I would advise use of existing autoconf functionality where it fits the bill and where it doesn't implement something specific.
That said, configure.ac/aclocal,m4 hacking is finicky and we should keep changes to small and targeted, even if that means way more commits than one big cleanup.
This patch can be abandoned, because HEAD doesn't use readelf anymore. See 109d7ce85aadbd9fb7a322a0a83548e5d4e83926.
@ony: I realize this whole process, which ultimately didn't result in a commit for GHC HEAD, must have been quite discouraging. So sorry for that. I hope it doesn't stop you from finding another ticket to work on. There are lots of bugs to fix still..