nodejs / node-convergence-archive

Archive for node/io.js convergence work pre-3.0.0
https://github.com/nodejs/io.js/issues/2327
Other
1.84k stars 96 forks source link

[Converge] Build related commits #27

Closed jasnell closed 9 years ago

jasnell commented 9 years ago

There are a number of build related commits that need to reconciled. /cc @nodejs/build @misterdjules

jasnell commented 9 years ago

See: https://github.com/nodejs/node/issues/26 ... The Intl work also impacted the Build.

Fishrock123 commented 9 years ago

src: make build pass with GCC < 4.5

iirc this isn't possible with the v8 we are using, cc @bnoordhuis

bnoordhuis commented 9 years ago

Correct. 4.8 is the absolute minimum.

jasnell commented 9 years ago

Another: c0766eb1a4dac9d9b555be271ef84ad401ba91b0 build: fix use of strict aliasing - https://github.com/joyent/node/commit/c0766eb1a4dac9d9b555be271ef84ad401ba91b0

Fishrock123 commented 9 years ago

Also looks like it should just be dropped, again < 4.8 isn't going to work anyways.

jasnell commented 9 years ago

@misterdjules @mhdawson ... do you know of anything that breaks without this?

Fishrock123 commented 9 years ago

Well, older systems that use GCC < 4.8 will break trying to build without having GCC upgraded.

bnoordhuis commented 9 years ago

And, FWIW, the library that was the source of the -Wstrict-aliasing warnings, src/queue.h, was removed in nodejs/io.js@7e779b459367f071366699550ea0162b63516564.

jbergstroem commented 9 years ago

Clang could be a simpler install through package managers on those systems. Avoiding the gcc 4.8 is pretty much impossible if we want to use recent/stable v8's.

jasnell commented 9 years ago

Ok, then I think this one is largely decided then. It would require a revert of the io.js changes in order to land. @misterdjules @cjihrig @trevnorris @orangemocha @mhdawson : if any of you think this is going to be a problem, we'll need to make the case to revert before we can proceed. Given that, I'm going to close this issue. We can reopen if new information is presented.

Fishrock123 commented 9 years ago

@jasnell Hmmm, isn't the 2nd commit still valid? The windows 32 one?

jasnell commented 9 years ago

Ah! missed that one. Perhaps, reopening until we can confirm

bnoordhuis commented 9 years ago

joyent/node@6168fe6 LGTM FWIW. It doesn't apply cleanly but you can get it to merge with a little nudge.

misterdjules commented 9 years ago

Added more info about https://github.com/joyent/node/commit/6168fe6720650052728f3e78a495b723f0b41ce3 in https://github.com/nodejs/node/pull/44#issuecomment-108045509.

Regarding https://github.com/joyent/node/commit/59265264d56cfb7ced87f9b3666ec570152c5f47 and https://github.com/joyent/node/commit/c0766eb1a4dac9d9b555be271ef84ad401ba91b0, the goal for these changes was to be able to run node on Linux systems with older C++ runtimes (like CentOS 5.7 and early 6.x, I don't remember the specific version numbers).

The way io.js solved this was to use the RHEL developer toolset to build a node binary for Linux. This toolset basically makes node links against a C++ runtime that is old enough to run on Linux systems with older C++ runtimes, and adds newer C++ runtime features with a static library linked directly into the node binary. I think this approach is better and I created https://github.com/joyent/node-infra/issues/4 to use it in joyent/node too. @mhdawson expressed some important concerns though here: https://github.com/joyent/node-infra/issues/4#issuecomment-100032893.

In other words: as long as node runs on Linux systems in wide usage that ship an older C++ runtime, we don't need these two changes. We'll need input from @mhdawson to determine what needs to be done on IBM platforms.

mhdawson commented 9 years ago

As stated earlier since the converged stream will have a newer level of V8 and V8 requires a newer compiler the converged stream will have a dependency on a later compiler so leaving out the first PR is not an issue.

I'll still concerned though that we don't have a full answer to the problem. Ignoring the issue of IBM based platforms, based on my understanding end users will only be able to build binaries for xlinux that run on older systems by one of the following:

1) using Red Hat Enterprise Linux with the Develper toolset 2) using a using the toolset here http://linux.web.cern.ch/linux/devtoolset/, were it says in bold red "TEST PURPOSE ONLY" on centos (not sure about this).

So for example, if I'm an ubuntu shop running ubuntu 12 I'd either have to install a different platform or figure it out for myself how I could build binaries that would run on ubuntu 12.

rvagg commented 9 years ago

Ubuntu 12.04 toolchain is fairly simple via a PPA: https://github.com/nodejs/build/blob/master/setup/ubuntu12.04/ansible-playbook.yaml#L25

Ubuntu 10.04 was just as simple: https://github.com/nodejs/build/blob/6b3867b99f555f3f1bb0b84c81ea20db2ca343b6/setup/ubuntu10.04/ansible-playbook.yaml#L25

Wheezy is a problem, although it's common enough that workarounds are appearing. For the ARMv7 machines I've managed to get gcc4.8 via default backport channels but I can't tell you if that goes for non-arm release lines too. Early on I created a procedure for compiling gcc4.8 on Wheezy, see https://github.com/nodejs/build/tree/master/setup/debian-wheezy-gcc for that.

It's actually easier to compile on non-RHEL/CentOS distros of the same age as it is on RHEL because of the hacky way you have to use SCLs.

Also, I read "TEST PURPOSE ONLY" in the same way as I read "DO NOT MACHINE WASH" on clothing labels, it's all about CYA.

mhdawson commented 9 years ago

Having the info documented on how to get the toolchain you'll need for a given distro level would help. The other question is if we know whether binaries built with gcc48 on ubuntu 12.04 will run on ubuntu 10.04 ? The concern was not so much that you could not get the toolchain to build on a given distro level (but as you mention for Wheezy, its not always straight forward, so documentation would be helpful), but that resulting binaries might not run on earlier distro levels without having to update libstdc++ levels or some other components on the backlevel distro. That's the issue that dev toolset addresses.

rvagg commented 9 years ago

So 12.04 binaries won't run on 10.04 but 10.04 is out of LTS so we've moved on anyway. The binaries we produce on CentOS5 will work on other CentOS 5 / RHEL5 / Wheezy-era distros without any additional extras. We build the ARM binaries on Wheezy-based distros for the same reason.

I think I've mentioned this before but it's worth saying again: any use of C++ features so far in V8 (and Node where it's crept in) are limited to those that don't require linking to libstdc++, so all that matters for the target distro is the version of libc. All that matters is where the binary is compiled and CentOS 5 and Wheezy represent the oldest reasonably supported distros out there.

To prove the point, here's a copy of what I've just run against a virgin CentOS 5 box:

$ ssh root@45.55.74.162
The authenticity of host '45.55.74.162 (45.55.74.162)' can't be established.
RSA key fingerprint is cc:35:5b:92:77:ad:e4:17:d2:da:93:52:26:3c:8a:ca.
Are you sure you want to continue connecting (yes/no)? yes
Warning: Permanently added '45.55.74.162' (RSA) to the list of known hosts.
[root@centostest ~]# uname -a
Linux centostest 2.6.18-308.1.1.el5 #1 SMP Wed Mar 7 04:16:51 EST 2012 x86_64 x86_64 x86_64 GNU/Linux
[root@centostest ~]# uptime
 22:10:59 up 0 min,  1 user,  load average: 0.44, 0.11, 0.04
# curl -sL https://iojs.org/dist/latest/iojs-v2.3.3-linux-x64.tar.gz | tar -zx --strip-components=1 -C /usr/local/
[root@centostest ~]# node -v
v2.3.3
[root@centostest ~]# node -pe process.versions
{ http_parser: '2.5.0',
  node: '2.3.3',
  v8: '4.2.77.20',
  uv: '1.6.1',
  zlib: '1.2.8',
  ares: '1.10.1-DEV',
  modules: '44',
  openssl: '1.0.2c' }
rvagg commented 9 years ago

I think this issue boiled down to the vcbuild.bat change but I believe that's redundant now thanks to the Intl support by @srl295: https://github.com/nodejs/node/commit/1f12e032

I've filed a ticket to make sure Jenkins is doing the right thing by v4, currently I don't think it is: https://github.com/nodejs/node/issues/2516