Njs: WIP: Improved auto configure.

Created on 12 Mar 2019 · 42Comments · Source: nginx/njs

@xeioex

Take a look. (draft)
https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90

Generated Makefile.
https://gist.github.com/hongzhidao/671e48e01504debeae2e4c9babbd8092

Some thoughts, welcome to suggest.

Move nxt/auto to outside since njs also need auto configure. (I thought add njs/auto, but it looks bad)
Move all the generated file into build directory. (nxt/nxt_auto_config.h, nxt/Makefile.conf).
Add '-MMD -MF -MT'
Add auto/sources, auto/make inspired from UNIT.
rename build/njs as build/js, I can't find an ideal way to create build/njs directory.
(or build/njs, build/src/nxt, build/src/njs?)
What about replace $NXT_LIB, $NXT_AUTO as nxt, auto? (hardcode the same in UNIT)
Rename NXT_BUILDDIR as NXT_BUILD_DIR?
Do you think njs is a library? (NJS_LIB_OBJS vs NJS_OBJS?). It seems yes.

Updated again.
https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90

feature

Source

hongzhidao

👍1

All 42 comments

@xeioex

Improved Makefile.

# HG changeset patch
# User hongzhidao <[email protected]>
# Date 1552401371 -28800
# Node ID 6211dd64d7935221764406781c73dae5e85b2b93
# Parent  49f4eb5afc9bd6528d08d2e07b4f1a3232c8a296
Improved Makefile.

diff -r 49f4eb5afc9b -r 6211dd64d793 Makefile
--- a/Makefile  Mon Mar 11 18:31:40 2019 +0300
+++ b/Makefile  Tue Mar 12 22:36:11 2019 +0800
@@ -7,7 +7,7 @@ NXT_BUILDDIR =  build

 $(NXT_BUILDDIR)/libnjs.a: \
    $(NXT_LIB)/nxt_auto_config.h \
-   $(NXT_BUILDDIR)/njs_shell.o \
+   $(NXT_BUILDDIR)/njs.o \
    $(NXT_BUILDDIR)/njs_vm.o \
    $(NXT_BUILDDIR)/njs_boolean.o \
    $(NXT_BUILDDIR)/njs_number.o \
@@ -55,7 +55,7 @@ NXT_BUILDDIR =    build
    $(NXT_BUILDDIR)/nxt_sprintf.o \

    ar -r -c $(NXT_BUILDDIR)/libnjs.a \
-       $(NXT_BUILDDIR)/njs_shell.o \
+       $(NXT_BUILDDIR)/njs.o \
        $(NXT_BUILDDIR)/njs_vm.o \
        $(NXT_BUILDDIR)/njs_boolean.o \
        $(NXT_BUILDDIR)/njs_number.o \
@@ -104,6 +104,7 @@ NXT_BUILDDIR =  build

 all:   test lib_test

+.PHONY: njs
 njs:   $(NXT_BUILDDIR)/njs

 libnjs:    $(NXT_BUILDDIR)/libnjs.a
@@ -136,7 +137,7 @@ dist:
    @echo
    @exit 1

-$(NXT_BUILDDIR)/njs_shell.o: \
+$(NXT_BUILDDIR)/njs.o: \
    $(NXT_BUILDDIR)/libnxt.a \
    njs/njs.h \
    njs/njs_core.h \
@@ -149,7 +150,7 @@ dist:
    njs/njs.h \
    njs/njs.c \

-   $(NXT_CC) -c -o $(NXT_BUILDDIR)/njs_shell.o $(NXT_CFLAGS) \
+   $(NXT_CC) -c -o $(NXT_BUILDDIR)/njs.o $(NXT_CFLAGS) \
        -I$(NXT_LIB) -Injs \
        njs/njs.c

Do you think target dist is still used?
Report error while running make dist

NJS_VER=`grep NJS_VERSION njs/njs.h | sed -e 's/.*"\(.*\)".*/\1/'`; \
    rm -rf njs-${NJS_VER} \
    && hg archive njs-${NJS_VER}.tar.gz \
              -p njs-${NJS_VER} \
              -X ".hg*" \
    && echo njs-${NJS_VER}.tar.gz done
abort: cannot give prefix when archiving to files

hongzhidao on 12 Mar 2019

👍1

@hongzhidao

Do you think target dist is still used?
Report error while running make dist

yes. It is used. Currently it works as expected.

make dist
NJS_VER=`grep NJS_VERSION njs/njs.h | sed -e 's/.*"\(.*\)".*/\1/'`; \
rm -rf njs-${NJS_VER} \
&& hg archive njs-${NJS_VER}.tar.gz \
              -p njs-${NJS_VER} \
              -X ".hg*" \
&& echo njs-${NJS_VER}.tar.gz done
njs-0.3.0.tar.gz done

xeioex on 12 Mar 2019

@hongzhidao

Move nxt/auto to outside since njs also need auto configure. (I thought add njs/auto, but it looks bad)

Move all the generated file into build directory. (nxt/nxt_auto_config.h, nxt/Makefile.conf).

Add '-MMD -MF -MT'

Add auto/sources, auto/make inspired from UNIT.

rename build/njs as build/js, I can't find an ideal way to create build/njs directory.
(or build/njs, build/src/nxt, build/src/njs?)

what is wrong with njs name?) it is a project name.

What about replace $NXT_LIB, $NXT_AUTO as nxt, auto? (hardcode the same in UNIT)

Rename NXT_BUILDDIR as NXT_BUILD_DIR?

Agree.

Do you think njs is a library? (NJS_LIB_OBJS vs NJS_OBJS?). It seems yes.

Yes, also we need a share library target.

xeioex on 12 Mar 2019

rename build/njs as build/js, I can't find an ideal way to create build/njs directory.
(or build/njs, build/src/nxt, build/src/njs?)
what is wrong with njs name?) it is a project name.

Plan to use build/src/x.o.

OK, I plan to archive it by refactor. On it.

hongzhidao on 12 Mar 2019

👍1

@hongzhidao

Improved Makefile.

BTW, I am not sure, whether we need this. If it will be completely replaced with new code.

xeioex on 12 Mar 2019

Deleted.

hongzhidao on 12 Mar 2019

@hongzhidao

https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90

I think, it is better to squash changes into one commit, while stating in commit log what was done.

xeioex on 12 Mar 2019

@hongzhidao

Improved Makefile.

BTW, I am not sure, whether we need this. If it will be completely replaced with new code.

Which patch?

hongzhidao on 12 Mar 2019

@hongzhidao

From https://github.com/nginx/njs/issues/110#issuecomment-472028553

xeioex on 12 Mar 2019

@hongzhidao

https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90

I think, it is better to squash changes into one commit, while stating in commit log what was done.

OK.

hongzhidao on 12 Mar 2019

@hongzhidao

From #110 (comment)

The key is this line. +.PHONY: njs
https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90#file-njs-auto-patch-L33

And I think it's clear that we add njs.o.

hongzhidao on 12 Mar 2019

The key is https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90#file-njs-auto-patch-L33

BTW, regarding .PHONY

Why only njs target and not other non-file targets?

xeioex on 12 Mar 2019

@hongzhidao

And I think it's clear that we add njs.o.

it is true. But, is not it will be overwritten by nxt/make script in later commits?

xeioex on 12 Mar 2019

The key is https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90#file-njs-auto-patch-L33

BTW, regarding .PHONY

Why only njs target and not other non-file targets?

Actually, other targets are also needed.
Sorry that I mislead you. We can ignore it.

Previously I add build/njs/src.o (now use build/src/src.o), we need to distinguish build/njs and build/njs/.
build/njs.o is auto generated by auto/make.

hongzhidao on 12 Mar 2019

@hongzhidao

And I think it's clear that we add njs.o.

it is true. But, is not it will be overwritten by nxt/make script in later commits?

I'll commit these by a complete patch.

hongzhidao on 12 Mar 2019

@hongzhidao

My concern is: if we have a huge changeset, I would rather have one such changeset instead of several of them. Logically, it is a single change (Introducing UNIT makefile dependency scripts.)

xeioex on 12 Mar 2019

👍1

@xeioex

Take a look.
https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90

hongzhidao on 13 Mar 2019

@hongzhidao

https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90

Looks great. Except for nxt/Makefile do we really need it as independent makefile?

xeioex on 13 Mar 2019

@hongzhidao

https://gist.github.com/hongzhidao/2c1ae0639771c366fa86093ecdac3e90

Looks great. Except for nxt/Makefile do we really need it as independent makefile?

I noticed it, it'll be deleted.

hongzhidao on 13 Mar 2019

👍1

@xeioex

Do you think njs is a library? (NJS_LIB_OBJS vs NJS_OBJS?). It seems yes.
Yes, also we need a share library target.

Do you mean we need libnjs.so? (what about libnxt.so?)

hongzhidao on 13 Mar 2019

@hongzhidao

Do you mean we need libnjs.so? (what about libnxt.so?)

yes (but only libnjs.so). It will be a separate topic, not for now.

xeioex on 13 Mar 2019

@hongzhidao

Do you mean we need libnjs.so? (what about libnxt.so?)

yes (but only libnjs.so). It will be a separate topic, not for now.

It's easy to add in. I'll add it in the same commit. (will do in hours, I need process other work.)

hongzhidao on 13 Mar 2019

@hongzhidao

It's easy to add in. I'll add it in the same commit.

yes, it is easy. No doubt. The main question is how we going to use it (link with nginx during configuration phase, we also have to install that library into system lib dirs (?? not sure)).

xeioex on 13 Mar 2019

@hongzhidao

It's easy to add in. I'll add it in the same commit.

yes, it is easy. No doubt. The main question is how we going to use it (link with nginx during configuration phase, we also have to install that library into system lib dirs (?? not sure)).

In real case, I prefer to use static library, it's easier to maintain.

hongzhidao on 13 Mar 2019

@hongzhidao

In real case, I prefer to use static library, it's easier to maintain.

yes, therefore I would leave it as is for now (no .so targets).

xeioex on 13 Mar 2019

👍1

@hongzhidao

You forgot to delete files in nxt/auto.

you did a new copy of the files in auto dir.

--- /dev/null   Thu Jan 01 00:00:00 1970 +0000
+++ b/auto/clang    Wed Mar 13 15:04:58 2019 +0800

The correct way is hg mv existing files to preserve history.

xeioex on 13 Mar 2019

@hongzhidao

You forgot to delete files in nxt/auto.

you did a new copy of the files in auto dir.
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/auto/clang  Wed Mar 13 15:04:58 2019 +0800
The correct way is hg mv existing files to preserve history.

OK, I need to commit again. And it'll close to the final design.

hongzhidao on 13 Mar 2019

@hongzhidao

diff --git a/nginx/config b/nginx/config
--- a/nginx/config
+++ b/nginx/config
@@ -3,7 +3,7 @@ ngx_addon_name="ngx_js_module"
 if [ $HTTP != NO ]; then
     ngx_module_type=HTTP
     ngx_module_name=ngx_http_js_module
-    ngx_module_incs="$ngx_addon_dir/../nxt $ngx_addon_dir/../njs"
+    ngx_module_incs="$ngx_addon_dir/../nxt $ngx_addon_dir/../njs $ngx_addon_dir/../build"
     ngx_module_deps="$ngx_addon_dir/../build/libnjs.a"
     ngx_module_srcs="$ngx_addon_dir/ngx_http_js_module.c"
     ngx_module_libs="PCRE $ngx_addon_dir/../build/libnjs.a -lm"
@@ -14,7 +14,7 @@ fi
 if [ $STREAM != NO ]; then
     ngx_module_type=STREAM
     ngx_module_name=ngx_stream_js_module
-    ngx_module_incs="$ngx_addon_dir/../nxt $ngx_addon_dir/../njs"
+    ngx_module_incs="$ngx_addon_dir/../nxt $ngx_addon_dir/../njs $ngx_addon_dir/../build"
     ngx_module_deps="$ngx_addon_dir/../build/libnjs.a"
     ngx_module_srcs="$ngx_addon_dir/ngx_stream_js_module.c"
     ngx_module_libs="PCRE $ngx_addon_dir/../build/libnjs.a -lm"

xeioex on 13 Mar 2019

👍1

@xeioex

I have an idea. Can you recommit it and I review it? It's easier for me to find a problem.
I'm not sure it's reasonable :)

hongzhidao on 13 Mar 2019

@hongzhidao

OK, but I am a bit busy now, will do later today.

xeioex on 13 Mar 2019

😄1

@hongzhidao

https://gist.github.com/xeioex/4fc3879de40a83085c4ff06cd9383a76

Still in progress. Many changes.

for example

clean:
    rm -rf $NXT_BUILD_DIR Makefile

removes main makefile.

Makefile is redesigned a bit:
it consist of
1) static part (make clean, make dist, ..)
2) generated parts:

-include build/Makefile.conf
...
-include build/Makefile

make njs (without configure is fixed), now it warns: Please run ./configure before make
NXT_LIB_AUX_CFLAGS as in Unit.

xeioex on 13 Mar 2019

👍1

@xeioex I'll see it tomorrow.

hongzhidao on 13 Mar 2019

@hongzhidao

https://gist.github.com/xeioex/c2e5f72d08874573c200e1d63d719fcb

generating in auto/make njs and nxt tests.
test targets are refactored.

What is left to do:

hg mv is not ready yet for all nxt/auto files.
NXT_BUILD_DIR is not used in main Makefile (return it back).
It seems, Makefile.conf can be also removed.

xeioex on 13 Mar 2019

@xeioex

hg mv is not ready yet for all nxt/auto files.

If it's hard to deal, we can separate it as a patch. It can make the task easier.

Move Makefile.conf into build directory.
Move nxt_auto_config.h into build directory.
move nxt/auto outside.
What do think of this?

NXT_BUILD_DIR is not used in main Makefile (return it back).

In nginx config.make, build is hard-code, now in main Makefile it's also hard-code.
So, what about replace NXT_BUILD_DIR as build?

It seems, Makefile.conf can be also removed.

Agree, I do it base on your commit tomorrow.

hongzhidao on 13 Mar 2019

@hongzhidao

In nginx config.make, build is hard-code, now in main Makefile it's also hard-code.
So, what about replace NXT_BUILD_DIR as build?

build was (I think it should be returned back) a default value for NXT_BUILD_DIR
https://gist.github.com/xeioex/c2e5f72d08874573c200e1d63d719fcb#file-deps-03-13-patch-L1855

So, is config.make simply does ./configure && make it gets what is wants.

on the other hand, if you do it manually you can change it through env variables.

If it's hard to deal, we can separate it as a patch. It can make the task easier.

IMHO, it is not that hard, see https://gist.github.com/xeioex/c2e5f72d08874573c200e1d63d719fcb#file-deps-03-13-patch-L998 (nxt/auto/editline -> auto/editline)

hg mv -f nxt/auto/editline auto/editline
It can be do in semiautomatic way, replace:
$nxt_echo -> $echo
${NXT_AUTO}feature -> auto/feature

I can do it myself.

xeioex on 13 Mar 2019

@xeioex

It seems, Makefile.conf can be also removed.

Based on the release (not your commit).
https://gist.github.com/hongzhidao/e31175b2fa9fe041aff54320625c13a9

Separate $NXT_LIBRT from $NXT_LIB_AUX_LIBS. Reference to Unit.

$NXT_BUILD_DIR/$NXT_LIB_SHARED: \$(NXT_LIB_OBJS)
    \$(NXT_SHARED_LOCAL_LINK) -o $NXT_BUILD_DIR/$NXT_LIB_SHARED \\
        \$(NXT_LIB_OBJS) \\
        $NXT_LIBM $NXT_LIBS $NXT_LIB_AUX_LIBS

Add $NXT_EDITLINE_LIB into $NXT_LIB_AUX_LIBS.
Remove these code into auto/make.
https://gist.github.com/hongzhidao/e31175b2fa9fe041aff54320625c13a9#file-makefile-conf-patch-L45
https://gist.github.com/xeioex/c2e5f72d08874573c200e1d63d719fcb#file-deps-03-13-patch-L1357
Add $NXT_EDITLINE_LIB default value.
https://gist.github.com/xeioex/c2e5f72d08874573c200e1d63d719fcb#file-deps-03-13-patch-L1005
https://gist.github.com/hongzhidao/e31175b2fa9fe041aff54320625c13a9#file-makefile-conf-patch-L84
Optional. NXT_LIBM="-lm"

hongzhidao on 14 Mar 2019

@hongzhidao

Separate $NXT_LIBRT from $NXT_LIB_AUX_LIBS. Reference to Unit.

Done

Add $NXT_EDITLINE_LIB into $NXT_LIB_AUX_LIBS.

NXT_EDITLINE_LIB is only needed for njs_shell.c, left as is.

Add $NXT_EDITLINE_LIB default value.

Done

hg mv is not ready yet for all nxt/auto files.

Done

NXT_BUILD_DIR is not used in main Makefile (return it back).

Done

It seems, Makefile.conf can be also removed.

Done

Final version: https://gist.github.com/xeioex/4153e421c7034a445b64962199e52177

Please, check various targets combination.

xeioex on 14 Mar 2019

https://gist.github.com/xeioex/4153e421c7034a445b64962199e52177

Great work. Looks look except for one place.

\\
https://gist.github.com/xeioex/4153e421c7034a445b64962199e52177#file-deps-03-14-patch-L1205
https://gist.github.com/xeioex/4153e421c7034a445b64962199e52177#file-deps-03-14-patch-L1232

hongzhidao on 14 Mar 2019

https://gist.github.com/xeioex/4153e421c7034a445b64962199e52177#file-deps-03-14-patch-L1205

fixed

https://gist.github.com/xeioex/4153e421c7034a445b64962199e52177#file-deps-03-14-patch-L1232

BTW, https://github.com/nginx/unit/blob/master/auto/make#L124

xeioex on 14 Mar 2019

BTW, https://github.com/nginx/unit/blob/master/auto/make#L124

I think UNIT missed it.

Another place.
https://gist.github.com/xeioex/4153e421c7034a445b64962199e52177#file-deps-03-14-patch-L1152

hongzhidao on 14 Mar 2019

👍1

I plan to commit it tomorrow.

xeioex on 14 Mar 2019

🎉1

@hongzhidao

FYI, absence of \\ before $njs_dep_post is not a bug. $njs_dep_post is a separate command. On all platforms except solaris it is empty.
On solaris 11:

cc -o build/njs_unit_test -fPIC -O -fast -errwarn=%all -g -m64 \
    -I/usr/include/pcre -I nxt -I build \
    -xMMD -xMF build/test/njs_unit_test.dep.tmp \
    -Injs njs/test/njs_unit_test.c build/libnjs.a \
    -lm  -lpcre \
    @sed -e 's#^.*:#build/njs_unit_test.c:#' build/test/njs_unit_test.dep.tmp > build/test/njs_unit_test.dep && rm -f build/test/njs_unit_test.dep.tmp
ld: fatal: file @sed: open failed: No such file or directory
ld: fatal: file build/test/njs_unit_test.dep.tmp: not an ELF object
gmake: *** [build/Makefile:635: build/njs_unit_test] Error 2

xeioex on 15 Mar 2019

👍1

Was this page helpful?

0 / 5 - 0 ratings

Related issues

bound constructors

drsm · 3Comments

invoke accessors in Object.defineProperties()

drsm · 4Comments

Cannot install nginx-module-njs

reyou · 5Comments

String.prototype.split() fails to split a unicode string correctly

drsm · 3Comments

NJS performance vs Chrome V8 & Firefox

xbb123 · 4Comments