Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix a Makefile conditional to test for empty string instead of 'none' #2841

Merged
merged 1 commit into from
Nov 7, 2023

Conversation

bradcray
Copy link
Contributor

@bradcray bradcray commented Nov 3, 2023

I was hitting an issue where, due to not having CHPL_HOME set, CHPL_CXX was being set to the empty string and the 'make compile-arrow-cpp' step would fail as follows due to not having a C++ compiler specified (I'm still not sure where the '-' on the '-O3' option went...):

O3 -std=c++17 -c /Users/bradc/arkouda//src/ArrowFunctions.cpp -o /Users/bradc/arkouda//src/ArrowFunctions.o -I/Users/bradc/arkouda//dep/zeromq-install/include -L/Users/bradc/arkouda//dep/zeromq-install/lib -I/Users/bradc/arkouda//dep/hdf5-install/include -L/Users/bradc/arkouda//dep/hdf5-install/lib -I/Users/bradc/arkouda//dep/arrow-install/include -L/Users/bradc/arkouda//dep/arrow-install/lib -I/Users/bradc/arkouda//dep/arrow-install/include -L/Users/bradc/arkouda//dep/arrow-install/lib -I/opt/homebrew/opt/libiconv/include -L/opt/homebrew/opt/libiconv/lib -fsanitize=
make: O3: No such file or directory

The conditional that was designed to guard against this is the following:

ifeq ($(CHPL_CXX),none)
CHPL_CXX=$(CXX)
endif

but this didn't fire because CHPL_CXX wasn't unset, it was simply set to the empty string. Here, I've changed the logic to:

ifeq ($(CHPL_CXX),)
CHPL_CXX=$(CXX)
endif

to test for the empty string, which causes CXX to be set to g++ as intended.

I was hitting an issue where, due to not having CHPL_HOME set,
CHPL_CXX was being set to the empty string and the 'make
compile-arrow-cpp' step would fail as follows due to not having a C++
compiler specified (I'm still not sure where the '-' on the '-O3'
option went...):

```
O3 -std=c++17 -c /Users/bradc/arkouda//src/ArrowFunctions.cpp -o /Users/bradc/arkouda//src/ArrowFunctions.o -I/Users/bradc/arkouda//dep/zeromq-install/include -L/Users/bradc/arkouda//dep/zeromq-install/lib -I/Users/bradc/arkouda//dep/hdf5-install/include -L/Users/bradc/arkouda//dep/hdf5-install/lib -I/Users/bradc/arkouda//dep/arrow-install/include -L/Users/bradc/arkouda//dep/arrow-install/lib -I/Users/bradc/arkouda//dep/arrow-install/include -L/Users/bradc/arkouda//dep/arrow-install/lib -I/opt/homebrew/opt/libiconv/include -L/opt/homebrew/opt/libiconv/lib -fsanitize=
make: O3: No such file or directory
```

The conditional that was designed to guard against this is the following:

```make
ifeq ($(CHPL_CXX),none)
CHPL_CXX=$(CXX)
endif
```

but this didn't fire because CHPL_CXX wasn't unset, it was simply set
to the empty string.  Here, I've changed the logic to:

```make
ifeq ($(CHPL_CXX),)
CHPL_CXX=$(CXX)
endif
```

to test for the empty string, which causes CXX to be set to g++ as
intended.

---
Signed-off-by: Brad Chamberlain <[email protected]>
@bradcray
Copy link
Contributor Author

bradcray commented Nov 4, 2023

In putting this together, I found myself wondering whether the Makefile should be setting CHPL_HOME if it's not set by running chpl --print-chpl-home (since chpl is definitely required to build Arkouda). My thinking was that this would ensure that the CHPL_CXX was a version that was consistent with what Chapel would use.

But that was a bigger change than I wanted to take on, and that flag was only introduced in Chapel 1.31, so 1.30 users would face issues without some additional cleverness...

@bmcdonald3
Copy link
Contributor

I've opened #2842 so we don't drop that. I believe Brandon is planning to drop our 1.30 support in the coming weeks, now that we no longer technically need to be supporting it, so can likely implement that soon and don't think it's worth the extra effort to move forward with that now, since we are going to be dropping 1.30 support so soon.

@stress-tess stress-tess added this pull request to the merge queue Nov 7, 2023
Merged via the queue into Bears-R-Us:master with commit 4f3426e Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants