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

update Makefile for macosX installation about using luajit building deps #217

Merged
merged 8 commits into from
Jul 10, 2019

Conversation

idevz
Copy link
Contributor

@idevz idevz commented Jul 7, 2019

No description provided.

@CLAassistant
Copy link

CLAassistant commented Jul 7, 2019

CLA assistant check
All committers have signed the CLA.

@idevz
Copy link
Contributor Author

idevz commented Jul 7, 2019

I will add a macosx .travis soon to check this.

Makefile Outdated
@@ -52,13 +54,13 @@ init:
run:
mkdir -p logs
mkdir -p /tmp/cores/
$$(which openresty) -p $$PWD/
$(OR_EXEC) -p $$PWD/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to specified fully path for config file, eg: -c $$PWD/conf/nginx.conf.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may fail under Mac OSX.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed already: 5fccb78

please rebase to master.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, -c is needed under Mac OSX. I have add a travis for it, and all check passed. Let's merge it alright?

@membphis
Copy link
Member

membphis commented Jul 8, 2019

https://travis-ci.org/iresty/apisix/jobs/555975598

There are a lot of error logs on travis, please fix them at first.

We must check the execution status of the command in travis. It is dangerous to hide the fault, which means we have no test system.

@idevz idevz requested a review from membphis July 8, 2019 23:16
Makefile Outdated
@@ -52,13 +54,13 @@ init:
run:
mkdir -p logs
mkdir -p /tmp/cores/
$$(which openresty) -p $$PWD/
$(OR_EXEC) -p $$PWD/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed already: 5fccb78

please rebase to master.

}

do_install() {
if [ $TRAVIS_OS_NAME = osx ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more convenient for different systems to use separate scripts.

how about: ***_mac.sh, ***_linux.sh ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if using like ***_mac.sh, ***_linux.sh things, the control logic about to using which one will need to write in .travis.yaml, I think it's so ugly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about this way in travis: bash utils/travis_$TRAVIS_OS_NAME.sh

@@ -4,6 +4,8 @@ INST_LUADIR ?= $(INST_PREFIX)/share/lua/5.1
INST_BINDIR ?= /usr/bin
INSTALL ?= install
UNAME ?= $(shell uname)
OR_EXEC ?= $(shell which openresty)
LUA_JIT_DIR ?= $(shell TMP='./v_tmp' && $(OR_EXEC) -V &>$${TMP} && cat $${TMP} | grep prefix | grep -Eo 'prefix=(.*?)/nginx' | grep -Eo '/.*/' && rm $${TMP})luajit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'./v_tmp' -> '/tmp/v_tmp', how about this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it doesn't make any sense, the v_tmp file will been clean immediately at the end of this command.

script() {
export_or_prefix
export PATH=$OPENRESTY_PREFIX/nginx/sbin:$OPENRESTY_PREFIX/luajit/bin:$OPENRESTY_PREFIX/bin:$PATH
if [ $TRAVIS_OS_NAME = osx ]; then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer this style:

# mac os
if [ $TRAVIS_OS_NAME = osx ]; then
   ...
   return
fi

# default linux
...

@membphis
Copy link
Member

membphis commented Jul 9, 2019

Code coverage check seems to have failed, but I can not sure for this. @moonming pls take a look at this.

image

@moonming moonming merged commit 374aba4 into master Jul 10, 2019
@moonming moonming deleted the idevz branch August 2, 2019 06:39
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.

4 participants