search

jwrdegoede / rtl8189ES_linux

78 stars 90 forks source link

Makefile: move 'EXTRA_CFLAGS += $(USER_EXTRA_CFLAGS)' at the end of E… #83

Closed giuliobenetti closed 1 year ago

giuliobenetti commented 1 year ago

…XTRA_FLAGS assignment

At the moment USER_EXTRA_CFLAGS can't override local Makfile EXTRA_CFLAGS since it's assigned at the beginning of the Makefile. For example it's not possible to undefine the hardcoded CONFIG_LITTLE_ENDIAN and this doesn't allow to build these modules for big endian architectures. So let's move the assignment of USER_EXTRA_CFLAGS to EXTRA_CFLAGS after the last EXTRA_CFLAGS assignment.

Signed-off-by: Giulio Benetti giulio.benetti@benettiengineering.com

CGarces commented 1 year ago

Hi.

Sorry for this huge delay to review the PR.

Instead of hack the Makefile, maybe is possible to entirely remove the CONFIG_LITTLE_ENDIAN and use the linux kernel standards.

Take a look over this commit.

https://github.com/CGarces/rtl8189ES_linux/commit/9483da914368485330f0a1db2216099cae34bc67

Looks big but is just a massive replacement/remove of custom RTL code/files.

Please use this approach and force-push this PR or open a new one.

giuliobenetti commented 1 year ago

Hi @CGarces,

I think these are 2 different things. Your solution is a good idea, so in Buildroot and in general we don't need to take care of endianness. But thinking about what happens in build systems it's useful to have a Makefile variable to override previously defined CFLAGS. So you can push your patch that is already done except some fuzz(and authored by you :-)) and I can reword a bit this patch. What do you think?

CGarces commented 1 year ago

You are right, I'm disusing a different issue :D

I'm confused right now, I'm not sure if your change will break something on current builds because you are overwriting EXTRA_CFLAGS at the end, before the platform selection.

BWT, I think that I'll remove this endianness caos this weekend.

giuliobenetti commented 1 year ago

You are right, I'm disusing a different issue :D

I'm confused right now, I'm not sure if your change will break something on current builds because you are overwriting EXTRA_CFLAGS at the end, before the platform selection.

BWT, I think that I'll remove this endianness caos this weekend.

It shouldn't break builds, on the contrary should give the chance to override CFLAGS, now it's not possible. Also, I'm not overwriting EXTRA_CFLAGS but I'm adding flags to it: EXTRA_CFLAGS += $(USER_EXTRA_CFLAGS) So I think it's ok. The only problem that could arise for current users is if they pass some wrong CFLAGS to USER_EXTRA_CFLAGS that are overriden from this repo Makefile they could show up. But this should really be a problem on the user side. To be honest rarely I've seen EXTRA_CFLAGS placed in the beginning.

CGarces commented 1 year ago

To be honest rarely I've seen EXTRA_CFLAGS placed in the beginning.

Looks the standard way for RTL drivers, but I'll accept your PR.

giuliobenetti commented 1 year ago

To be honest rarely I've seen EXTRA_CFLAGS placed in the beginning.

Looks the standard way for RTL drivers, but I'll accept your PR.

I know, but other rtl drivers gave problems this way. It’s been solved by removing the default little endianness from makefile on some of them. Your solution is better :-)