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

doc: upgrade Clang requirement to 3.4.2 #12388

Closed
wants to merge 1 commit into from
Closed

doc: upgrade Clang requirement to 3.4.2 #12388

wants to merge 1 commit into from

Conversation

targos
Copy link
Member

@targos targos commented Apr 13, 2017

Clang 3.4.1 has a bug that prevents compilation of V8.
As of Node.js 8.0.0 we stopped floating a workaround for this issue.

I don't have a test system to check if 3.4.2 is enough.

/cc @nodejs/build

Checklist

@targos
doc: upgrade Clang requirement to 3.4.2
63e0295 
Clang 3.4.1 has a bug that prevents compilation of V8.
As of Node.js 8.0.0 we stopped floating a workaround for this issue.
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. labels Apr 13, 2017
bnoordhuis
bnoordhuis approved these changes Apr 13, 2017
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM and good catch on configure.

I'd be okay with raising the baseline to 3.5.2 or 3.6.2 for node 8. We'll be supporting it until 2021, it would make sense to start with a compiler that's not already long in the tooth.

@mhdawson
Copy link
Member

In terms 3.5.2 and 3.6.2 will those run on the existing OS levels were Clang is used in our regression runs ?

@jbergstroem
Copy link
Member

I'm all for raising the bar in node 8 as well. ubuntu trusty seems to offer clang 3.4, 3.5, 3.6 and 3.8.

Any arguments on 3.5.x vs 3.6.x?

benjamingr
benjamingr approved these changes Apr 13, 2017
View reviewed changes
Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

LGTM

@benjamingr benjamingr mentioned this pull request Apr 13, 2017
Merged
@bnoordhuis
Copy link
Member

In terms 3.5.2 and 3.6.2 will those run on the existing OS levels were Clang is used in our regression runs ?

I'd say so. I don't think we have any systems in our matrix where you can install 3.4 but not 3.5 or 3.6.

Any arguments on 3.5.x vs 3.6.x?

PPC support got a big boost in 3.6. Might be worth it for that reason alone.

@jbergstroem
Copy link
Member

@bnoordhuis said:
PPC support got a big boost in 3.6. Might be worth it for that reason alone.

Sounds good to me. The newer, the better.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

CI since there's a change in the configure script here: https://ci.nodejs.org/job/node-test-pull-request/7489/

@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

CI failure is unrelated

mhdawson
mhdawson approved these changes Apr 18, 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

jasnell pushed a commit that referenced this pull request Apr 18, 2017
@targos @jasnell
doc: upgrade Clang requirement to 3.4.2
b4f59a7 
Clang 3.4.1 has a bug that prevents compilation of V8.
As of Node.js 8.0.0 we stopped floating a workaround for this issue.

PR-URL: #12388
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

Landed in b4f59a7

@jasnell jasnell closed this Apr 18, 2017
@jasnell jasnell mentioned this pull request May 11, 2017
Closed
@targos targos deleted the clang-342 branch June 1, 2017 07:38
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

I'm assuming this doesn't apply to v7 and earlier, LMK if that's not correct.

@bnoordhuis
Copy link
Member

We carry a compatibility patch in older branches although it's no longer tested now that the clang 3.4.1 buildbot has been decommissioned. I'd back-port this PR, that way people know what the known-good version of clang is.

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@targos @gibfahn
doc: upgrade Clang requirement to 3.4.2
11a7875 
Clang 3.4.1 has a bug that prevents compilation of V8.
As of Node.js 8.0.0 we stopped floating a workaround for this issue.

PR-URL: #12388
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@gibfahn
Copy link
Member

gibfahn commented Jun 20, 2017

Backported in 11a7875

MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@targos @MylesBorins
doc: upgrade Clang requirement to 3.4.2
74a6929 
Clang 3.4.1 has a bug that prevents compilation of V8.
As of Node.js 8.0.0 we stopped floating a workaround for this issue.

PR-URL: #12388
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

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

Successfully merging this pull request may close these issues.
9 participants
@targos @mhdawson @jbergstroem @bnoordhuis @jasnell @gibfahn @benjamingr @cjihrig @nodejs-github-bot