Fix compiling against njs 0.8.3

[drmad]

[!NOTE] This PR applies to NJS 0.8.3, not earlier versions. Original comment follows.

As @hongzhidao noted, Unit 1.31 should use njs 0.8.1. This commit removes two NULL parameter when creating a struct njs_vm_ops_t variable in nxt_script.c and nxt_js.c to comply with njs_vm_ops_t definition in njs.h from njs 0.8.2

[hongzhidao]

If this is making it work with njs 0.8.2 doesn't it then break it for earlier versions?

Unit 1.3.1 should only work with njs 0.8.0 and njs 0.8.1. We need to enhance the document.

Do we have access to the njs version at build time

Unfortunately, there's no njs version from the library.

[ac000]

If this is making it work with njs 0.8.2 doesn't it then break it for earlier versions?

Unit 1.3.1 should only work with njs 0.8.0 and njs 0.8.1. We need to enhance the document.

But this patch is making it work with 0.8.2, as per the summary line

Fix unit 1.31 compiling with njs 0.8.2

Do we have access to the njs version at build time

Unfortunately, there's no njs version from the library.

Hmm, the version is in njs.h

#define NJS_VERSION                 "0.8.3"
#define NJS_VERSION_NUMBER          0x000803

[hongzhidao]

But this patch is making it work with 0.8.2, as per the summary line

I haven't reviewed the PR.

Hmm, the version is in njs.h

I didn't notice it, but thanks for pointing it out. We discussed if we need to maintain different versions of support of njs. We thought the points:

1. The library hasn't turned into a stable big about its version like 1.0.
2. I feel we can follow the way of nginx njs module which only supports the latest version.

I'd like not to maintain njs version now.

[ac000]

OK, well I'm just very confused...

The commit summary says

Fix unit 1.31 compiling with njs 0.8.2

The commit message says

As @hongzhidao noted, Unit 1.31 should use njs 0.8.1.

These two statements seem at odds with each other. Making it compile with 0.8.2 seems like it will break it for 0.8.1.

[hongzhidao]

Agree with Andrew's point.

The code change looks good. As I understand, this commit is to adjust for njs 0.8.2 structural changes. I would suggest something like this:

NJS: adjust for njs 0.8.2 structural changes.

more details

Feel free to rewrite it.

[thresheek]

Interestintly I don't have issues building current git HEAD Unit with njs 0.8.2. Are you talking about non-released njs 0.8.3 yet maybe, e.g. current mercurial version?

[thresheek]

Indeed, the changeset that removes the fields Unit mentions is not in 0.8.2, but in unreleased yet 0.8.3: https://hg.nginx.org/njs/rev/dffdf7c50dfc

[tippexs]

@hongzhidao can you take another look if this is ready to be shipped with 1.32?

[hongzhidao]

Hi @drmad

Indeed, the changeset that removes the fields Unit mentions is not in 0.8.2, but in unreleased yet 0.8.3: https://hg.nginx.org/njs/rev/dffdf7c50dfc

I think @thresheek is correct. The njs 0.82 works well, here's the structure. https://hg.nginx.org/njs/file/45f81882c780/src/njs.h#l242

Would you mind changing the issue title from 0.82 to 0.83?

[hongzhidao]

Hi Timo, It looks like we only need to check if we should upgrade njs to 0.82. @thresheek, welcome to suggest. The PR belongs to Unit 1.33.

[thresheek]

I've already proposed a bump for njs to 0.8.2 in https://github.com/nginx/unit/pull/1031 to be shipped with 1.32 so there's that.

[hongzhidao]

Hi all, Welcome to discuss the new PR https://github.com/nginx/unit/pull/1134 since there are a bit many changes for njs 0.83. We'll have to do more jobs on it. Thanks.

[callahad]

I've adjusted the title and edited the first message to hopefully clear up some confusion.

[ac000]

This PR was 'the land of confusion..."

AIUI this PR is superseded by https://github.com/nginx/unit/pull/1134

[callahad]

Ah, yep, that replaces this. Closing as superseded. Thank you, @drmad!