Add a couple of rules to (1) download (via GIT) John's working copy of regulatory.bin and (2) install it in the appropriate target destination. This avoids us having to parse the Makefile for REG_BIN and REG_GIT from an enclosing (nesting) makefile, as is common in distros and buildroot environments in particular.
--- crda-1.1.0/Makefile 2009-08-10 13:37:36.000000000 -0700
+++ crda-1.1.0/Makefile.new 2009-08-10 13:37:11.000000000 -0700
@@ -136,6 +164,14 @@
$(NQ) ' INSTALL regdbdump.8.gz'
$(Q)$(INSTALL) -m 644 -t $(DESTDIR)/$(MANDIR)/man8/ regdbdump.8.gz
+install-rb: wireless-regdb/regulatory.bin
+ $(NQ) ' INSTALL regulatory.bin'
+ $(Q)$(INSTALL) -m 444 -D wireless-regdb/regulatory.bin $(DESTDIR)/$(REG_BIN)
+
+wireless-regdb/regulatory.bin:
+ @rm -rf wireless-regdb
+ git clone -q $(REG_GIT) wireless-regdb
+
clean:
$(Q)rm -f crda regdbdump intersect *.o *~ *.pyc keys-*.c *.gz \
udev/$(UDEV_LEVEL)regulatory.rules udev/regulatory.rules.parsed
Pavel Roskin wrote:
> On Mon, 2009-08-10 at 16:55 -0700, Philip A. Prindeville wrote:
>
>> You're right: that comment was much more helpful... does it apply to the first file or the second or both?
>>
>> And what in particular is a mess?
>
> Cross compilation is not easy. That's why there are such
> "metadistros" (for the lack of a better word) as buildroot and
> openembedded. They have special entries for every package that specify
> how to cross-compile it. There are patches for many sources, although
> it's better to have such files applied to the upstream sources. But
> it's inevitable that the build is influenced in some way to
> cross-compile, often by specifying variables on the make command line.
>
> I believe the developers of buildroot and openembedded would be able to
> deal with CRDA as is. If they find something that could be improved,
> they can send a patch, but I don't think they will bother to change so
> many things as your patch does.
>
> Besides, it's one thing to follow sane rules that simplify
> cross-compilation, such as providing the fourth argument to
> AC_RUN_IFELSE in configure.ac or not using uname to determine the target
> architecture. It's another thing to support cross-compilation in a way
> unique to the package. The gain is miniscule, and the potential for
> breaking is substantial.
>
> Most importantly, you are wasting time of people who could be doing
> something they are better at, such as development of wireless drivers.
>
> There is no point in pushing the same patch over and over again, just
> because you wrote it. Please try to accept the fact that it's not
> useful for others. Maybe it was useful for you as an exercise. But now
> you are not helping. Please move on and do something else.
Pavel,
Consider the code in the form of the latest submission. The conditional-compilation path *is* necessary.
Here's why.
Consider that at some point someone decides to spruce up the functionality of CRDA, perhaps by adding internationalization/NLS (just to pick an example)... and adds code for GETTEXT or some other library.
Then they decide that it isn't optional, it's a requirement.
Currently to make things build, I'd have to call:
make -C crda USE_OPENSSL=1 NL1FOUND=Y NLLIBNAME=libnl-1 NLLIBS=-lnl-1 ...
just to get it to compile successfully.
Then someone has the brilliant idea above (of adding new functionality, and making it mandatory), and adds a corresponding bit of code like:
ifeq ($(NLLIBNAME),)
$(error Cannot find development files for any supported version of libnl)
endif
to do the equivalent functionality. Bingo. The cross-compilation would break, and I'd have to come back and stare at the makefile and add to an ever increasing list of variables that had to be passed in.
But that's not even the worst of it.
NL1FOUND is an internal variable! It's used as a temporary for some transient state in the makefile... It shouldn't even be externally visible, ditto for NLLIBNAME. Yet if I don't pass this variable in, the make dies with the above $(error ...) message.
And you're telling me that my patch is a glory mess?
Not hardly.
At least my proposed fix compartmentalizes the auto-configuration bits from the list of external state that needs to be passed in, so it's quickly apparent which is which.
-Philip
Luis R. Rodriguez wrote:
> On Mon, Aug 10, 2009 at 3:13 PM, Philip A.
> Prindeville<[email protected]> wrote:
>> Pavel Roskin wrote:
>>> On Fri, 2009-08-07 at 17:09 -0700, Philip A. Prindeville wrote:
>>>> Luis R. Rodriguez wrote:
>>>>> Please send white space changes first in one patch, ie, that do not
>>>>> change anything other than that. Then send the other stuff.
>>>>>
>>>>> Luis
>>>> Here it is.
>>> ...
>>>> -else
>>>> + else
>>> ...
>>>> +install-rb: wireless-regdb/regulatory.bin
>>> If you don't understand what you were asked to do, I'll appreciate if
>>> you stop wasting everybody's time.
>>>
>>> Nobody is going to extract useful bits from the patches you send,
>>> especially if you don't provide an adequate description of the changes.
>>>
>>> I was able to cross-compile CDRA for MIPS without any changes other than
>>> those that have already been applied.
>> Since we compile a variety of images that are both Linux and i586 based as are the target and host, detecting instances amongst the 180+ packages contained in our distribution where the build used host parameters instead of target parameters can be painful and tedious.
>>
>> Sometimes it's just best to isolate such code paths with a safe, obvious gating variable. That's what I've tried to do here.
>>
>> We don't just build images, we also make the entire SVN tree available to others to build in their environments as well (Fedora, Centos, Gentoo, Debian, x86, PPC, x86_64, etc). We can't possibly test for every scenario our users will encounter, so we make things as safe as we know how.
>>
>> Yes, I'm sure Pavel *was* able to build for his MIPS target in his environment. That's a bet I can't make for all of our users, however, without these changes.
>>
>> I've split our changes into two functional groups:
>>
>> * allowing a cross-compilation without contamination by the host's own state via pkg-config bleeding into it.
>>
>> * allowing regulatory.bin to be installed via an external Makefile without having to extract internal values from crda/Makefile (i.e. REG_BIN and REG_GIT).
>>
>> These patches assume that Pavel's own "noverify" patch has already been applied.
>>
>> Changes:
>>
>> 1. Remove trailing slashes from directory variables: they're redundant and cause duplicates if they are appended to a path that's already absolute.
>>
>> 2. Move invariant definitions to before any cross-compilation conditional sections for clarity (and to avoid having to duplicate them).
>>
>> 3. Bracket any host (native) compilation sections with conditional based on $(CROSS_COMPILE) being empty.
>>
>> 4. Add 'else' section for cross-compilation using openssl and libnl-1 (or libnl-2 if explicitly selected).
>>
>>
>> --- crda-1.1.0/Makefile 2009-08-10 13:37:36.000000000 -0700
>> +++ crda-1.1.0/Makefile.new 2009-08-10 13:37:11.000000000 -0700
>> @@ -3,10 +3,10 @@
>> REG_BIN?=/usr/lib/crda/regulatory.bin
>> REG_GIT?=git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-regdb.git
>>
>> -PREFIX ?= /usr/
>> -MANDIR ?= $(PREFIX)/share/man/
>> +PREFIX ?= /usr
>> +MANDIR ?= $(PREFIX)/share/man
>>
>> -SBINDIR ?= /sbin/
>> +SBINDIR ?= /sbin
>>
>> # Use a custom CRDA_UDEV_LEVEL when callling make install to
>> # change your desired level for the udev regulatory.rules
>> @@ -14,7 +14,7 @@
>> UDEV_LEVEL=$(CRDA_UDEV_LEVEL)-
>> # You can customize this if your distributions uses
>> # a different location.
>> -UDEV_RULE_DIR?=/lib/udev/rules.d/
>> +UDEV_RULE_DIR?=/lib/udev/rules.d
>>
>> # If your distribution requires a custom pubkeys dir
>> # you must update this variable to reflect where the
>> @@ -22,48 +22,76 @@
>> # with make PUBKEY_DIR=/usr/lib/crda/pubkeys
>> PUBKEY_DIR?=pubkeys
>>
>> +MKDIR ?= mkdir -p
>> +INSTALL ?= install
>> +
>> CFLAGS += -Wall -g
>>
>> all: all_noverify verify
>>
>> all_noverify: crda intersect regdbdump
>>
>> -ifeq ($(USE_OPENSSL),1)
>> +ifeq ($(CROSS_COMPILE),)
>> +
>> + ifeq ($(USE_OPENSSL),1)
>> CFLAGS += -DUSE_OPENSSL `pkg-config --cflags openssl`
>> LDLIBS += `pkg-config --libs openssl`
>>
>> reglib.o: keys-ssl.c
>>
>> -else
>> + else
>> CFLAGS += -DUSE_GCRYPT
>> LDLIBS += -lgcrypt
>>
>> reglib.o: keys-gcrypt.c
>>
>> -endif
>> -MKDIR ?= mkdir -p
>> -INSTALL ?= install
>> + endif
>>
>> NL1FOUND := $(shell pkg-config --atleast-version=1 libnl-1 && echo Y)
>> NL2FOUND := $(shell pkg-config --atleast-version=2 libnl-2.0 && echo Y)
>>
>> -ifeq ($(NL1FOUND),Y)
>> + ifeq ($(NL1FOUND),Y)
>> NLLIBNAME = libnl-1
>> -endif
>> + endif
>>
>> -ifeq ($(NL2FOUND),Y)
>> + ifeq ($(NL2FOUND),Y)
>> CFLAGS += -DCONFIG_LIBNL20
>> NLLIBS += -lnl-genl
>> NLLIBNAME = libnl-2.0
>> -endif
>> + endif
>>
>> -ifeq ($(NLLIBNAME),)
>> + ifeq ($(NLLIBNAME),)
>> $(error Cannot find development files for any supported version of libnl)
>> -endif
>> + endif
>>
>> NLLIBS += `pkg-config --libs $(NLLIBNAME)`
>> CFLAGS += `pkg-config --cflags $(NLLIBNAME)`
>>
>> +else
>> +
>> + ifeq ($(USE_OPENSSL),1)
>> +CFLAGS += -DUSE_OPENSSL
>> +LDLIBS += -lssl
>> +
>> +reglib.o: keys-ssl.c
>> +
>> + else
>> +CFLAGS += -DUSE_GCRYPT
>> +LDLIBS += -lgcrypt
>> +
>> +reglib.o: keys-gcrypt.c
>> +
>> + endif
>> +
>> + ifeq ($(USE_LIBNL20),1)
>> +CFLAGS += -DCONFIG_LIBNL20
>> +NLLIBS = -lnl-genl -lnl-2.0
>> + else
>> +NLLIBS = -lnl
>> + endif
>> +
>> +endif
>> +
>> ifeq ($(V),1)
>> Q=
>> NQ=@true
>>
>>
>> Add a couple of rules to (1) download (via GIT) John's working copy of regulatory.bin and (2) install it in the appropriate target destination. This avoids us having to parse the Makefile for REG_BIN and REG_GIT from an enclosing (nesting) makefile, as is common in distros and buildroot environments in particular.
>>
>> --- crda-1.1.0/Makefile 2009-08-10 13:37:36.000000000 -0700
>> +++ crda-1.1.0/Makefile.new 2009-08-10 13:37:11.000000000 -0700
>> @@ -136,6 +164,14 @@
>> $(NQ) ' INSTALL regdbdump.8.gz'
>> $(Q)$(INSTALL) -m 644 -t $(DESTDIR)/$(MANDIR)/man8/ regdbdump.8.gz
>>
>> +install-rb: wireless-regdb/regulatory.bin
>> + $(NQ) ' INSTALL regulatory.bin'
>> + $(Q)$(INSTALL) -m 444 -D wireless-regdb/regulatory.bin $(DESTDIR)/$(REG_BIN)
>> +
>> +wireless-regdb/regulatory.bin:
>> + @rm -rf wireless-regdb
>> + git clone -q $(REG_GIT) wireless-regdb
>> +
>> clean:
>> $(Q)rm -f crda regdbdump intersect *.o *~ *.pyc keys-*.c *.gz \
>> udev/$(UDEV_LEVEL)regulatory.rules udev/regulatory.rules.parsed
>
> This is a glory mess, please stop trying.
>
> Luis
You're right: that comment was much more helpful... does it apply to the first file or the second or both?
And what in particular is a mess?
-Philip
Philip A. Prindeville wrote:
> * allowing a cross-compilation without contamination by the host's own state via pkg-config bleeding into it.
Setting PKG_CONFIG_LIBDIR should fix that without any makefile changes.
I haven't tried with this package, but it works for every other
cross compilation I've done.
On Mon, 2009-08-10 at 16:55 -0700, Philip A. Prindeville wrote:
> You're right: that comment was much more helpful... does it apply to the first file or the second or both?
>
> And what in particular is a mess?
Cross compilation is not easy. That's why there are such
"metadistros" (for the lack of a better word) as buildroot and
openembedded. They have special entries for every package that specify
how to cross-compile it. There are patches for many sources, although
it's better to have such files applied to the upstream sources. But
it's inevitable that the build is influenced in some way to
cross-compile, often by specifying variables on the make command line.
I believe the developers of buildroot and openembedded would be able to
deal with CRDA as is. If they find something that could be improved,
they can send a patch, but I don't think they will bother to change so
many things as your patch does.
Besides, it's one thing to follow sane rules that simplify
cross-compilation, such as providing the fourth argument to
AC_RUN_IFELSE in configure.ac or not using uname to determine the target
architecture. It's another thing to support cross-compilation in a way
unique to the package. The gain is miniscule, and the potential for
breaking is substantial.
Most importantly, you are wasting time of people who could be doing
something they are better at, such as development of wireless drivers.
There is no point in pushing the same patch over and over again, just
because you wrote it. Please try to accept the fact that it's not
useful for others. Maybe it was useful for you as an exercise. But now
you are not helping. Please move on and do something else.
--
Regards,
Pavel Roskin
On Mon, Aug 10, 2009 at 3:13 PM, Philip A.
Prindeville<[email protected]> wrote:
> Pavel Roskin wrote:
>> On Fri, 2009-08-07 at 17:09 -0700, Philip A. Prindeville wrote:
>>> Luis R. Rodriguez wrote:
>>>> Please send white space changes first in one patch, ie, that do not
>>>> change anything other than that. Then send the other stuff.
>>>>
>>>> Luis
>>> Here it is.
>> ...
>>> -else
>>> + else
>> ...
>>> +install-rb: wireless-regdb/regulatory.bin
>>
>> If you don't understand what you were asked to do, I'll appreciate if
>> you stop wasting everybody's time.
>>
>> Nobody is going to extract useful bits from the patches you send,
>> especially if you don't provide an adequate description of the changes.
>>
>> I was able to cross-compile CDRA for MIPS without any changes other than
>> those that have already been applied.
>
> Since we compile a variety of images that are both Linux and i586 based as are the target and host, detecting instances amongst the 180+ packages contained in our distribution where the build used host parameters instead of target parameters can be painful and tedious.
>
> Sometimes it's just best to isolate such code paths with a safe, obvious gating variable. That's what I've tried to do here.
>
> We don't just build images, we also make the entire SVN tree available to others to build in their environments as well (Fedora, Centos, Gentoo, Debian, x86, PPC, x86_64, etc). We can't possibly test for every scenario our users will encounter, so we make things as safe as we know how.
>
> Yes, I'm sure Pavel *was* able to build for his MIPS target in his environment. That's a bet I can't make for all of our users, however, without these changes.
>
> I've split our changes into two functional groups:
>
> * allowing a cross-compilation without contamination by the host's own state via pkg-config bleeding into it.
>
> * allowing regulatory.bin to be installed via an external Makefile without having to extract internal values from crda/Makefile (i.e. REG_BIN and REG_GIT).
>
> These patches assume that Pavel's own "noverify" patch has already been applied.
>
> Changes:
>
> 1. Remove trailing slashes from directory variables: they're redundant and cause duplicates if they are appended to a path that's already absolute.
>
> 2. Move invariant definitions to before any cross-compilation conditional sections for clarity (and to avoid having to duplicate them).
>
> 3. Bracket any host (native) compilation sections with conditional based on $(CROSS_COMPILE) being empty.
>
> 4. Add 'else' section for cross-compilation using openssl and libnl-1 (or libnl-2 if explicitly selected).
>
>
> --- crda-1.1.0/Makefile 2009-08-10 13:37:36.000000000 -0700
> +++ crda-1.1.0/Makefile.new 2009-08-10 13:37:11.000000000 -0700
> @@ -3,10 +3,10 @@
> REG_BIN?=/usr/lib/crda/regulatory.bin
> REG_GIT?=git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless-regdb.git
>
> -PREFIX ?= /usr/
> -MANDIR ?= $(PREFIX)/share/man/
> +PREFIX ?= /usr
> +MANDIR ?= $(PREFIX)/share/man
>
> -SBINDIR ?= /sbin/
> +SBINDIR ?= /sbin
>
> # Use a custom CRDA_UDEV_LEVEL when callling make install to
> # change your desired level for the udev regulatory.rules
> @@ -14,7 +14,7 @@
> UDEV_LEVEL=$(CRDA_UDEV_LEVEL)-
> # You can customize this if your distributions uses
> # a different location.
> -UDEV_RULE_DIR?=/lib/udev/rules.d/
> +UDEV_RULE_DIR?=/lib/udev/rules.d
>
> # If your distribution requires a custom pubkeys dir
> # you must update this variable to reflect where the
> @@ -22,48 +22,76 @@
> # with make PUBKEY_DIR=/usr/lib/crda/pubkeys
> PUBKEY_DIR?=pubkeys
>
> +MKDIR ?= mkdir -p
> +INSTALL ?= install
> +
> CFLAGS += -Wall -g
>
> all: all_noverify verify
>
> all_noverify: crda intersect regdbdump
>
> -ifeq ($(USE_OPENSSL),1)
> +ifeq ($(CROSS_COMPILE),)
> +
> + ifeq ($(USE_OPENSSL),1)
> CFLAGS += -DUSE_OPENSSL `pkg-config --cflags openssl`
> LDLIBS += `pkg-config --libs openssl`
>
> reglib.o: keys-ssl.c
>
> -else
> + else
> CFLAGS += -DUSE_GCRYPT
> LDLIBS += -lgcrypt
>
> reglib.o: keys-gcrypt.c
>
> -endif
> -MKDIR ?= mkdir -p
> -INSTALL ?= install
> + endif
>
> NL1FOUND := $(shell pkg-config --atleast-version=1 libnl-1 && echo Y)
> NL2FOUND := $(shell pkg-config --atleast-version=2 libnl-2.0 && echo Y)
>
> -ifeq ($(NL1FOUND),Y)
> + ifeq ($(NL1FOUND),Y)
> NLLIBNAME = libnl-1
> -endif
> + endif
>
> -ifeq ($(NL2FOUND),Y)
> + ifeq ($(NL2FOUND),Y)
> CFLAGS += -DCONFIG_LIBNL20
> NLLIBS += -lnl-genl
> NLLIBNAME = libnl-2.0
> -endif
> + endif
>
> -ifeq ($(NLLIBNAME),)
> + ifeq ($(NLLIBNAME),)
> $(error Cannot find development files for any supported version of libnl)
> -endif
> + endif
>
> NLLIBS += `pkg-config --libs $(NLLIBNAME)`
> CFLAGS += `pkg-config --cflags $(NLLIBNAME)`
>
> +else
> +
> + ifeq ($(USE_OPENSSL),1)
> +CFLAGS += -DUSE_OPENSSL
> +LDLIBS += -lssl
> +
> +reglib.o: keys-ssl.c
> +
> + else
> +CFLAGS += -DUSE_GCRYPT
> +LDLIBS += -lgcrypt
> +
> +reglib.o: keys-gcrypt.c
> +
> + endif
> +
> + ifeq ($(USE_LIBNL20),1)
> +CFLAGS += -DCONFIG_LIBNL20
> +NLLIBS = -lnl-genl -lnl-2.0
> + else
> +NLLIBS = -lnl
> + endif
> +
> +endif
> +
> ifeq ($(V),1)
> Q=
> NQ=@true
>
>
> Add a couple of rules to (1) download (via GIT) John's working copy of regulatory.bin and (2) install it in the appropriate target destination. This avoids us having to parse the Makefile for REG_BIN and REG_GIT from an enclosing (nesting) makefile, as is common in distros and buildroot environments in particular.
>
> --- crda-1.1.0/Makefile 2009-08-10 13:37:36.000000000 -0700
> +++ crda-1.1.0/Makefile.new 2009-08-10 13:37:11.000000000 -0700
> @@ -136,6 +164,14 @@
> $(NQ) ' INSTALL regdbdump.8.gz'
> $(Q)$(INSTALL) -m 644 -t $(DESTDIR)/$(MANDIR)/man8/ regdbdump.8.gz
>
> +install-rb: wireless-regdb/regulatory.bin
> + $(NQ) ' INSTALL regulatory.bin'
> + $(Q)$(INSTALL) -m 444 -D wireless-regdb/regulatory.bin $(DESTDIR)/$(REG_BIN)
> +
> +wireless-regdb/regulatory.bin:
> + @rm -rf wireless-regdb
> + git clone -q $(REG_GIT) wireless-regdb
> +
> clean:
> $(Q)rm -f crda regdbdump intersect *.o *~ *.pyc keys-*.c *.gz \
> udev/$(UDEV_LEVEL)regulatory.rules udev/regulatory.rules.parsed
This is a glory mess, please stop trying.
Luis
Philip Craig wrote:
> Philip A. Prindeville wrote:
>> * allowing a cross-compilation without contamination by the host's own state via pkg-config bleeding into it.
>
> Setting PKG_CONFIG_LIBDIR should fix that without any makefile changes.
> I haven't tried with this package, but it works for every other
> cross compilation I've done.
Indeed, and that would work until, as Pavel points out, someone adds something naive and ill-informed like a `uname -r` (or `arch`) into the Makefile.
I'm not just trying to make the current version safe for cross-compilation, but all future versions too so I don't ever have to come back and fix this issue again.
You have no idea how many packages we had to make cross-compilation safe. Literally dozens. And some of them continue to have periodic regressions.
Changes:
1. Move invariant definitions to before any cross-compilation conditional sections for clarity (and to avoid having to duplicate them).
2. Bracket any host (native) compilation sections with conditional based on $(CROSS_COMPILE) being empty.
4. Add 'else' section for cross-compilation using openssl and libnl-1 (or libnl-2 if explicitly selected).
--- crda-1.1.0/Makefile 2009-08-10 13:37:36.000000000 -0700
+++ crda-1.1.0/Makefile.new 2009-08-10 17:29:22.000000000 -0700
@@ -22,12 +22,17 @@
# with make PUBKEY_DIR=/usr/lib/crda/pubkeys
PUBKEY_DIR?=pubkeys
+MKDIR ?= mkdir -p
+INSTALL ?= install
+
CFLAGS += -Wall -g
all: all_noverify verify
all_noverify: crda intersect regdbdump
+ifeq ($(CROSS_COMPILE),)
+
ifeq ($(USE_OPENSSL),1)
CFLAGS += -DUSE_OPENSSL `pkg-config --cflags openssl`
LDLIBS += `pkg-config --libs openssl`
@@ -41,8 +46,6 @@
reglib.o: keys-gcrypt.c
endif
-MKDIR ?= mkdir -p
-INSTALL ?= install
NL1FOUND := $(shell pkg-config --atleast-version=1 libnl-1 && echo Y)
NL2FOUND := $(shell pkg-config --atleast-version=2 libnl-2.0 && echo Y)
@@ -64,6 +67,31 @@
NLLIBS += `pkg-config --libs $(NLLIBNAME)`
CFLAGS += `pkg-config --cflags $(NLLIBNAME)`
+else
+
+ifeq ($(USE_OPENSSL),1)
+CFLAGS += -DUSE_OPENSSL
+LDLIBS += -lssl
+
+reglib.o: keys-ssl.c
+
+else
+CFLAGS += -DUSE_GCRYPT
+LDLIBS += -lgcrypt
+
+reglib.o: keys-gcrypt.c
+
+endif
+
+ifeq ($(USE_LIBNL20),1)
+CFLAGS += -DCONFIG_LIBNL20
+NLLIBS = -lnl-genl -lnl-2.0
+else
+NLLIBS = -lnl
+endif
+
+endif
+
ifeq ($(V),1)
Q=
NQ=@true