Patch attached.
I have on two occasions in a short period of time run into systems where the pciutils check fails, while both packages are in fact installed. In one case, no compiler was installed. The other case has not been resolved yet, but the fact is that the check is not precise enough.
It is also a waste of time to do the check on every single make invocation.
The other hunk is about make clean. I think make clean should only remove files that were created by other targets in the Makefile. On principle I don't like make clean removing backups that someone may want to keep. (I haven't gotten bit by this yet, but I would be upset if it should happen.)
Hope you agree on these! :)
//Peter
Peter Stuge wrote:
Patch attached.
I have on two occasions in a short period of time run into systems where the pciutils check fails, while both packages are in fact installed. In one case, no compiler was installed. The other case has not been resolved yet, but the fact is that the check is not precise enough.
It is very precise. It does not check for a compiler, because having a compiler installed for compiling is obvious. Having those additional packages installed is less obvious.
It is also a waste of time to do the check on every single make invocation.
I disagree with removing this. We started including it because we had so many people not reading the readme and complaining about failed compilation.
NACK.
On Wed, Jul 02, 2008 at 02:08:18AM +0200, Stefan Reinauer wrote:
but the fact is that the check is not precise enough.
It is very precise. It does not check for a compiler, because having a compiler installed for compiling is obvious. Having those additional packages installed is less obvious.
True, but because the compiler is so obvious it is easy to overlook.
At the very least the message is not clear enough. It claims to check for this package but in fact it isn't the only requirements, and when the check says that the package wasn't found, that can be incorrect. I don't think we should have error messages that can be false like that.
It is also a waste of time to do the check on every single make invocation.
I disagree with removing this. We started including it because we had so many people not reading the readme and complaining about failed compilation.
How do you feel about a configure script? Not autoconf but a simple sh script, much like xcompile, to check for compiler and library just once and then create the Makefile, possibly adding to LDFLAGS and CFLAGS from environment variables and/or --with-cflags --with-ldflags parameters in the process.
//Peter
On Tue, Jul 1, 2008 at 9:23 PM, Peter Stuge peter@stuge.se wrote:
On Wed, Jul 02, 2008 at 02:08:18AM +0200, Stefan Reinauer wrote:
but the fact is that the check is not precise enough.
It is very precise. It does not check for a compiler, because having a compiler installed for compiling is obvious. Having those additional packages installed is less obvious.
True, but because the compiler is so obvious it is easy to overlook.
At the very least the message is not clear enough. It claims to check for this package but in fact it isn't the only requirements, and when the check says that the package wasn't found, that can be incorrect. I don't think we should have error messages that can be false like that.
I've seen this too, where I had a compiler installed but forgot to set CC or link it to gcc or some such thing. Wouldn't the more correct workaround be to do a compiler check?
-Corey
On Thu, Jul 03, 2008 at 03:16:34AM +0200, Carl-Daniel Hailfinger wrote:
How do you feel about configure?
Well, the configure scripts i've seen in the past are completely unreadable hideous blobs.
Yeah, they're generated by autoconf, from a pretty readable configure.in file.
Just to be clear, I am not advocating autoconf+automake. I don't think we need that.
Sourcing the requirement checks out to another script is OK, though, as long as that script is human readable and short.
Great, that's the plan.
However, that leads me to the question why we don't want to keep it inside the makefile.
I think Makefile should deal only with the internal dependency graph.
I haven't seen a single Makefile that tries to tie into package management before (outside ports, where they _are_ the package management, well sort-of anyway :) and for good reason I believe.
System variations are too large for this to be handled in Makefile. I think Makefile must assume ideal circumstances, and let the plumbing (compiler, linker, install, etc) report error conditions as they are encountered, lest Makefile becomes like those configure scripts.
Another example would be that it does not make much sense to first check if we have permission to write to $prefix before actually executing /usr/bin/install in the install: target.
On Thu, Jul 03, 2008 at 01:48:33AM -0400, Corey Osgood wrote:
At the very least the message is not clear enough.
I've seen this too, where I had a compiler installed but forgot to set CC or link it to gcc or some such thing. Wouldn't the more correct workaround be to do a compiler check?
Yes, that's what I'm thinking too. I am pieceing something together with parts of coresystems' xcompile script. :)
//Peter
On Thu, Jul 03, 2008 at 11:41:10AM +0200, Peter Stuge wrote:
Yeah, they're generated by autoconf, from a pretty readable configure.in file.
Just to be clear, I am not advocating autoconf+automake. I don't think we need that.
Ack.
On Thu, Jul 03, 2008 at 01:48:33AM -0400, Corey Osgood wrote:
At the very least the message is not clear enough.
I've seen this too, where I had a compiler installed but forgot to set CC or link it to gcc or some such thing. Wouldn't the more correct workaround be to do a compiler check?
Yes, that's what I'm thinking too. I am pieceing something together with parts of coresystems' xcompile script. :)
Uh, I'd say this is overkill. Personally I prefer keeping the check in the Makefile (if at all). An extra script might make sense if there are _lots_ of _verbose_ and complicated tests, but we're far away from that.
Adding yet another file just doesn't make much sense and is not really useful either, IMO.
Uwe.
On Thu, Jul 03, 2008 at 07:15:39PM +0200, Uwe Hermann wrote:
I am pieceing something together with parts of coresystems' xcompile script. :)
Uh, I'd say this is overkill.
Nothing was left of xcompile when I took out the crosscompiling detection, which isn't needed for flashrom. Makes sense.
Personally I prefer keeping the check in the Makefile (if at all).
I would love to simply remove the check. Then flashrom will be like every other package I've seen.
The error message from compile or link commands (as run by make) is IMHO far better than anything we can pretend to produce on our own.
If there is a library missing, the linker will make that clear.
Can we convince Stefan? :)
//Peter
Peter Stuge wrote:
Personally I prefer keeping the check in the Makefile (if at all).
I would love to simply remove the check. Then flashrom will be like every other package I've seen.
The error message from compile or link commands (as run by make) is IMHO far better than anything we can pretend to produce on our own.
If there is a library missing, the linker will make that clear.
Can we convince Stefan? :)
No, I disagree. Go check the mailing list archive for error reports that lead to that message.
People started complaining because /usr/include/linux/pci.h was there and it still would not work.
I don't see what's wrong with the check.
Stefan
On 02.07.2008 00:18, Peter Stuge wrote:
Patch attached.
I have on two occasions in a short period of time run into systems where the pciutils check fails, while both packages are in fact installed. In one case, no compiler was installed. The other case has not been resolved yet, but the fact is that the check is not precise enough.
It is also a waste of time to do the check on every single make invocation.
Stefan alread commented on this.
The other hunk is about make clean. I think make clean should only remove files that were created by other targets in the Makefile. On principle I don't like make clean removing backups that someone may want to keep. (I haven't gotten bit by this yet, but I would be upset if it should happen.)
Hope you agree on these! :)
Fully agreed on the non-removal of backup files.
flashrom: Remove Makefile pciutils check, and don't rm *~ in clean
Signed-off-by: Peter Stuge peter@stuge.se
The "don't rm *~ in clean" part is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Regards, Carl-Daniel
On Wed, Jul 02, 2008 at 04:40:34AM +0200, Carl-Daniel Hailfinger wrote:
It is also a waste of time to do the check on every single make invocation.
Stefan alread commented on this.
How do you feel about configure?
flashrom: Remove Makefile pciutils check, and don't rm *~ in clean
Signed-off-by: Peter Stuge peter@stuge.se
The "don't rm *~ in clean" part is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Thanks! r3404
//Peter
On Wed, Jul 02, 2008 at 05:05:00AM +0200, Peter Stuge wrote:
flashrom: Remove Makefile pciutils check, and don't rm *~ in clean
Signed-off-by: Peter Stuge peter@stuge.se
The "don't rm *~ in clean" part is Acked-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Thanks! r3404
Correction, 3405.
On 02.07.2008 05:05, Peter Stuge wrote:
On Wed, Jul 02, 2008 at 04:40:34AM +0200, Carl-Daniel Hailfinger wrote:
It is also a waste of time to do the check on every single make invocation.
Stefan alread commented on this.
How do you feel about configure?
Well, the configure scripts i've seen in the past are completely unreadable hideous blobs. Sourcing the requirement checks out to another script is OK, though, as long as that script is human readable and short. However, that leads me to the question why we don't want to keep it inside the makefile.
Thoughts?
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Corey Osgood -
Peter Stuge -
Stefan Reinauer -
Uwe Hermann