-
Notifications
You must be signed in to change notification settings - Fork 11
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
Add Ubuntu x86_64 FLEX_LIB_DIR into Makefile.defs #55
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checks failed. Maybe /usr/lib/x86_64-linux-gnu is not the correct path in this case?
@jimmielin With this update the sed command now causes The CI test can be fixed by updating the Dockerfile line from Technically, the above |
The check may have failed because probably didn't update the text replacement in the Dockerfile. I can look into a fix. |
Also the Dockerfile now pegs Ubuntu 20.04 and AMD64 platform, so that library paths will be consistent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks OK. I will fix the Dockerfile issue.
Thanks @laestrada and @yantosca! I didn't know there was a specific fix in Perhaps we should also improve ifeq ($(USER_FLEX_LIB_DIR),)
FLEX_LIB_DIR = ${FLEX_HOME}/lib64
else
FLEX_LIB_DIR = ${USER_FLEX_LIB_DIR}
endif Maybe we could add this: ifneq ($(USER_FLEX_LIB_DIR),)
FLEX_LIB_DIR = ${USER_FLEX_LIB_DIR}
endif And then we could probably just pass |
@jimmielin: I was thinking of a better way to specify the |
@jimmielin good suggestion! It would be much easier to just specify an env variable for |
@jimmielin, @RolfSander, this seems to work OK. I've tested it in the container locally (setting an environment variable ############################################################
### Default settings ###
############################################################
CC := gcc
CC_FLAGS := -g -Wall -Wno-unused-function
FLEX := flex
FLEX_LIB_DIR := /usr/lib
BISON := bison
INCLUDE_DIR := /usr/include
SYSTEM := $(shell uname) # returns e.g. "Linux"
SYSTEM_M := $(shell uname -m) # returns e.g. "x86_64"
HOST := $(shell hostname) # returns name of machine
############################################################
### Keep looking for the flex library until found ###
### Otherwise use the path specified in KPP_FLEX_LIB_DIR ###
### ###
### NOTE for MacOS X: IF you have installed flex with ###
### HomeBrew, then flex will be installed to a path such ###
### as /usr/local/Cellar/flex/X.Y.Z/lib, where X.Y.Z is ###
### the version number. -- Bob Yantosca (01 Nov 2021) ###
############################################################
# Try /usr/lib64
ifeq ($(wildcard $(FLEX_LIB_DIR)/libfl.*),)
FLEX_LIB_DIR := /usr/lib64
endif
# Try /usr/local/lib
ifeq ($(wildcard $(FLEX_LIB_DIR)/libfl.*),)
FLEX_LIB_DIR := /usr/local/lib
endif
# Try /usr/local/lib64
ifeq ($(wildcard $(FLEX_LIB_DIR)/libfl.*),)
FLEX_LIB_DIR := /usr/local/lib64
endif
# If flex isn't found, look again e.g. /lib/x86_64
ifeq ($(wildcard $(FLEX_LIB_DIR)/libfl.*),)
FLEX_LIB_DIR := /lib/$(SYSTEM_M)
endif
# If we can't find the flex library path, then specify it
# from the user's environment variable KPP_FLEX_LIB_DIR.
ifneq ($(KPP_FLEX_LIB_DIR),)
FLEX_LIB_DIR = ${KPP_FLEX_LIB_DIR}
endif
# ERROR CHECK: EXIT if we can't find libfl.a (Flex library file)
ifeq ($(wildcard $(FLEX_LIB_DIR)/libfl.*),)
$(error "Could not find the Flex library at $(FLEX_LIB_DIR)! Specify FLEX_LIB_DIR in Makefile.defs.")
endif
############################################################
### System-specific or host-specific default settings ###
############################################################
# Settings for MacOS
ifeq ($(SYSTEM),Darwin)
CC_FLAGS += -DMACOS -O
endif
############################################################# If you think this is OK I can merge & also update the docs. |
Also I think we can change |
Hi @yantosca, looks good to me! Thanks for improving this, getting rid of system-specific checks will definitely make |
Thanks @jimmielin. Will test on my Mac and also on Cannon then will merge. |
Hi @yantosca, we might have to add this clause as well:
This will then look for Tested on AWS Arm instances as well:
Edit: Actually |
Thanks, I'll add that too @jimmielin. |
Sorry, I just realized a weird bug where # If flex isn't found, look again e.g. /usr/lib/$(arch)-linux-gnu
ifeq ($(wildcard $(FLEX_LIB_DIR)/libfl.*),)
FLEX_LIB_DIR := /usr/lib/$(firstword $(SYSTEM_M))-linux-gnu
endif |
@jimmielin feel free to make any edits to the doc. I think I've covered it though. |
Thanks @yantosca! The documentation looks great as well! |
This is a minor fix that matches
Ubuntu
andx86_64
againstuname -a
to setFLEX_LIB_DIR=/usr/lib/x86_64-linux-gnu
for these systems. This should support a pretty wide range of Ubuntu-based linux distros out-of-the-box. (libfl-dev
still needs to be installed)I also updated the "error message" to better clarify that
Makefile.defs
needs to be changed, iflibfl.a
is not found.Perhaps we should also support
USER_FLEX_LIB_DIR
soFLEX_LIB_DIR
in this file can be overridden without changingMakefile.defs
in the future? Just to avoid accidentally committing user changes toMakefile.defs
in the future, I almost got caught by surprise...