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

src: introduce process.release object (WIP) #493

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jan 18, 2015

I'm messing around in node-gyp to see how workable this is and I haven't actually tested this on Windows yet although I've modified vcbuild.bat. Putting this up early to get feedback.

supersedes #491 and see also nodejs/node-gyp#564 for further discussion and an apparent 👍 from @TooTallNate on the proposal.

READONLY_PROPERTY(release,
"libUrl",
OneByteString(env->isolate(), NODE_RELEASE_LIB_URL));
#endif
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it possible to reconstruct the download URL from the NODE_VERSION_* macros in src/node_version.h? The approach you've taken is not bad but it's considerably more complex than is strictly necessary, I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes ... this is an option but this gives us more flexibility for testing and baking in arbitrary urls to releases including nightlies, I can hardwire the host in here and handle the nightlies special case with macros if you think that's the best way to go. For now this is really helpful for testing this feature so I'll leave it asis.

@TooTallNate
Copy link
Contributor

I like this 😄

A thought: Is perhaps process.release a bit redundant with process.config? I'm thinking it would be simpler if we just re-used that mechanism, since whatever's in config.gypi will be on that object during runtime (would make the changes in node.gyp and src/node.cc unnecessary).

We would just need to move the 3 URL props to the root-level of the config.gypi file, and add config.name = 'iojs' in the configure phase, so that it ends up in the config.gypi file as well.

@rvagg
Copy link
Member Author

rvagg commented Jan 18, 2015

@TooTallNate that's a good suggestion; I'm working my way through some node-gyp changes for this and that's the really difficult end so I'll loop back here when I have something working over there.

@rvagg rvagg added the semver-minor PRs that contain new features and should be released in the next minor version. label Jan 23, 2015
@cjihrig
Copy link
Contributor

cjihrig commented Feb 15, 2015

@rvagg is anything still holding this up?

@rvagg
Copy link
Member Author

rvagg commented Feb 16, 2015

Yes, I'm the holdup, we need to arrive on code that's acceptable to both node-gyp and io.js before we can land either.

The current state of code is @ https://github.com/rvagg/node-gyp/compare/iojs?expand=1 but it's pending proper Windows support, it's only part-way there. I'll try and get to it this week but if I keep finding that I just don't have time to finish it up I'll put out a call for someone else to take over.

@mikeal
Copy link
Contributor

mikeal commented Feb 25, 2015

Didn't I see windows gyp support land? Does that unblock this?

@JCMais
Copy link
Contributor

JCMais commented Feb 27, 2015

What is the status here? Anything you need help with @rvagg?

@othiym23
Copy link
Contributor

othiym23 commented Mar 9, 2015

This is still blocking nodejs/node-gyp#564, which is in turn blocking npm from shipping a floating-patch free version of itself. Is there anything I can do to move this along, @rvagg?

am11 referenced this pull request in sass/node-sass Mar 9, 2015
@Fishrock123
Copy link
Contributor

@rvagg now that you're less busy, any chance of this landing soon? Seems like a few people depend on this.

@stevenvachon
Copy link

..................................

@jbergstroem
Copy link
Member

@stevenvachon it'd be great if you avoided comments like these. Since a lot of people get pinged when new info arrives, we also assume that information will be beneficial to us. Thanks for understanding.

@stevenvachon
Copy link

@jbergstroem and other comments merely asking for status updates shouldn't also be avoided?

@silverwind
Copy link
Contributor

@rvagg any progress here?

@stevenvachon
Copy link

@rvagg wtfux?

@brendanashworth brendanashworth added the wip Issues and PRs that are still a work in progress. label May 11, 2015
nylen added a commit to request/request-debug that referenced this pull request May 13, 2015
while we're waiting on nodejs/node#493
@nylen
Copy link

nylen commented May 13, 2015

@rvagg @TooTallNate @othiym23

i-need-dis-otter

@rvagg
Copy link
Member Author

rvagg commented May 14, 2015

I do dis ... it's becoming more and more urgent; I just have to squash a . few . more . things . from . my . TODO

@Fishrock123
Copy link
Contributor

Hmm, semver-minor.. no way to port this to 1.x then? It'd probably be worth it.

@domenic
Copy link
Contributor

domenic commented May 14, 2015

@Fishrock123 if the LTS working group wills it, I'm sure they can make it happen.

@lygstate
Copy link

Looks good, when to merge?

@stevenvachon
Copy link

Is this necessary anymore since http://www.infoq.com/news/2015/05/nodejs-iojs ?

@rvagg
Copy link
Member Author

rvagg commented Jul 5, 2015

Yes because 1) io.js still exists and is still releasing and has a patched version of node-gyp that you can't upgrade and more importantly 2) we need to support nightlies and rc builds with node-gyp and this metadata provides us with the flexibility to do so.

@Fishrock123
Copy link
Contributor

Succeeded by #2154

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.