Fixed OpenFST building on Windows/cygwin64

[binghe]

In tools/Makefile, when building OpenFST, the following code was used to have a special configure command on Windows/Cygwin:

ifeq ($(OSTYPE),cygwin)

But I found it doesn't work. $(OSTYPE) is null. However $(OS) is "Windows_NT", which is useful here.

For CXXFLAGS, in additional to "-O", I added "-Wa,-mbig-obj" for safety purposes. Now OpenFST builds correctly on Windows/Cygwin by just typing "make" in "tools" directory.

[jtrmal]

I'm a little nervous that you remove the old code path. How about adding it as "else if"
y.

[binghe]

Well ... I think adding "else if" is acceptable (and more safe). I'm using latest Cygwin64, but maybe in some old versions of Cygwin OSTYPE is indeed "cygwin" but OS is not set.

[jtrmal]

Exactly. Please add some comment that that OSTYPE path is probably dead
for cygwin version whatever you have and windows whatever you have.
THanks!
y.

On Thu, Nov 10, 2016 at 7:09 PM, Chun Tian notifications@github.com wrote:

@binghe commented on this pull request.

In tools/Makefile #1182:

@@ -78,8 +78,8 @@ openfst-$(OPENFST_VERSION)/lib: | openfst-$(OPENFST_VERSION)/Makefile

Add the -O flag to CXXFLAGS on cygwin as it can fix the compilation error

"file too big".

openfst-$(OPENFST_VERSION)/Makefile: openfst-$(OPENFST_VERSION)/.patched | check_required_programs
-ifeq ($(OSTYPE),cygwin)

• cd openfst-$(OPENFST_VERSION)/; ./configure --prefix=pwd --enable-static --enable-shared --enable-far --enable-ngram-fsts CXX=$(CXX) CXXFLAGS="$(CXXFLAGS) -O" LDFLAGS="$(LDFLAGS)" LIBS="-ldl"
  +ifeq ($(OS),Windows_NT)
• cd openfst-$(OPENFST_VERSION)/; ./configure --prefix=pwd --enable-static --enable-shared --enable-far --enable-ngram-fsts CXX=$(CXX) CXXFLAGS="$(CXXFLAGS) -O -Wa,-mbig-obj" LDFLAGS="$(LDFLAGS)" LIBS="-ldl"

Well ... I think adding "else if" is acceptable (and more safe). I'm using
latest Cygwin64, but maybe in some old versions of Cygwin OSTYPE is
"Cygwin" but OS is not set.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1182, or mute the thread
https://github.com/notifications/unsubscribe-auth/AKisX5qUg9snlD8UC3dqeihbUlrkGhDuks5q87I3gaJpZM4KvSIM
.

[binghe]

I have updated the branch with old conditional branch kept for safety purposes. Confirmed on Windows 10 / cygwin64.

[jtrmal]

Thanks! merging.