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

meta: 32 bit linux is experimental #16723

Merged
merged 1 commit into from
Nov 13, 2017
Merged

Conversation

refack
Copy link
Contributor

@refack refack commented Nov 3, 2017

Refs: nodejs/build#885
Refs: nodejs/nodejs.org#1446

[update]
This PR is the Build WG making a recommendation. Detailed discussion is in nodejs/build#885

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

meta

@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 Nov 3, 2017
@seishun
Copy link
Contributor

seishun commented Nov 3, 2017

Does dropping public builds for a platform automatically downgrade it to experimental? I mean, we still run CI on it and all.

@refack
Copy link
Contributor Author

refack commented Nov 3, 2017

Does dropping public builds for a platform automatically downgrade it to experimental? I mean, we still run CI on it and all.

I don't think automatically, but that was the consensus in nodejs/build#885 (comment):
image

@addaleax
Copy link
Member

addaleax commented Nov 3, 2017

I’m not sure this is the right place, but am I the only one uncomfortable dropping what is, according to the metrics, the architecture with the second-most monthly downloads by far?

@refack
Copy link
Contributor Author

refack commented Nov 3, 2017

I’m not sure this is the right place, but am I the only one uncomfortable dropping what is, according to the metrics, the architecture with the second-most monthly downloads by far?

IMHO this is the right place. This PR is the build WG making a recommendation.
There was some discussion in nodejs/build#885, but for the near future there are now hard limitations to do this, but there are approaching:

  1. CentOS has dropped official 32 bit support for CentOS 7 (and tooling updates for 32 bit CentOS 6 has already moved to "community support")
  2. Canonical dropped official 32 bit artifacts for Ubuntu 17.10
  3. Our DL numbers show only a residual 32 bit download demand (at about 5%). The stagnation of the 32bit absolute numbers was interpreted as mostly CI scripts.
    image
    https://nodejs.org/metrics/summaries/arch.png

@addaleax addaleax requested a review from a team November 3, 2017 18:25
@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2017

I'm wondering if those "unknown" values are 32-bit? I am not sure we can safely assume they aren't?

I think we should consider supporting 32-bit for at least as long as V8 does.

@seishun
Copy link
Contributor

seishun commented Nov 3, 2017

@mscdex

I am not sure we can safely assume they aren't?

Contains a data column per distributed architecture, including unknown where the architecture cannot be determined (source or header tarballs).

I think we should consider supporting 32-bit for at least as long as V8 does.

It will still be supported. You will just have to build it yourself.

@refack
Copy link
Contributor Author

refack commented Nov 3, 2017

(source or header tarballs).

So the plan is to continue CI on 32 bit linux exactly to make sure "source and headers" 32 bit support will not rot.

@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2017

It will still be supported. You will just have to build it yourself.

I don't get the reasoning for this. If we are still going to be testing it in CI anyway, why not just make the build artifact available to the public?

@seishun
Copy link
Contributor

seishun commented Nov 3, 2017

@mscdex Because that artifact wouldn't necessarily work everywhere, see for example nodejs/build#797 (comment). Making a build that works everywhere is more complicated than just building and testing on the same machine.

Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

LGTM as per the discussion in nodejs/build.

@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2017

@seishun I don't see that as being any different than the situation with the currently available 32-bit (or any architecture really) binaries? If we're just keeping one 32-bit machine, then just build against whatever the oldest glibc we support on that machine, then make those build artifacts available. It just seems silly for us to test against 32-bit then simply discard the perfectly usable built binary because we don't want to simply tar it up.

@gibfahn
Copy link
Member

gibfahn commented Nov 3, 2017

Marking tsc-review, it would be good to have more of the TSC weigh in.

Note that the reason to do this now is so we have six months to test it out before 10.x. Hopefully the Current release cycle will give us time to gather feedback about how many people will be affected.

This is definitely something we could "revert" if there is still demand. We could also partially revert, and support a smaller set of platforms on x86 Linux. Removing is semver-major, re-adding is semver-minor.

@seishun
Copy link
Contributor

seishun commented Nov 3, 2017

@mscdex

I don't see that as being any different than the current situation with the available 32-bit (or any architecture really) binaries?

You can read more about the current situation in nodejs/build#885. In short: 32-bit Linux releases are currently built on a CentOS 6 machine, but it has gcc 4.8, which is not supported. But the packages with newer gcc versions are not available on 32-bit CentOS 6.

@addaleax
Copy link
Member

addaleax commented Nov 3, 2017

But the packages with newer gcc versions are not available on 32-bit CentOS 6.

Is compiling them ourselves from source, for the CI machine, an option?

@seishun
Copy link
Contributor

seishun commented Nov 3, 2017

Is compiling them ourselves from source, for the CI machine, an option?

Possibly, but that's a lot of work that no one in Build WG seems to be willing to do. See the discussion around nodejs/build#809 (comment).

@refack
Copy link
Contributor Author

refack commented Nov 3, 2017

But the packages with newer gcc versions are not available on 32-bit CentOS 6.

Is compiling them ourselves from source, for the CI machine, an option?

A glibc maintainer advised against that, as it will not solve need to distribute a new glibc. nodejs/build#809 (comment)

@refack
Copy link
Contributor Author

refack commented Nov 3, 2017

It just seems silly for us to test against 32-bit then simply discard the perfectly usable built binary because we don't want to simply tar it up.

It's more an issue of supporting. That binary may simply not work on all 32 bit linux distros out there. Researching this and trying to figure out all the implications has already required substantial effort from the Build WG, and we are still without clear conclusions. So we recommend downgrading 32 bit linux support in order to focus resources on more significant platforms.

@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2017

In short: 32-bit Linux releases are currently built on a CentOS 6 machine, but it has gcc 4.8, which is not supported. But the packages with newer gcc versions are not available on 32-bit CentOS 6.

Ok, so then here are two solutions:

  1. Use clang instead of gcc on CentOS 6. From some brief research, CentOS 6 has clang 3.4.2, which meets our current build requirements.
  2. Use a different 32-bit machine. Debian/Ubuntu is pretty popular and has newer compilers available.

@mscdex
Copy link
Contributor

mscdex commented Nov 3, 2017

So we recommend downgrading 32 bit linux support in order to focus resources on more significant platforms.

Downgrading the support is one thing, but throwing away the binary away instead of making it available to those who can use it is silly IMHO. If you want, put the glibc version in the tarball filename or something.

@gibfahn
Copy link
Member

gibfahn commented Nov 3, 2017

I think we should consider supporting 32-bit for at least as long as V8 does.

Well, really at most right? If V8 stop supporting 32-bit, then we will by default.

I don't get the reasoning for this. If we are still going to be testing it in CI anyway, why not just make the build artifact available to the public?

We might have to stop testing in CI, or change what levels we can test, during 10.x's timeframe.

If we're just keeping one 32-bit machine, then just build against whatever the oldest glibc we support on that machine, then make those build artifacts available.

Whatever we're Tier 1 supporting in April will be what we are supporting till April 2021 (in 10.x). We could definitely consider making binaries available for people to use, experimental doesn't mean "we can't do builds", it means "you shouldn't rely on this sticking around for the whole release line".

Definitely open to still shipping some binaries though, maybe we could continue to do nightlies on a different machine, that way people could get node builds while knowing they shouldn't be relied on.

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

I'm generally +1 on this. I don't know about calling it "Experimental"... perhaps we need a new term.

@gireeshpunathil
Copy link
Member

Given

  • the available data suggests consumptions in 32 bit is stagnant
  • the underlying tools to support the build & release are becoming obsolete
  • the platofrm itself is being seriously considered to be shelved by its vendors

This change, with pre-requisites to make sure we are able to build again if need be, looks reasonable to me.

@rvagg
Copy link
Member

rvagg commented Nov 8, 2017

If someone wants to look deeper into the numbers, grab a month or so from the bottom of the list of files @ https://nodejs.org/metrics/logs/ and see what versions of Node that the x86 builds are being downloaded for. A simple breakdown of semver-major versions would be really helpful here.

@refack
Copy link
Contributor Author

refack commented Nov 10, 2017

AFAICT there are no strong objections to the documentation change, as long as we keep testing, and releasing the binary for as long as it's simple to do.

If anyone objects, please state so, as I'm planning to land this.

@seishun
Copy link
Contributor

seishun commented Nov 10, 2017

From the definition of Experimental:

May not compile reliably or test suite may not pass.

It's not true since we run CI on three 32-bit Linux machines on master. Moreover, one of those machines runs Ubuntu 16.04, and Ubuntu 16.04 is going to be supported until April 2021. Which means we can continue running CI on 32-bit Linux until the end of Node 10.x LTS (April 2021) without any effort from the Build WG. (correct me if I'm missing something)

And if we want to explicitly drop support for 32-bit Linux during Node 10.x lifetime (by intentionally landing changes that break 32-bit Linux), then Experimental doesn't cut it - it should be removed from the list entirely.

So I'm -1 on this, but I would agree with downgrading the support level to Tier 2.

edit: never mind, see below.

Copy link
Contributor

@seishun seishun left a comment

Choose a reason for hiding this comment

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

Making -1 explicit.

@gibfahn
Copy link
Member

gibfahn commented Nov 10, 2017

It's not true since we run CI on three 32-bit Linux machines on master.

I don't think that is adequate coverage to say we support kernel >= 2.6.32, glibc >= 2.12. Debian 8 is kernel 3.16.43, and glibc is 2.25.

And if we want to explicitly drop support for 32-bit Linux during Node 10.x lifetime (by intentionally landing changes that break 32-bit Linux) then Experimental doesn't cut it - it should be removed from the list entirely.

Why? It's experimental as long as we're making some effort to support it. We aren't currently planning to intentionally break x86, if we do then at that point it should be removed.

Overall I think it's not semver-major to break an experimental platform, but it is for Tier 1 and 2. And that means we should move x86 to experimental.

@seishun
Copy link
Contributor

seishun commented Nov 11, 2017

I don't think that is adequate coverage to say we support kernel >= 2.6.32, glibc >= 2.12. Debian 8 is kernel 3.16.43, and glibc is 2.25.

In that case, perhaps it would be reasonable to state that kernel >= 2.6.32 < 3.16.43, glibc >= 2.12 < 2.25 is Experimental? (that is, if there is someone actually willing to test patches on such outdated systems. Otherwise "unsupported" is more apt)

Why? It's experimental as long as we're making some effort to support it.

On a second thought, I agree with it if Experimental basically means "it's okay to break it, but we also welcome patches to un-break it". But I'm not sure if it's really necessary. 32-bit Linux is not an exotic platform that's hard to set up to test your changes locally, so the person who breaks it is probably the best person to fix it (that is, if we want to fix it at all).

@seishun seishun dismissed their stale review November 11, 2017 11:09

(withdrawn)

@gibfahn
Copy link
Member

gibfahn commented Nov 11, 2017

P.S. How do I rescind the GitHub rejection without approving?

You want Dismiss review:

image

In that case, perhaps it would be reasonable to state that kernel >= 2.6.32 < 3.16.43, glibc >= 2.12 < 2.25 is Experimental? (that is, if there is someone actually willing to test patches on such outdated systems. Otherwise "unsupported" is more apt)

I agree that it might be more accurate to split them out, but in terms of the overall message we want to send here, I think it would be clearer to just say "if you're using 32-bit Linux on Node 9.x+, don't rely on it continuing to work going forward". Sure, people can update to a newer OS level and stay on x86, but I think we'd rather suggest that they get off 32-bit x86 entirely (and if they can't, come and tell us so we can reconsider). Running CI means we can be sure it's not too hard to move it back up if there's demand.

PR-URL: nodejs#16723
Refs: nodejs/build#885
Refs: nodejs/nodejs.org#1446
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@refack refack added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 13, 2017
@refack refack merged commit dbb3c00 into nodejs:master Nov 13, 2017
evanlucas pushed a commit that referenced this pull request Nov 14, 2017
PR-URL: #16723
Refs: nodejs/build#885
Refs: nodejs/nodejs.org#1446
Reviewed-By: Gibson Fahnestock <[email protected]>
Reviewed-By: Gireesh Punathil <[email protected]>
@MylesBorins
Copy link
Contributor

Does this make sense for 6.x or 8.x?

@gibfahn
Copy link
Member

gibfahn commented Nov 17, 2017

Does this make sense for 6.x or 8.x?

Don't think so, discussion was specifically about 9.x and above.

@Trott Trott removed the tsc-review label Dec 4, 2017
@refack refack deleted the no-32-linux-for-9 branch April 26, 2018 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. doc Issues and PRs related to the documentations. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.