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

Build: Produces identifiable binary name #657

Merged
merged 2 commits into from
Feb 10, 2015
Merged

Build: Produces identifiable binary name #657

merged 2 commits into from
Feb 10, 2015

Conversation

am11
Copy link
Contributor

@am11 am11 commented Feb 10, 2015

The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: #655.

//cc @xzyfer

am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.
Related Issue: sass#655.
PR URL: sass#657.
am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
module.exports.getBinaryIdentifiableName = function() {
return process.platform + '-' +
process.arch + '-' +
runtime + '-' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like you factored out runtime

Copy link
Contributor Author

Choose a reason for hiding this comment

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

LOL yes and one more thing. Repushing now 😃

@xzyfer
Copy link
Contributor

xzyfer commented Feb 10, 2015

Will this approach handle things that aren't node or io, like atom shell?

am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
@am11
Copy link
Contributor Author

am11 commented Feb 10, 2015

@xzyfer, I have not yet looked into atom shell yet. We can incorporate it later. However, we can add binary for io.js in node-sass-binaries v2.0.0.

am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
@xzyfer
Copy link
Contributor

xzyfer commented Feb 10, 2015

Yep! It's definitely a step forward

am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
am11 added a commit to am11/node-sass that referenced this pull request Feb 10, 2015
The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.28%) to 92.59% when pulling 2be25ba on am11:master into 9c3accb on sass:master.

The new format is:
{platform}-{arch}-{runtime}-{major.minor}
where major and minor version belong to
the runtime.

Related Issue: sass#655.
PR URL: sass#657.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.28%) to 92.59% when pulling 4b5ebaf on am11:master into 9c3accb on sass:master.

@@ -8,11 +9,10 @@ var fs = require('fs'),
*/

function getBinding() {
var name = process.platform + '-' + process.arch;
var candidates = [
path.join(__dirname, '..', 'build', 'Release', 'binding.node'),
path.join(__dirname, '..', 'build', 'Debug', 'binding.node'),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@xzyfer, @kevva,
I think Debug should take precedence over Release here; because if you explicitly build Debug binary with node-gyp rebuild -d, then your primary intent is to debug that binary in debug mode. :)

Copy link
Member

Choose a reason for hiding this comment

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

y

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because if you explicitly build Debug binary with node-gyp rebuild -d , then your primary intent is to debug that binary in debug mode

am11 added a commit that referenced this pull request Feb 10, 2015
Build: Produces identifiable binary name
@am11 am11 merged commit 1180417 into sass:master Feb 10, 2015
@am11
Copy link
Contributor Author

am11 commented Feb 10, 2015

@xzyfer, tested with atom shell, it returns win32-ia32-atom-1.0. 😏

@xzyfer
Copy link
Contributor

xzyfer commented Feb 10, 2015

Nailed it 👍
On 10 Feb 2015 21:11, "Adeel Mujahid" notifications@github.com wrote:

@xzyfer https://github.com/xzyfer, tested with atom shell, it returns
win32-ia32-atom-1.0. [image: 😏]


Reply to this email directly or view it on GitHub
#657 (comment).

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.

None yet

4 participants