Hello All! In order to simplify the mainteinance of packages for these two utilities in Fedora/RHEL we need to be able to redefine several makefile variables such as CFLAGS and PREFIX. There are two ways to do so:
* first (ugly and a bit annoying) is to patch Makefile each time it changes by mainstream developers. * second - we may patch Makefile one time to allow conditional assignments for these variables and submit it to upstream (as I'm doing right now).
Conditional assignment means that instead of defininv variables in the folowing way:
PREFIX = /usr/local
we must define it as following:
PREFIX ?= /usr/local
That means "if prefix was not defined somethere earlier we assign it to /usr/local".
Another one issue is simplification of "install" section in Makefile. Installation preserves timestamps, creates necessary directories and ensures that man-page will not have the executable permission.
Signed-off-by: Peter Lemenkov lemenkov@gmail.com ---
Two simple patches attached. They also available at http://peter.fedorapeople.org/LinuxBIOS/
Hi Peter,
On 18.05.2008 11:40, Peter Lemenkov wrote:
Hello All! In order to simplify the mainteinance of packages for these two utilities in Fedora/RHEL we need to be able to redefine several makefile variables such as CFLAGS and PREFIX. There are two ways to do so:
I can understand setting PREFIX, but the reason why you need to redefine CFLAGS is a mystery to me.
- first (ugly and a bit annoying) is to patch Makefile each time it
changes by mainstream developers.
- second - we may patch Makefile one time to allow conditional
assignments for these variables and submit it to upstream (as I'm doing right now).
Sorry, parts of both Makefile patches are obviously wrong and even actively harmful, so the patch set is NACKed for now.
Conditional assignment means that instead of defininv variables in the folowing way:
PREFIX = /usr/local
we must define it as following:
PREFIX ?= /usr/local
That means "if prefix was not defined somethere earlier we assign it to /usr/local".
Understood.
Another one issue is simplification of "install" section in Makefile. Installation preserves timestamps, creates necessary directories and ensures that man-page will not have the executable permission.
Will the modified install section work with a non-FSF install program as well, i.e. is it truly cross-platform?
Regards, Carl-Daniel
Hello All!
2008/5/18 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net:
I can understand setting PREFIX, but the reason why you need to redefine CFLAGS is a mystery to me.
We use our custom CFLAGS.
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables
See here:
http://buildsys.fedoraproject.org/logs/fedora-5-epel/39019-flashrom-0-0.9.20...
Sorry, parts of both Makefile patches are obviously wrong and even actively harmful
What parts?
Will the modified install section work with a non-FSF install program as well, i.e. is it truly cross-platform?
No idea.
On 18.05.2008 14:52, Peter Lemenkov wrote:
2008/5/18 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net:
I can understand setting PREFIX, but the reason why you need to redefine CFLAGS is a mystery to me.
We use our custom CFLAGS.
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables
And these custom CFLAGS are missing -DDISABLE_DOC in the flashrom case (harmful). Furthermore, they are missing $(SVNDEF) for superiotool (wrong, but only a nuisance).
See here:
http://buildsys.fedoraproject.org/logs/fedora-5-epel/39019-flashrom-0-0.9.20...
The build log shows CFLAGS differing from the custom CFLAGS you listed above.
Sorry, parts of both Makefile patches are obviously wrong and even actively harmful
What parts?
The missing CFLAGS mentioned above.
Will the modified install section work with a non-FSF install program as well, i.e. is it truly cross-platform?
No idea.
Sorry, then that part is a no-go. We are currently preparing flashrom for FreeBSD compatibility and I don't want to ruin that.
Regards, Carl-Daniel
2008/5/18 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net:
I can understand setting PREFIX, but the reason why you need to redefine CFLAGS is a mystery to me.
We use our custom CFLAGS.
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables
And these custom CFLAGS are missing -DDISABLE_DOC in the flashrom case (harmful). Furthermore, they are missing $(SVNDEF) for superiotool (wrong, but only a nuisance).
No.
CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -D'''FLASHROM_VERSION="3332"''' -DDISABLE_DOC'
We provide them in form of CFLAGC=%some_predefined_part% -D'''FLASHROM_VERSION="3332"''' -DDISABLE_DOC'
So we do define FLASHROM_VERSION and DISABLE_DOC.
But, as I mentioned above, the best way to do so is to define then in a conditional way.
Will the modified install section work with a non-FSF install program as well, i.e. is it truly cross-platform?
No idea.
Sorry, then that part is a no-go. We are currently preparing flashrom for FreeBSD compatibility and I don't want to ruin that.
OK, understood and agree with this.
Apart from install-section other things (trick with conditional defines) are OK?
On 18.05.2008 15:21, Peter Lemenkov wrote:
2008/5/18 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net:
I can understand setting PREFIX, but the reason why you need to redefine CFLAGS is a mystery to me.
We use our custom CFLAGS.
-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables
And these custom CFLAGS are missing -DDISABLE_DOC in the flashrom case (harmful). Furthermore, they are missing $(SVNDEF) for superiotool (wrong, but only a nuisance).
No.
CFLAGS='-O2 -g -pipe -Wall -Wp,-D_FORTIFY_SOURCE=2 -fexceptions -fstack-protector --param=ssp-buffer-size=4 -m32 -march=i386 -mtune=generic -fasynchronous-unwind-tables -D'''FLASHROM_VERSION="3332"''' -DDISABLE_DOC'
We provide them in form of CFLAGC=%some_predefined_part% -D'''FLASHROM_VERSION="3332"''' -DDISABLE_DOC'
So we do define FLASHROM_VERSION and DISABLE_DOC.
OK, you may do that, but what happens if someone defines CFLAGS='-O2' and leaves it at that? With the patch you proposed, -DDISABLE_DOC would be missing then.
But, as I mentioned above, the best way to do so is to define then in a conditional way.
-DDISABLE_DOC must be specified regardless of the CFLAGS passed in. I'm sure there are ways to do that. Having $SVNDEF in the unconditional CFLAGS is desirable as well. You could make SVNDEF a conditional define to solve problems with that.
Will the modified install section work with a non-FSF install program as well, i.e. is it truly cross-platform?
No idea.
Sorry, then that part is a no-go. We are currently preparing flashrom for FreeBSD compatibility and I don't want to ruin that.
OK, understood and agree with this.
Apart from install-section other things (trick with conditional defines) are OK?
If you can solve the conditional define problem as outlined above, I think the patch can be Acked. Makefile stuff is very brittle and we had several accidents in the past, so getting 2 Acks is a minimum.
Regards, Carl-Daniel
2008/5/18 Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net:
OK, you may do that, but what happens if someone defines CFLAGS='-O2' and leaves it at that? With the patch you proposed, -DDISABLE_DOC would be missing then.
Understood. I'll review this patch.
If you can solve the conditional define problem as outlined above, I think the patch can be Acked. Makefile stuff is very brittle and we had several accidents in the past, so getting 2 Acks is a minimum.
Ok.
-
Carl-Daniel Hailfinger -
Peter Lemenkov