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

Add a flag for IOJS to distinguish versioning from NodeJS #1364

Closed
wants to merge 1 commit into from

Conversation

modeswitch
Copy link

Introduce a new flag rather than overloading existing NODE_ flags. This will make version checks more clear and robust.

@bnoordhuis
Copy link
Member

I don't think this is the right approach because downstream projects like nw.js and atom will report IS_IOJS even when they are not in fact io.js (and may ship subtly incompatible libraries, for example.)

As discussed on IRC yesterday, the best approach is to check NODE_MODULE_VERSION if you care about the ABI level, or V8_{MAJOR,MINOR}_VERSION if you want to special-case on the version of V8.

The V8 macros are recent additions. Their absence tells you you're running on an old io.js version or node.js v0.10 or v0.12. Maybe you can persuade joyent/node to back-port the commit that added the macros, I know a lot of people have been asking for them.

@modeswitch
Copy link
Author

Looks like NW.js has defined distinct version macros: https://github.com/nwjs/nw.js/blob/master/src/nw_version.h

Do projects like atom use a modified version of iojs? If not, then they should report IS_IOJS, no?

@bnoordhuis
Copy link
Member

I'm reasonably sure they float patches from time to time.

@modeswitch
Copy link
Author

How is this intended to work over the long-term? Node, IOJS, and Atom are all using NODE_MODULE_VERSION. If Node wants to increment its value, does a developer need to check all these other projects to make sure the new value isn't already in use?

@bnoordhuis
Copy link
Member

ABI version bumps are infrequent and the projects have NODE_MODULE_VERSION values that are spaced pretty far apart. They shouldn't conflict anytime soon.

@modeswitch
Copy link
Author

Looking at nan, it seems like that's already happened at least for Atom and IOJS:

#define NODE_0_10_MODULE_VERSION 11
#define NODE_0_12_MODULE_VERSION 12
#define ATOM_0_21_MODULE_VERSION 41
#define IOJS_1_0_MODULE_VERSION  42
#define IOJS_1_1_MODULE_VERSION  43

@kkoopa
Copy link

kkoopa commented Apr 7, 2015

That atom entry was added retroactively specifically to coincide. io.js started out on 42 nodejs/nan#226 (comment)

@modeswitch
Copy link
Author

@kkoopa That's pretty much my point. This is already complicated, mostly due to the fact that these preprocessor definitions have multiple meanings.

NW.js seems to have done the sane thing and renamed them to NW_ instead, so there's no chance for code to confuse it with NodeJS.

As discussed on IRC yesterday ...

I wasn't involved in this discussion, so I have no idea what alternatives were covered. Unless there's a transcript I don't see how this is helpful.

the best approach is to check NODE_MODULE_VERSION if you care about the ABI level ...

It seems like a better approach would be to avoid reusing these macros, so that sane tests like NODE_MODULE_VERSION >= X continue to make sense.

@silverwind silverwind added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 9, 2015
@Fishrock123
Copy link
Contributor

I wasn't involved in this discussion, so I have no idea what alternatives were covered. Unless there's a transcript I don't see how this is helpful.

http://logs.libuv.org/io.js/2015-04-06#20:07:10.035

Closing as this sort of thing has been suggested before, and the general consensus is it isn't the correct approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants