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

build: backport config for new CI infrastructure to v0.12 #3642

Closed
wants to merge 3 commits into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Nov 3, 2015

Once complete, this work should be fairly easily transferable to v0.10 too, I'm starting off here because it's the closest to what we have with the new infra.

The goal is to require as little modification and special-casing of our new infra setup to get the old branches to spit out builds to staging. I'm aiming to try and have an RC build out for at least 0.12 later this week, likely not with all of the items below but I'd like to get the ball rolling with test builds at least. The ideal beyond that is to finish this work within 2 weeks if possible. It's hard to predict how much work is in here and what rabbit holes there are that lay ahead (and how much time I'll actually have to shave these yaks).

TODO:

  • OSX binary tarballs, x86 and x64—Since we now only have a single build machine to do darwin tarballs because we only do 64-bit I've taken the approach of doing both 64-bit and 32-bit within the same Makefile task. This is the same as the way the .pkg is built with the universal binary being a munge of both 32-bit and 64-bit binaries so it takes a double-compile. It also means the build task we have in Jenkins for v4+ just works as is.
  • OSX .pkg, universal binary
  • Linux binary tarballs, 32-bit & 64-bit
  • SmartOS binary tarballs, 32-bit & 64-bit, this is probably going to require using the old SmartOS machines still attached to jenkins.nodejs.org, will look to @misterdjules for guidance here
  • Source tarball
  • Windows .msi, 32-bit & 64-bit, located in ./ and ./x64/, unlike the newer ./win-x86/ and ./win-x64/ that we are using now. I'll need to consult with @nodejs/build on what servers we're going to use for Windows, I think they've all been disconnected from the old infra and converted to the new stuff but we probably need to compile with an older MSVC?
  • Windows plain binaries, in ./ and ./x64/: node.exe, node.exp, node.lib, node.pdb, openssl-cli.exe, openssl-cli.pdb (we are only shipping node.exe and node.lib now, so far anyway, there have been no calls for node.pdb that I'm aware of, but we should probably stay consistent for v0.12 and v0.10).
  • Docs, note that for v0.12 and v0.10 the website was built for the ./docs/ directory and we are now only shipping ./docs/api/, will look to @nodejs/documentation and @nodejs/tsc for guidance on whether we can strip down or whether we need to stay consistent here.
  • Exclude unsupported arches and platforms—this is probably best done on Jenkins, some scripting in the job to make sure that the builds aren't attempted on the newer platforms we're shipping (arm, freebsd), we still want to use the same build job in Jenkins if possible but it has to be smart about which builds are performed.

/cc @nodejs/lts @nodejs/security

@kkoopa
Copy link

kkoopa commented Nov 3, 2015

Regarding Windows builds, you should be able to get away with using a current verion of MSVC as long as you have the old Platform SDKs installed, which should enable the correct target platform setting.

@mscdex mscdex added the build Issues and PRs related to build files or the CI. label Nov 3, 2015
@orangemocha
Copy link
Contributor

what servers we're going to use for Windows, I think they've all been disconnected from the old infra and converted to the new stuff but we probably need to compile with an older MSVC?

No need to compile with an old MSVC. We ported VS 2015 support all the way back to v0.10.

The old Windows build machines are still around if needed. They are just turned off at the moment.

@jbergstroem
Copy link
Member

smartos update: old release machine is available through CI. We need to rewrite the release script so both 32-bit and 64-bit versions are using the same machine though.

@rvagg
Copy link
Member Author

rvagg commented Nov 4, 2015

added linux support, had to import the ia32 -> x86 hack we now have in the code because we're using x86 everywhere now but the old code preferred it to be specified as ia32 when building (even though the tarball names use x86)

@rvagg
Copy link
Member Author

rvagg commented Nov 4, 2015

re excluding unsupported arches for building, I'm trying this approach in Jenkins:

if [[ \
     ( $NODE_LABELS =~ pre-1-release  && $(python tools/getnodeversion.py) =~ ^[0] ) \
  || ( $NODE_LABELS =~ post-1-release && $(python tools/getnodeversion.py) =~ ^[^0] ) \
]]; then

make ...

fi

So I've labelled release slaves with pre-1-release and post-1-release, some with both. This is only active on the binary tarball section, the Windows builder, OSX .pkg builder and source/docs builder. I'm hoping that we can get away with this.

The wastage is that the repo will still be cloned on all of the release machines, including the pi1's, but they just won't build. I'm sure @orangemocha would do something much more clever like he's done for node-test-pull-request but this is all still contained within a single job, no sub-jobs.

Additional TODOs here for the build slaves are:

  • Set up new slaves on the CentOS5 machines with appropriate toolchains, the ones we have available are all for gcc-4.8 but we need to match the old ones (I don't know exactly what that is yet)
  • Set up the old SmartOS machines, @jbergstroem has been working on this as he has access to the Joyent account and therefore the servers, I believe there's just a Java update required to make it work with how we're running Jenkins.

@rvagg
Copy link
Member Author

rvagg commented Nov 4, 2015

confirmed OSX .pkg universal binary support works:

$ file $(which node)
/usr/local/bin/node: Mach-O universal binary with 2 architectures
/usr/local/bin/node (for architecture i386):    Mach-O executable i386
/usr/local/bin/node (for architecture x86_64):  Mach-O 64-bit executable x86_64

@rvagg
Copy link
Member Author

rvagg commented Nov 4, 2015

Also backported the LICENSE from v4.x, in a separate commit

@jbergstroem
Copy link
Member

@rvagg the jenkins slave is already running with a new java. You need to run both 32- and 64-bit builds on the same machine though. The machine is called iojs-joyent-smartos13.3.1-release.

@misterdjules
Copy link

@jbergstroem

You need to run both 32- and 64-bit builds on the same machine though.

Just to make sure we're on the same page: you can create as many machines as you need, so if you need two, please create two machines. During our conversation about cross-compiling what I meant is that building 32bits releases on a 64bits machine is fine, and using a 32bits machine to do that would probably not bring any improvement.

@jbergstroem
Copy link
Member

@misterdjules yep, that's great. Since 0.10 and 0.12 is built from the same host I think it's better to handle it the same way. The 32-bit machine was rather intended for our ci test runner.

@rvagg rvagg force-pushed the v0.12-build-update branch 8 times, most recently from 8f2eaaf to a6d256b Compare November 14, 2015 12:23
@rvagg
Copy link
Member Author

rvagg commented Nov 14, 2015

Lookie here: https://nodejs.org/download/nightly/v0.12.8-nightly20151114a6d256be0b/ — almost done! Compare to https://nodejs.org/download/release/v0.12.7/

There are two major items that I'm aware of that still need to be taken care of:

  • Use native compiler on CentOS5 build machines rather than the devtoolset-2 toolchain (gcc 4.8), currently it's just reusing what we use for v4+
  • Moar SHASUMS, we need SHA-1 versions and the .gpg files to go with them, we've trimmed right down on those files in v4+

Aside from that, I need help testing the builds and reviewing the commits in this PR. When I tick off the CentOS5 toolchain I'll push through an RC1 for 0.12 and move on to 0.10 which will hopefully be straightforward. Unfortunately node-gyp won't be happy with nightlies or RCs on <= 0.12 so testing won't be simple without a source to point at when compiling addons.

@rvagg
Copy link
Member Author

rvagg commented Nov 18, 2015

All done but the SHASUMS changes for promoting, if you are able, please test https://nodejs.org/download/nightly/v0.12.8-nightly20151118a6d256be0b/, I'm particularly interested in older systems because that's the risk we're up against here. I believe we are at parity with the previous infra and how it built these binaries but of course it's possible I'm missing something. I don't really want to find out by pushing out 0.12.8 and having everyone yell at us but maybe we have to if we can't find enough testers for this.

When I've done the SHASUMS changes I'm going to move to a proper release proposal and push out an RC.1 for v0.12.8.

@rvagg rvagg changed the title WIP: build: backport config for new CI infrastructure to v0.12 build: backport config for new CI infrastructure to v0.12 Nov 19, 2015
@rvagg
Copy link
Member Author

rvagg commented Nov 19, 2015

Removed the WIP label from this, it's ready to review please @nodejs/build

I backported tools/release.sh and updated the server to match so that for <=v0.12 it'll create a SHA-1 SHASUMS.txt and it'll also do a gpg -s to create an encrypted .gpg version of both files (we dropped that for >=v1).

See nodejs/build#261 for the minor updates on the server side to make this work.

I've run a test compile using this branch simulating a full v0.12.8, signed and placed @ https://nodejs.org/download/test/v0.12.8/ NOTE THAT THIS IS A FAUX RELEASE, DO NOT USE IN PRODUCTION! I will delete this directory shortly but it exists for review eyes.
Compare that with https://nodejs.org/download/release/v0.12.7/

I'll work on the release proposal and push out a v0.12.8-rc.1, probably today.

@rvagg
Copy link
Member Author

rvagg commented Nov 19, 2015

Also, I pulled the entire README.md off master, I needed it for the GPG release keys cause tools/release.sh cares about that but figured that there was no good reason to keep any of the old README.

@@ -188,37 +213,28 @@ maintained libraries. The externally maintained libraries used by Node are:
IN THE SOFTWARE.
"""

- Closure Linter is located at tools/closure_linter. Closure's license
follows:
- ESLint is located at tools/eslint. ESLint's license follows:

Choose a reason for hiding this comment

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

eslint is not used for the v0.12 branch.

@rvagg
Copy link
Member Author

rvagg commented Nov 19, 2015

doh, I did the updates on my v0.12.8-proposal branch rather than here. Fixed now so the changes to the README including removal of FIPS, update of prerequisites and license doc fixes are included here also.

@rvagg
Copy link
Member Author

rvagg commented Nov 22, 2015

@nodejs/collaborators need reviewers here to get an 0.12 out asap. I know there's a lot going on here, feel free to only review the parts you want and provide feedback if you have any. If you don't feel qualified to review all of it but are willing to 'lgtm' on parts of it, please be specific with a qualified 'lgtm' and hopefully we can gather enough to get full coverage of the changes.

I'll have another one of these for v0.10 and it'll be mostly the same, probably just the icu stuff that'll have to go. So it'd be good to get this right here first.

"""

This license applies to parts of Node.js originating from the
https://github.com/joyent/node repository:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the repository correct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, but since this is what's in master I think I'd rather contain that bikeshed to a separate PR. Perhaps you could open one with a suggestion? This language actually comes from io.js when there were two separate projects so the whole thing is a bit out of date. Perhaps we should even escalate this to the Foundation board to get proper legal advice so maybe if you don't have concrete suggestions for wording (I don't) just an issue to start the discussion?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rvagg Even I don't know how exactly this could be done :(


====
"""
Copyright Node.js contributors. All rights reserved.

Choose a reason for hiding this comment

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

Same as https://github.com/nodejs/node/pull/3965/files#r45569502:

By making additive changes to the license, are we changing it? If so, it is valid? Have we consulted someone familiar with the topic on that matter?

@rvagg
Copy link
Member Author

rvagg commented Nov 24, 2015

I've removed the LICENSE updates from here and opened #3979 to propose we seek proper legal advice before moving forward.

I've opened #3998 to collect any changes folks are noticing are required for the README as I don't think this, or #3965 for v0.10 are the place to make those changes, they need to come from master back.

So, leaving those two things aside, are there any objections to this PR? Are we able to muster a couple of lgtms here? It'd be really nice to get v0.12 out tomorrow and this is a blocker.

OSTYPE := $(shell uname -s | tr '[A-Z]' '[a-z]')

# Flags for packaging.
BUILD_DOWNLOAD_FLAGS ?= --download=all
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps not relevant to 0.10/0.12/other versions but I'd like to avoid usage of all since it could expand in the future (where i'd prefer it being removed). Could we use --download=icu instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @srl295, I agree.

But, it's not really relevant to this PR as it's already on master, v5.x and v4.x, a change should filter down from there

@jbergstroem
Copy link
Member

Few minor comments/questions but don't let it hold up a release. LGTM with the added note that its big enough for me to miss something.

PR-URL: nodejs#3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
PR-URL: nodejs#3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
PR-URL: nodejs#3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@orangemocha
Copy link
Contributor

Left a couple of minor suggestions, but generally LGTM. It is a fairly big change, let's see how it does through CI: https://ci.nodejs.org/job/node-test-pull-request/832/

rvagg added a commit that referenced this pull request Nov 24, 2015
PR-URL: #3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
rvagg added a commit that referenced this pull request Nov 24, 2015
PR-URL: #3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
rvagg added a commit that referenced this pull request Nov 24, 2015
PR-URL: #3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
@rvagg
Copy link
Member Author

rvagg commented Nov 24, 2015

thanks @orangemocha and @jbergstroem, merging this in to the v0.12.8 release. Next is #3965 when you have the time.

@rvagg rvagg closed this Nov 24, 2015
@rvagg rvagg deleted the v0.12-build-update branch November 24, 2015 23:30
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
PR-URL: nodejs/node#3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
PR-URL: nodejs/node#3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016
PR-URL: nodejs/node#3642
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
Reviewed-By: Alexis Campailla <alexis@janeasystems.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants