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

ansible: use gcc 4.9 on Ubuntu 14.04 #797

Closed
wants to merge 2 commits into from
Closed

ansible: use gcc 4.9 on Ubuntu 14.04 #797

wants to merge 2 commits into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Jul 16, 2017

Currently Ubuntu 14.04 machines use gcc 4.8 while Node.js requires at least gcc 4.9. See #762.

I was originally planning to make changes for all affected platforms in the same PR but it's taking longer than expected.

This PR depends on #735.

@jbergstroem
Copy link
Member

Thanks for this! Will review.

@mhdawson
Copy link
Member

@jbergstroem can you remind me if for releases we build with a later compiler or stick to the one supported by the release. I'm just thinking this could affect the compiler on the release machines for PPC and s390 which in turn could affect the binaries for 4.x and 6.x

@chrislea
Copy link

I hate to be the bearer of bad news @seishun and @jbergstroem, but I'm pretty sure if you use the ubuntu-toolchain-r/test repository to install gcc 4.9, it will update libstdc++ on the target machine when you do it. The result will be binaries that don't (or at least may not) work on a Trusty machine unless the end user has also added that PPA and installed the new compiler themselves. I'd strongly suggest checking on this before putting a lot of effort in going down this road.

@rvagg
Copy link
Member

rvagg commented Jul 21, 2017

^ that should be pretty easy to test inside Docker if someone has time, run a 14.04 container and mount your local node source directory, install buildessential and the ubuntu-toolchain-r PPA and compile. Then quit and run it again and see if the ./node executable works inside the new container without the PPA attached. If you need slightly more advanced testing then install buildessential and curl and do a curl https://deb.nodesource.com/test | bash, that's my node testing tool that'll run through some basic functionality, including compiling a native addon and give you a success or fail mark.

@chrislea
Copy link

I'm going to be up late waiting on other compiles to finish, so I went ahead and tested this on ec2 nodes. Unfortunately I was correct. You can essentially see the problem from this:

ubuntu@ip-172-30-25-156:~$ apt-cache policy libstdc++6
libstdc++6:
  Installed: 4.8.4-2ubuntu1~14.04.3
  Candidate: 7.1.0-5ubuntu2~14.04
  Version table:
     7.1.0-5ubuntu2~14.04 0
        500 http://ppa.launchpad.net/ubuntu-toolchain-r/test/ubuntu/ trusty/main amd64 Packages
 *** 4.8.4-2ubuntu1~14.04.3 0
        500 http://us-west-2.ec2.archive.ubuntu.com/ubuntu/ trusty-updates/main amd64 Packages
        500 http://security.ubuntu.com/ubuntu/ trusty-security/main amd64 Packages
        100 /var/lib/dpkg/status
     4.8.2-19ubuntu1 0
        500 http://us-west-2.ec2.archive.ubuntu.com/ubuntu/ trusty/main amd64 Packages

I went ahead and compiled 8.2.1 on a Trusty instance with gcc = 4.9 from the ubuntu-toolchain-r/test PPA, then I moved the build tree over to a "pristine" Trusty instance and ran make install. After it was installed and I tried to invoke node I got:

ubuntu@ip-172-30-31-229:~$ which node
/usr/local/bin/node
ubuntu@ip-172-30-31-229:~$ node -v
node: /usr/lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.20' not found (required by node)

So, I don't think using the ubuntu-toolchain-r/test repository is a viable option here.

@seishun
Copy link
Contributor Author

seishun commented Jul 21, 2017

Are Ubuntu 14.04 machines used for building public releases? If they are used just for CI, then why does it matter?

@rvagg
Copy link
Member

rvagg commented Jul 21, 2017

sorry, should have thought more clearly about this, we build public releases for 8.x+ on CentOS6 using devtoolset3 (I think!), and I don't believe we've had to upgrade the compiler there (have we?)

@seishun
Copy link
Contributor Author

seishun commented Jul 21, 2017

CentOS6 machines do indeed require an upgrade too (they use gcc 4.8 I think). I was going to work on that but am blocked by #801.

Are release machines separate from CI machines?

@seishun
Copy link
Contributor Author

seishun commented Aug 29, 2017

Rebased on master now that #735 has landed.

refack

This comment was marked as off-topic.

@joaocgreis
Copy link
Member

(@seishun yes, release machines are separate from test CI machines. They run connected to an instance of Jenkins that's private for releasers and jenkins-admins.)

Change seems good, I will run a test.

The question about CentOS6 looks pertinent though. We have committed to this table: https://github.com/nodejs/node/blob/master/BUILDING.md#supported-platforms-1 (change to version branches to see the table for other node versions) . So, if we update gcc, will we break support? Even if we add a new machine to build the next semver major version of node, we'll have to keep the old machines until the versions that they build go out of LTS support. Should this PR still land if we can't update CentOS6?

@seishun
Copy link
Contributor Author

seishun commented Aug 31, 2017

@joaocgreis

Should this PR still land if we can't update CentOS6?

Can't we? I just built node with gcc 6.3.1 on CentOS 6 (using my PR #809), then moved the binary to a pristine CentOS 6 instance and it ran successfully.

@joaocgreis
Copy link
Member

joaocgreis commented Aug 31, 2017

Tested, this works perfectly. This can land as soon as the issue with compatibility is resolved.

@nodejs/build I don't feel qualified to decide about Linux compiler updates alone, can you please take a look? According to @seishun , the update does not break compatibility, but I'm not familiar with all the details.

@chrislea
Copy link

Sorry I'm a little late coming back to this thread @seishun and @joaocgreis.

I've tested this extensively. Building Node with gcc = 6.3.1 from devtoolset-6 works just fine. Meaning, the resulting binaries run just fine on a "pristine" CentOS 6. The same binaries run just fine on every other distro I've tested on, which is every currently supported version of CentOS / RHEL, Fedora, Debian, and Ubuntu with the exception of Debian Wheezy which I think we've decided we don't worry about anymore.

There are two (really 1.5) concerns here.

The 0.5 is the concern about building a binary add-on with an older compiler may have some incompatibility issue due to the C++ ABI not being stable. So if you take those built binaries, put them on say CentOS 6, and then build an add-on there with whatever the native compiler there is, could there be a problem. I've been told by people who know a ton more about compilers than myself that this isn't a concern practically.

The other concern is that devtoolset-* is 64bit only. If this is the new plan, we drop 32bit support I think. Personally I'm just fine with that, but I thought I should bring it up.

@seishun
Copy link
Contributor Author

seishun commented Aug 31, 2017

The 0.5 is the concern about building a binary add-on with an older compiler may have some incompatibility issue due to the C++ ABI not being stable. So if you take those built binaries, put them on say CentOS 6, and then build an add-on there with whatever the native compiler there is, could there be a problem. I've been told by people who know a ton more about compilers than myself that this isn't a concern practically.

Not an issue according to @bnoordhuis nodejs/node#13466 (comment)

The other concern is that devtoolset-* is 64bit only. If this is the new plan, we drop 32bit support I think. Personally I'm just fine with that, but I thought I should bring it up.

No need to drop 32-bit support I think, we just wouldn't test node on 32-bit CentOS 6. +1 for dropping public 32-bit builds.

(Alternatively, we could build 32-bit builds on newer OS, but I suspect that such builds wouldn't work on CentOS 6, which would be kind of confusing)

@gibfahn
Copy link
Member

gibfahn commented Sep 12, 2017

Result of WG discussion was that this PR is fine to land, we don't build on Ubuntu so this only affects test boxes.

@gibfahn
Copy link
Member

gibfahn commented Sep 12, 2017

I've tested this extensively. Building Node with gcc = 6.3.1 from devtoolset-6 works just fine. Meaning, the resulting binaries run just fine on a "pristine" CentOS 6. The same binaries run just fine on every other distro I've tested on, which is every currently supported version of CentOS / RHEL, Fedora, Debian, and Ubuntu with the exception of Debian Wheezy which I think we've decided we don't worry about anymore.

@rvagg you said you were going to confirm this, but it sounds like we should be okay here in terms of devtoolset-6 builds running on stock CentOS 6 machines.

@gibfahn
Copy link
Member

gibfahn commented Sep 12, 2017

The other concern is that devtoolset-* is 64bit only. If this is the new plan, we drop 32bit support I think. Personally I'm just fine with that, but I thought I should bring it up.

@chrislea are you saying we can't build 32-bit binaries at all with devtoolset-6?

refack

This comment was marked as off-topic.

@chrislea
Copy link

@gibfahn That is correct. All of the devtoolset-* packages for RHEL 6 / CentOS 6 are 64 bit only. So if we go down this road, it means we are dropping support for 32bit x86.

@seishun
Copy link
Contributor Author

seishun commented Sep 13, 2017

@chrislea Could you clarify why this requires dropping support for 32-bit? Looking at https://ci.nodejs.org/job/node-test-commit-linux/12372/, there are a bunch of other 32-bit Linux machines where node is being tested. Among them is Ubuntu 14.04, which definitely can run gcc 4.9.4.

@chrislea
Copy link

@seishun sorry I should be more explicit.

The current strategy as I understand it for building the released binary tarballs for x86 is to build things on CentOS 6 since that's the "oldest" distro we support. Those binaries will then work on any "newer" distro because we have forward compatibility with libc6 and libstdc++. If you build 32bit x86 binaries on say Ubuntu 14.04, they won't work on anything "older" than that, which would include CentOS 6.

So if we're willing to drop support for 32bit CentOS 6 and Debian Wheezy, and also build the 32bit x86 binaries on a different distro than we build the 64bit x86 binaries on, then it can happen. At that point, things are somewhat messier and personally I'd be wondering if it was worth the effort, but I also don't have to do the work to make it all happen so ¯_(ツ)_/¯ .

@gibfahn
Copy link
Member

gibfahn commented Sep 13, 2017

So the discussion in the Build WG yesterday was operating under the impression that we could continue to build 32-bit binaries with this change, as we can't we'll have to rethink. I don't think dropping CentOS 6 is likely to just happen, in fact I suspect that dropping 32-bit builds is more likely. I'll be raising a separate issue to discuss that anyway.

@chrislea
Copy link

Sounds good @gibfahn, please let me know if there's anything I can do to help.

@seishun
Copy link
Contributor Author

seishun commented Oct 25, 2017

@refack status?

@rvagg
Copy link
Member

rvagg commented Nov 2, 2017

ok, so I've run this on the x64 and x86 machines and merged this commit into master

However we have a bunch of non-x machines that I'm not comfortable running this on. I started doing it on the ppc machines but there were too many changes being made (like setting hostname) that make me think that the new ansible scripts haven't been run on those before, so I'm not comfortable pushing forward. Also there's the xgene machines and I'm going to bet that ubuntu-toolchain-r doesn't have a gcc4.9 for arm64 ubuntu14.04 because that's a pretty unique combo (I've had a heap of trouble with Java on these). Plus I'd really like to yank those xgene machines anyway so ignoring them is probably OK for now.

So, @mhdawson (or someone else IBM?), how safe do you think it is to run the current ansible scripts against the ppc machines? Is adding the ubuntu-toolchain-r ppa actually going to give us what we need on ppc le & ppc be? Launchpad says that these are built for "powerpc" but is that even what we need? Should we just exclude ppc ubuntu14.04 from all of this?

I've had similar problems with the Java stuff I did in #964, I haven't put an arch restriction on that but I'm guessing webupd8 isn't available for ppc either, like it's not for arm64 ubuntu 14.04, I just haven't even bothered running it on those.

@chrislea
Copy link

chrislea commented Nov 2, 2017

Tossing my $0.02 back in here again (if this is getting annoying somebody please tell me and I'll stop).

I don't think using the ubuntu-toolchain-r stuff should ever be an option. It's a big nasty Pandora's box and will cause many, many more problems than it solves.

I am increasingly feeling like it's completely reasonable to just stop shipping 32bit Intel binaries as of Node 9.x. All of the major distros are moving away from even providing 32bit builds, and just discussing it is draining a lot of time for people on the build WG.

I'm not saying that I think we should stop supporting 32bit Intel builds. I'm just saying that "supporting" for that arch should simply mean that we make sure the build works and passes tests on newer distros where people don't have to jump through a bunch of hoops just to get a modern compiler toolchain installed and working.

I suspect most of the live 32bit Intel things out there that are running Node are devices like NAS appliances and the like, and I've never seen any of them that don't build their own binaries. So as long as we know that Node can be built on a reasonably modern 32bit box, I don't think we're short changing anybody.

/ $0.02

@seishun
Copy link
Contributor Author

seishun commented Nov 2, 2017

@rvagg

ok, so I've run this on the x64 and x86 machines and merged this commit into master

Finally! Thanks for taking the time.

Should we just exclude ppc ubuntu14.04 from all of this?

I'm fine with that as long as it's also excluded from CI jobs on master.

@chrislea

I don't think using the ubuntu-toolchain-r stuff should ever be an option.
I am increasingly feeling like it's completely reasonable to just stop shipping 32bit Intel binaries as of Node 9.x.

Could you clarify how these two statements are related? gcc needs to be updated on 64-bit Ubuntu 14.04 as well.

@chrislea
Copy link

chrislea commented Nov 2, 2017

@seishun sure, I'll do my best.

Using ubuntu-toolchain-r is fine if you just want to make sure that Node will build on Ubuntu 14.04. But if you actually distribute those binaries, they won't work on any end user 14.04 box unless they also have the ubuntu-toolchain-r PPA enabled and they upgrade their libstdc++ when they install Node. I don't think we want to distribute binaries that require a distro to do a libstdc++ upgrade in order to use them.

The bit with regard to dropping 32bit builds is because if we only care about 64bit, then we can build 64bit Node with the devtoolset-6 toolchain on CentOS 6, and those binaries will "just work" for every newer distro that we care about.

@seishun
Copy link
Contributor Author

seishun commented Nov 2, 2017

@chrislea According to #797 (comment), Ubuntu 14.04 isn't used for building public releases.

@chrislea
Copy link

chrislea commented Nov 2, 2017

Ah my bad @seishun! Please disregard that bit.

@seishun
Copy link
Contributor Author

seishun commented Nov 20, 2017

@nodejs/build ping... what's the plan with the ppc machines? If gcc isn't going to be upgraded to 4.9.4 or newer on all CI machines (at least on master), then there is no point in upgrading it on any of them.

@refack
Copy link
Contributor

refack commented Nov 29, 2017

So if while we are not 100% on the ansible scripts, I updated the 4 PPCs to GCC4.9, see how this works out.

add-apt-repository ppa:ubuntu-toolchain-r/test
apt-get update
apt-get purge gcc-4.8
apt-get autoremove
apt-get install gcc-4.9 g++-4.9 cpp-4.9
update-alternatives --install /usr/bin/gcc gcc /usr/bin/gcc-4.9 40
update-alternatives --config gcc
update-alternatives --install /usr/bin/cpp cpp /usr/bin/cpp-4.9 40 
update-alternatives --config cpp
update-alternatives --install /usr/bin/g++ g++ /usr/bin/g++-4.9 40
update-alternatives --config g++

adding

update-alternatives --install /usr/bin/cc cc /usr/bin/gcc 30
update-alternatives --set cc /usr/bin/gcc
update-alternatives --install /usr/bin/c++ c++ /usr/bin/g++ 30
update-alternatives --set c++ /usr/bin/g++

and for the BE machines

apt-get install gcc-4.9-multilib g++-4.9-multilib

@refack refack closed this Nov 29, 2017
@seishun seishun deleted the ubuntu1404-gcc49 branch November 29, 2017 07:34
@seishun
Copy link
Contributor Author

seishun commented Nov 29, 2017

Looks good: https://ci.nodejs.org/job/node-test-commit-plinux/13557/

Although making manual changes kind of defeats the purpose of having Ansible scripts...

@mhdawson
Copy link
Member

mhdawson commented Nov 29, 2017

@refack I don't think we should have removed 4.8 on the PPC machines, as the requirement for 4.9 only applies to later versions of Node.js and we should be testing/validating with 4.8 on those earlier releases right ? What we need is to have both 4.8 and 4.9 installed so that we can test later versions with 4.9 and earlier versions with 4.8. That is was we are going to need to do on the release machine as well. The problem is that we don't have as many PPC machines so we can't have different ones for each release.

@mhdawson
Copy link
Member

I had asked @jBarz to look at how we get both the older and the newer compiler on the machines at the same time.

@mhdawson
Copy link
Member

@refack one more request can I ask that before you do changes like this on the PPC, AIX or zLinux machines you give me a heads up first

@mhdawson
Copy link
Member

mhdawson commented Nov 29, 2017

@refack do you have the steps to revert back to 4.8 ? We can figure them out but if you have them on hand because you just updated that would be great.

@mhdawson
Copy link
Member

Talking with Gibson, he seems to remember that Ben had looked at 4.8 -> 4.9 and it was not going to break compatibility with older systems (even though in many cases an update to the compiler will). I'll talk to Ben to see if that is the case, in which case we may not need to revert.

@refack
Copy link
Contributor

refack commented Nov 29, 2017

@mhdawson Ack.
I raised the flag and asked for feedback two months ago (#797 (comment)) and only got thumbs up, so I assumed I was clear to go ahead.

As for 4.8 / 4.9 side-by-side, I had the same thought since my last message (old node version compatibility). It should be easy restore (making 4.9 stand alone was the more tricky part).
IMO we need to keep some coverage of 4.8 for older versions of node, to make sure we don't introduce non compatible code.

@refack
Copy link
Contributor

refack commented Nov 29, 2017

P.S. I only updated the test machines, the release machines were not touched. This way we can verify binary compatibility.

@mhdawson
Copy link
Member

@refack the issue with Release at 4.8 and test is 4.9 is that things may look ok in the test infra but then fail when we go to do a release....

@mhdawson
Copy link
Member

Generally we need to figure out how to have multiple versions of the compiler on the same machine and use the appropriate one for each release. @jBarz is working on that. Was there something that was broken and needed 4.9 right now ?

@mhdawson
Copy link
Member

Looking at the earlier comments the libstdc++ from 4.9 should be compatible with the one from 4.8 so we don't believe it will affect compatibility.

What I'd like to do is

  1. On BE just revert. We are already working on dropping BE from builds for 8.X and later. (I've removed from main download page, next step is to stop building in releases).

  2. on LE leave it at 4.9, validate that the binaries run on machines with gcc 4.8 installed and if ok update the release machine to match.

Opened issue #1020 to track

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.

8 participants