-
Notifications
You must be signed in to change notification settings - Fork 29.9k
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
doc: add supported platforms list #8922
Conversation
I should note that this is targeting v6, we can tweak for v7 if need be (I'm not sure it's any different) but it will need to be changed when backporting to v4. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with just some tiny copyedit nits
BUILDING.md
Outdated
|
||
### Input | ||
|
||
We rely on a few dependencies that _makes us who we are—most importantly V8 and libuv. We therefore need to adopt their supported platforms and potentially add to their lists based on test and/or release coverage. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Micro nits
_makes
->make
(remove underscore, go singular)who we are-most
->who we are - most
(spacing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this to be more direct. Maybe just:
We rely on V8 and libuv. We therefore adopt their supported platforms as a starting point.
Or whatever is accurate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that this should be clarified. Its easy to understand that we should be a subset of the platforms supported by the dependencies. Maybe:
We rely on V8 and libuv. We therefore support of subset of their supported platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax error on this line.
BUILDING.md
Outdated
|
||
### Shared libraries & dependencies | ||
|
||
Node.js intends to support building against shared representations of dependencies found found in the [*deps*][./deps/] directory. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"found found"
LGTM subject to fixing the Input section and the other nits mentioned by others. |
Seeing how we now test (green) smartos14 through 16 I'm inclined moving 15,16 off experimental. |
No CTC objections, need to fix up the "Input" section to clarify what it's supposed to mean. |
LGTM with the few nits addressed. |
BUILDING.md
Outdated
| GNU/Linux | Tier 2 | kernel >= 3.10, glibc >= 2.17 | s390x | | | ||
| macOS | Experimental | >= 10.8 < 10.10 | x64 | no test coverage | | ||
| SmartOS | Experimental | >= 15 | x86, x64 | | | ||
| Linux (musl) | Experimental | musl >= 1.0 | x64 | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kernel?
c133999
to
83c7a88
Compare
@rvagg is it ok if I land this ? I have a change I'd like to make but at this point probably best to PR once it lands. |
BUILDING.md
Outdated
| FreeBSD | Tier 2 | >= 10 | x64 | | | ||
| GNU/Linux | Tier 2 | kernel >= 4.2.0, glibc >= 2.19 | ppc64be | | | ||
| GNU/Linux | Tier 2 | kernel >= 3.13.0, glibc >= 2.19 | ppc64le | | | ||
| AIX | Tier 2 | >= 6.1 | ppc64be | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please change to
+| AIX | Tier 2 | >= 6.1 TL09 | ppc64be | |
I see there are still changes that need to be incorporated so instead of looking to land added my requested change as an additional line comment |
Bump! We should really get this info published. @rvagg Do you have the bandwidth to fix up the handful of nits on this?(Maybe consider landing as-is and people can submit their own PRs to fix up the nits? |
bumping again.... |
81d59d2
to
a3564c5
Compare
fixed most nits, ptal @Trott @jbergstroem @mhdawson @claudiorodriguez @ChALkeR @Nibbler999 @ChALkeR: re musl kernel version, do you have a suggestion because my thought is just "whatever kernel version you're using with musl!" i.e. assuming musl is the lowest common denominator so kernel doesn't need to be specified. sorry for the big delay, I went looking for this list in our codebase to reference for someone and couldn't find it and eventually found this PR I'd been neglecting! |
BUILDING.md
Outdated
| FreeBSD | Tier 2 | >= 10 | x64 | | | ||
| GNU/Linux | Tier 2 | kernel >= 4.2.0, glibc >= 2.19 | ppc64be | | | ||
| GNU/Linux | Tier 2 | kernel >= 3.13.0, glibc >= 2.19 | ppc64le | | | ||
| AIX | Tier 2 | >= 6.1 | ppc64be | | | ||
| AIX | Tier 2 | >= >= 6.1 TL09 | ppc64be | | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate >=
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, fixed
Smartos needs to be >= 15 |
@bnoordhuis: do we also change gcc to 4.8.5? |
I'm discussing SmartOS build and runtime requirements in nodejs/build#628. As a result, it's possible that I'll need to submit an update to this table soon. One change that might be safe to make at this point for SmartOS is to update the
and add the following in the
@jbergstroem What do you think? I'm fine with submitting a follow-up PR as soon as the discussion in nodejs/build#628 reaches consensus though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Other than @jbergstroem suggested changes I'd vote we land and then update as changes are required. |
@rvagg we discussed in the last build meeting as wanting to land this sooner than later. I'm happy to grab and incorporate @jbergstroem comments. Or possibly I could just submit a PR against your repo with them? |
cc/ @shellberg |
Thanks @gibfahn In which case, at tier 2, we should potentially revise the AIX position to ">= 7.1 TL3", and drop AIX 6.1 >=6.1 TL09 to a tier 3/experimental status, since it will continue to build and pass tests as it does today and continues to be a legitimate build target; but it may prove harder to maintain the community CI/test infrastructure going forward and the emphasis should reasonably switch to AIX 7.1 (or later) capabilities for 'master' and future 'current' releases. |
Internal discussion so far is that we need to continue to build on AIX 6.1 and support it for the rest of the life of Node version 4.X and Node version 6.X (despite official end of life customers can still get support and use it in some cases), but for Node version 8.x move the officially supported level up to 7.X. Since our build resources are limited that means leaving the builds as is for now, using AIX 6.1 but when we cut Node version 8 we will likely list 7.1 as the minimum level to allow us to change that later on in its lifespan. |
I talked to Rod and he agreed I could take over getting this landed. Created new PR which I think addresses outstanding comments: https://github.com/nodejs/node/pull/11872/files. @jbergstroem, @misterdjules, @silverwind , @bnoordhuis , @claudiorodriguez , @ChALkeR, @Trott can you head over to review/comment there. @ChALkeR, I'd suggest we address adding the kernel level if you think we need to list something for that in a follow on PR. My goal is to get this landed so that when we branch for Node version we have something to PR changes specific to that release (for example moving support AIX level to 7.1+) |
Closing this in favor of the new stuff that has landed |
Continuing on from nodejs/build#488 (comment), this was punted to the Build WG a month or so ago, the Build WG came up with the list, tossed up the proposal last week, got some feedback and here it is for the CTC to consider now. On the agenda for this week's @nodejs/ctc meeting.