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

jenkins: don't skip osx release sources build for 10.x #2282

Merged
merged 1 commit into from
Apr 12, 2020

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Apr 10, 2020

Also remove redundant release sources for Node 10

Ref: nodejs/node#32755

Explanation:

Hello foot-gun.

#2203 switched our release builds for osx to 10.15, which changed this whole block to prevent 10.10 and 10.11 for release builds.

Unfortunately, we have overlapping regexes that weren't noticed. Below this section is the "release-sources" labels. We used to build tarballs with osx all the way up to Node 11 when we switched to centos for Node 12. Node 10 is osx1010 and Node 11 was osx1011, so we have "release-sources" labels for those.

When we excluded ^osx1010 and ^osx1011 from releaseType we also excluded osx1010-release-sources from Node 10.

So the 10.20.0 iojs+release build has this:

20:27:22 Skipping centos7-release-sources for Node.js 10
20:27:22 Skipping osx1011-release-sources for Node.js 10
20:27:22 Skipping osx1010-release-sources for Node.js 10

No release-sources! So this PR uses a negative look-ahead to allow osx1010-release-sources back in to Node 10, the release-sources block below should do the right thing to select the right release-sources builder for each of them. I've also removed osx1011-release-sources from the list, that was only for Node 11.

Regarding nodejs/node#32755 and the successful release of 10.20.0 and the headers and source files it has (see https://nodejs.org/download/release/v10.20.0/): I don't have the complete explanation because unfortunately the 26th of March is ~2 days too far beyond our ci-release build logs pruning date and I'm not sure if we store those as backups anywhere. But my guess is that a test 10.20.0 build was done on the 26th with an earlier version of the source and those files sat in staging. The change to VersionSelectorScript to switch to osx1015 was done on the 4th of April, so the 26th of May was before and it would have done a release-sources run. Then when the actual one was done, all of the assets were overwritten with new versions except for source and headers files, and the expected-assets check gave a green tick! If there weren't files in staging to make up the difference then it would have given a red cross.

So now we have a mismatch between sources, headers and the binaries. So 10.20.0 should probably be considered broken.

also remove redundant release sources for Node 10

Ref: nodejs/node#32755
Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

Ugh. Maybe we need to take the one hit pain and flip the script to inclusions rather than exclusions.

Copy link
Member

@AshCripps AshCripps left a comment

Choose a reason for hiding this comment

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

So does this mean the osx1010 release machine needs to stay for the rest of 10.x?

@rvagg
Copy link
Member Author

rvagg commented Apr 10, 2020

@AshCripps yes, it does! unfortunately

@rvagg
Copy link
Member Author

rvagg commented Apr 10, 2020

FYI I've also just deleted a v14.0.0 directory in staging from March 25th with a bunch of assets in it. That'll might have a similar impact if we remove any assets from the release before it goes live.

So I guess we need to come up with some kind of staging-clean or release timestamp range check or something to deal with this kind of possibility in the future.

Maybe if you start an iojs+release it first runs a job that cleans out staging for the version being built. That might have to go into Makefile which is where the canonical staging directory is selected, unfortunately. That might be getting a bit too complicated though.

@AshCripps
Copy link
Member

@AshCripps yes, it does! unfortunately

Thats fine I was about to start looking into migrating the machine to ORKA so its good to know we do need it.

@rvagg
Copy link
Member Author

rvagg commented Apr 10, 2020

No, I think for staging we need to be militant about the commit. We take some amount of trust that node_version.h is correct and that there's not duplicate runs of the same "version". The release builds should upload to a directory that has the commit in it. Then the tools/release.sh script should validate that commit is tagged on the releasers computer for that version and refuse to promote anything else. We can handle non-release auto-promotion more lax, but this release pipeline isn't tight enough.

tbh though I don't really relish the idea of taking responsibility for adding more complexity to the process. Taking the blame when things break can be a bit tiring! Will have to think on this.

@BethGriggs
Copy link
Member

BethGriggs commented Apr 10, 2020

👋me again

But my guess is that a test 10.20.0 build was done on the 26th with an earlier version of the source and those files sat in staging.

That is quite likely. The release was prepared, built, and ready to go on the 26th March, but the schedule was changed and the release delayed after the release meeting that same day - nodejs/node#31984 (comment)

The 2-week delay enabled few additional commits to land in the release (including the notarization changes and the n-api semver minor commits). As v10.x is going into maintenance soon, it made sense to pull those in while we were doing a minor release. That explains the difference in the release content.

Also sorry for v10.x dramas! I think the delaying of the release, subsequently updating, and multiple releases candidates has definitely added to the problems here.

v10.20.1 - nodejs/node#32768 🤞

@@ -116,8 +116,6 @@ def buildExclusions = [

// Source / headers / docs -------------------------------
[ /^osx1010-release-sources$/, releaseType, gte(11) ],
[ /^osx1011-release-sources$/, releaseType, lt(11) ],
[ /^osx1011-release-sources$/, releaseType, gte(12) ],
Copy link
Member

Choose a reason for hiding this comment

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

Unless we've removed osx1011-release-sources from the job in the release CI, do we not need to still exclude ^osx1011-release-sources$ for Node.js 12+ to prevent two source jobs running on 12+ (one on osx1011-release-sources and one on centos7-release-sources)?

Copy link
Member

Choose a reason for hiding this comment

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

@richardlau I can confirm that osx1011-release-sources is still a tick-able configuration in release build job (if that helps, I'm aware that you cannot access it to check.)

Copy link
Member Author

Choose a reason for hiding this comment

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

@richardlau food-gun again, this one certainly does support flipping from exclude to include!

I've removed all the osx1011 labels (cc @AshCripps) from iojs+release and the pkg & tar labels for osx1010, leaving only osx1010-release-sources.

https://github.com/nodejs/jenkins-config-release/commit/93d3ad13a52a108dea7e37e1463848d1bf4b6405

-        <string>osx1010-release-pkg</string>
         <string>osx1010-release-sources</string>
-        <string>osx1010-release-tar</string>
-        <string>osx1011-release-pkg</string>
-        <string>osx1011-release-sources</string>
-        <string>osx1011-release-tar</string>
         <string>osx1015-release-pkg</string>
         <string>osx1015-release-tar</string>

@rvagg rvagg merged commit 43f4c75 into master Apr 12, 2020
@rvagg rvagg deleted the rvagg/rvagg-osx-label-restrict branch April 12, 2020 10:31
@rvagg
Copy link
Member Author

rvagg commented Apr 12, 2020

Test build for @BethGriggs' proposed 10.20.1: https://ci-release.nodejs.org/job/iojs+release/5892 seems to be doing the right thing.

20:33:02 Triggering iojs+release » rhel7-s390x-release
20:33:02 Triggering iojs+release » osx1010-release-sources
20:33:02 Triggering iojs+release » osx1015-release-pkg
20:33:02 Triggering iojs+release » vs2017-x64
20:33:02 Triggering iojs+release » centos7-arm64-gcc6
20:33:02 Triggering iojs+release » cross-compiler-armv6-gcc-4.9.4
20:33:02 Triggering iojs+release » osx1015-release-tar
20:33:02 Triggering iojs+release » vs2017-x86
20:33:02 Triggering iojs+release » centos6-64-gcc6
20:33:02 Triggering iojs+release » centos7-ppcle-release-64
20:33:02 Triggering iojs+release » aix71-ppc64
20:33:02 Triggering iojs+release » smartos17-release
20:33:02 Triggering iojs+release » cross-compiler-armv7-gcc-4.9.4

@AshCripps AshCripps mentioned this pull request Apr 14, 2020
4 tasks
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.

4 participants