search

boludoz / lz4

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

OS detection in Makefile could be more platform-agnostic #86

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Currently, the Makefile checks to see if we are on Linux, and appends .exe if 
not.

It seems simpler to do this:

ifeq ($(OS),Windows*)
EXT =.exe
else
EXT =
endif

This should correctly target operating systems like Solaris, Illumos, the BSDs, 
AIX, HP-UX, etc.

Original issue reported on code.google.com by bl...@wanelo.com on 23 Sep 2013 at 9:50

GoogleCodeExporter commented 9 years ago 
Sounds fair.

Are we sure that $(OS) == Windows*
for all versions of Windows ?

Original comment by yann.col...@gmail.com on 23 Sep 2013 at 11:13

GoogleCodeExporter commented 9 years ago 
mmmh, just made a test, and I got :
$(OS) == MINGW32_NT-6.1

So Windows* doesn't work "as is".

Original comment by yann.col...@gmail.com on 23 Sep 2013 at 11:24

GoogleCodeExporter commented 9 years ago 
OK, checked some docs, which recommended to use $(OS) for Windows, and uname 
for *nix.

And the r104 Makefile, both are blended together, due to this line :
OS := $(shell uname)

Removing/commenting this line, now we have
$(OS) == Windows_NT
which is, I guess, the expected behavior.

(Note : the test is done on Windows 7 64-bits)

So, back to initial question :
what are the expected values of $(OS) for Windows ?
It seems Windows_NT is going to be the most common one, what about the others ?

Original comment by yann.col...@gmail.com on 23 Sep 2013 at 11:31

GoogleCodeExporter commented 9 years ago 
The proposed test :
ifeq ($(OS),Windows*)

doesn't seem to work. It seems '*' wildcard support is not part of ifeq syntax.

I had to change the test to the following syntax (to keep the same test) :
ifneq (,$(filter Windows%,$(OS)))

This one works as intended.

Alternatively, it could have been possible to restrict the number of Windows OS 
detected :
ifeq ($(OS),Windows_NT)

Original comment by yann.col...@gmail.com on 23 Sep 2013 at 11:46

GoogleCodeExporter commented 9 years ago

Original comment by yann.col...@gmail.com on 23 Sep 2013 at 11:55

GoogleCodeExporter commented 9 years ago 
The following is a potential release candidate which includes your requirement

Original comment by yann.col...@gmail.com on 24 Sep 2013 at 8:38

Attachments:

GoogleCodeExporter commented 9 years ago 
Will check out this new code.  I didn't have a proper windows machine to test 
on.  I think that the only people who are going to be running gcc on windows 
are likely to have the nt-based kernel?  Thanks!

Original comment by bl...@wanelo.com on 24 Sep 2013 at 8:35

GoogleCodeExporter commented 9 years ago 
integrated into r105

Original comment by yann.col...@gmail.com on 25 Sep 2013 at 9:05