search

boludoz / lz4

Automatically exported from code.google.com/p/lz4
Other
0 stars 0 forks source link

Improvements to the Makefile #136

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
I'm looking through the Makefiles and I see a few important issues:

1. CC := $(CC) looks entirely redundant. Is there a specific reason where this 
does anything? I'm not attaching a patch since I don't want to break some 
corner-case I'm unaware of.

2. -O3 is being forced unconditionally which prevents user from specifying 
another -O option (like -Os for embedded). I'm attaching a patch which makes 
-O3 the default but respects another CFLAGS set in environment.

3. $(CPPFLAGS) and $(LDFLAGS) are not respected. I'm attaching a patch for this 
one, based on built-in make rules.

4. 'make test' by default runs both test-64 and test-32, even on plain x86. 
Which means that on x86, two identical versions will be built and tested, and 
on architectures different than x86 (like arm or mips), the -m32 option will 
result in compiler error.

5. programs/Makefile unnecessary build lz4 library code multiple times. 
Instead, it should link against the library built by top-level Makefile 
(calling 'make' to build it if necessary).

Original issue reported on code.google.com by mgo...@gentoo.org on 21 Jul 2014 at 12:35

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks for hints. Some answers, point by point :

1) Yes, this was added by a contributor, and I indeed asked the same question 
(and received no answer, see 
https://code.google.com/p/lz4/issues/detail?id=112#c4 ). I'm also wondering if 
there is a single corner case where this might end up being actually useful. I 
suspect this line does not do anything useful, and probably does not break 
anything either.

2) It's a good idea. I will probably integrate it for next release

3) Yes, it's much better this way. To be integrated.

4) You are right. Is there a solution for that ? Detecting 32-bits / 64-bits ?
The current work around is to manually select test-64 or test-32, but that's a 
bit confusing too, since test-64 could end up being 32-bits. Maybe renaming it 
test-native would help.

5) OK, that's right, but is it really worthwhile ? lz4 is a very small library, 
and cost only a few fragments of seconds to compile.

Original comment by yann.col...@gmail.com on 21 Jul 2014 at 7:12

GoogleCodeExporter commented 9 years ago 
> 4) You are right. Is there a solution for that ? Detecting 32-bits / 64-bits ?
> The current work around is to manually select test-64 or test-32, but that's 
a bit confusing too, since test-64 could end up being 32-bits. Maybe renaming 
it test-native would help.

Well, I personally believe that -m32/-m64 support doesn't belong in the build 
system. There are multiple architectures and each one comes with different 
kinds of multilib. Then there are various support which support only some of 
the all ABIs. It's something you can't ever get right without having it updated 
all the time.

IMHO it would be better to just do the native build and let the dedicated 
distribution tools handle multilib.

> 5) OK, that's right, but is it really worthwhile ? lz4 is a very small 
library, and cost only a few fragments of seconds to compile.

Hmm... thinking about it, a possible solution is to have a single Makefile for 
all the stuff. This would make this relatively simple and at the same time 
severely reduce the duplication of code.

Original comment by mgo...@gentoo.org on 21 Jul 2014 at 7:33

GoogleCodeExporter commented 9 years ago 
I've updated the "dev" branch on Github
with a new Makefile reflecting points 2, 3 and 4

https://github.com/Cyan4973/lz4/tree/dev

Tests of 32-bits mode are still possible, but no longer "default" : make test 
will now only compile and test native binaries.

Regarding point 5 : merging the 2 makefiles is more complex, and a likely 
candidate for a later release. There are 2 Makefile because there are 2 
directories.
There are 2 directories to better differentiate their license (lz4 libraries 
are BSD, the programs using them are GPL).

Original comment by yann.col...@gmail.com on 21 Jul 2014 at 8:06

GoogleCodeExporter commented 9 years ago

Original comment by yann.col...@gmail.com on 21 Jul 2014 at 8:06

GoogleCodeExporter commented 9 years ago 
Thanks, the new Makefiles work great for me.

Is github a new official source of code for lz4?

Original comment by mgo...@gentoo.org on 22 Jul 2014 at 4:54

GoogleCodeExporter commented 9 years ago 
It's an official mirror,
and all development versions are also hosted there, due to greater flexibility 
with git.

Original comment by yann.col...@gmail.com on 22 Jul 2014 at 7:30

GoogleCodeExporter commented 9 years ago 
Added into r120

Original comment by yann.col...@gmail.com on 24 Jul 2014 at 3:31