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.

Sign up for GitHub

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

Jump to bottom

s390: enable march=z196 #11730

Closed
wants to merge 1 commit into from
Closed

s390: enable march=z196 #11730

wants to merge 1 commit into from

Conversation

john-yan
Copy link

@john-yan john-yan commented Mar 7, 2017

enable -march=z196 for s390, because it's the lowest platform we intend to support for zlinux platforms.

@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 7, 2017
@addaleax
Copy link
Member

addaleax commented Mar 7, 2017

@nodejs/platform-s390

@sam-github
Copy link
Contributor

ci: https://ci.nodejs.org/job/node-test-pull-request/6737/

@gibfahn
Copy link
Member

gibfahn commented Mar 7, 2017

cc/ @joransiu @jBarz

@joransiu
Copy link
Contributor

joransiu commented Mar 7, 2017

LGTM. http://www-03.ibm.com/systems/z/os/linux/resources/testedplatforms.html shows that matches the minimum hardware requirements for the distros with right level of GCC/Glibc support (SLES 12, RHEL 7, Ubuntu 16.04 or newer).

richardlau
richardlau approved these changes Mar 7, 2017
View reviewed changes
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

LGTM.

Does this need to be noted in #8922?

@sam-github
Copy link
Contributor

Hard to believe this broke OS X, try again:

ci: https://ci.nodejs.org/job/node-test-pull-request/6738/

@jBarz
Copy link
Contributor

jBarz commented Mar 7, 2017

LGTM

@john-yan
Copy link
Author

john-yan commented Mar 7, 2017

@sam-github It doesn't seem to fail by looking at the details of ARM build.

@sam-github
Copy link
Contributor

ARM builds always fail, so ci result is a pass.

@gibfahn
Copy link
Member

gibfahn commented Mar 7, 2017 •
edited
Loading

Rerun of CI was green (arm failure on Github is a reporting issue, you need to click on the CI link).

EDIT: what Sam said

mhdawson
mhdawson approved these changes Mar 7, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex mscdex added the s390 Issues and PRs related to the s390 architecture. label Mar 7, 2017
@john-yan
Copy link
Author

john-yan commented Mar 8, 2017

any updates on this?

@sam-github
Copy link
Contributor

Requires 2 mid-week days to give people time to comment before it can be merged.

@gibfahn
Copy link
Member

gibfahn commented Mar 8, 2017

@john-yan see https://github.com/nodejs/node/blob/master/CONTRIBUTING.md

@mhdawson
Copy link
Member

Landing since its now past the wait time.

@mhdawson
Copy link
Member

Landed as b806e18

@mhdawson mhdawson closed this Mar 10, 2017
mhdawson pushed a commit that referenced this pull request Mar 10, 2017
@mhdawson
s390: enable march=z196
b806e18 
PR-URL: #11730
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 13, 2017
@italoacasas
s390: enable march=z196
52f0092 
PR-URL: nodejs#11730
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@italoacasas italoacasas mentioned this pull request Mar 13, 2017
Merged
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
@jungx098
s390: enable march=z196
9072293 
PR-URL: nodejs#11730
Reviewed-By: Sam Roberts <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

should this be backported to v6.x?

@gibfahn
Copy link
Member

gibfahn commented Apr 18, 2017

Talked to @joransiu and @john-yan , and this probably shouldn't apply to v6.x.

cc/ @nodejs/platform-s390 , if anyone disagrees let me know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sam-github sam-github sam-github approved these changes

@jasnell jasnell jasnell approved these changes

@jbajwa jbajwa jbajwa approved these changes

@cjihrig cjihrig cjihrig approved these changes

@richardlau richardlau richardlau approved these changes

@mhdawson mhdawson mhdawson approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI. s390 Issues and PRs related to the s390 architecture.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.