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

Implement RFC 002: Atom Nightly Releases #17538

Merged
merged 30 commits into from Jul 11, 2018

Conversation

Projects
None yet
5 participants
@daviwil
Copy link
Member

daviwil commented Jun 19, 2018

Description of the Change

This change adds automation for producing nightly Atom releases using VSTS CI. Most of the changes are just slight modifications to Atom's existing build scripts to produce another build channel and publish those artifacts in a way that can be installed and updated when new
releases are available.

See Atom RFC 002 for more details.

Alternate Designs

See this section of RFC 002.

Why Should This Be In Core?

This is a new release channel for Atom Core!

Benefits

See this section of RFC 002.

Possible Drawbacks

There isn't a major downside to this effort since it would run in parallel to the existing Atom release process without affecting it.

Verification Process

Produced signed Nightly builds using CI and verified that they are updatable using Electron's update service on Windows and macOS (the only platforms that support Squirrel).

At this moment nightly builds are not auto-updating because I decided to switch back to using Atom's update service on atom.io. Updating will work again once I push a change to atom.io today or tomorrow to enable the Nightly channel.

Applicable Issues

None.

Remaining Tasks

There are a few remaining tasks that need to be finished before this PR can be merged:

  • Upload release assets to S3 bucket
  • Update title bar (and other relevant strings) to read "Atom Nightly"
  • Add Atom and apm launcher scripts (atom-nightly.sh and apm-nightly.sh) for Linux builds
  • Enlist @simurai's help to produce a special icon color for the Nightly release channel (purple shade?)
  • Update the metrics package to report the Nightly channel (atom/metrics#92)
  • Find a way to set the version of the build in nightly-releases.yml so the major/minor parts aren't hardcoded
  • Update build environment variables to use ones defined by us and not VSTS
  • Update atom.io to support the new Nightly channel
  • Uncomment the assignment of remoteReleases in script/lib/create-windows-installer.js so that Windows delta updates can be generated
  • Fix or disable tests that fail on VSTS (more info below)
  • Document the VSTS setup in a short Markdown file for future reference
  • Update the About package to link to the right repo for Nightly release notes (atom/about#82)

Failing Tests

There are 4 tests that fail consistently on VSTS for no obvious reason:

macOS

keybinding-resolver package:

 KeyBindingResolverView
   when a keydown event occurs
     it displays all commands for the keydown event but does not clear for the keyup when there is no keyup binding
       TypeError: Cannot read property 'textContent' of null
         at it (/Users/vsts/agent/2.134.2/work/1/s/node_modules/keybinding-resolver/spec/keybinding-resolver-view-spec.js:91:80)
         at <anonymous>

GitHub package:

 GithubLoginModel default strategy manages passwords:
    AssertionError: assert.equal((await loginModel.getToken('test-account')), TOKEN): expected Symbol(UNAUTHENTICATED) to equal 'TOKEN'
       at Context.<anonymous> (/Users/vsts/agent/2.134.2/work/1/s/node_modules/github/test/models/github-login-model.test.js:22:18)
       at <anonymous>

GitHub package:

 GithubLoginModel SecurityBinaryStrategy manages passwords:
     Error: Command failed: security add-generic-password -s atom-github -a test-account -w TOKEN -U
 security: SecKeychainItemCreateFromContent (<default>): The specified item already exists in the keychain.
       at ChildProcess.exithandler (child_process.js:287:12)
       at emitTwo (events.js:126:13)
       at ChildProcess.emit (events.js:214:7)
       at maybeClose (internal/child_process.js:925:16)
       at Process.ChildProcess._handle.onexit (internal/child_process.js:209:5)

I'd certainly appreciate any ideas for how these issues might be diagnosed!

Future Tasks

There are other tasks that will be finished in future PRs:

  • Enable x86 Windows releases (VSTS currently doesn't support x86 builds) (#17668)
  • Automatically generate release notes for nightly releases (#17669)
  • Switch back to using Electron's update.electronjs.org service once it gains features that Atom needs (#17670)
  • Fix an issue in our Windows releases where different release channels can't be used side-by-side (#9247)

I'll file issues for these when this PR is approved and merged.

Enable automated nightly Atom releases
This change adds automation for producing nightly Atom releases using
VSTS CI.  Most of the changes are just slight modifications to Atom's
existing build scripts to produce another build channel and publish
those artifacts in a way that can be installed and updated when new
releases are available.

@daviwil daviwil requested review from jasonrudolph and maxbrunsfeld Jun 19, 2018

@@ -41,7 +44,9 @@ module.exports = {
}

function getChannel () {
if (appMetadata.version.match(/dev/)) {
if (process.env.BUILD_DEFINITIONNAME === 'Atom Nightly') {

This comment has been minimized.

@daviwil

daviwil Jun 19, 2018

Author Member

This is a VSTS environment variable but I'm planning to change it to something specific to Atom

const commitHash = result.stdout.toString().trim()
version += '-' + commitHash
} else if (channel === 'nightly') {
version = process.env.BUILD_BUILDNUMBER

This comment has been minimized.

@daviwil

daviwil Jun 19, 2018

Author Member

I'll also change this env variable to something that isn't specific to VSTS

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 20, 2018

Member

It looks like this uses the build number as Atom's version number. If that's the current behavior, how would you feel about changing it to append the build number to the version number (e.g., "1.30.0-nightly-42")?

This comment has been minimized.

@daviwil

daviwil Jul 11, 2018

Author Member

This was actually the case but it wasn't super clear since I was using VSTS' environment variable :) At this point in the code, I had configured the build to use the full 1.30.0-nightly42 version as the "build number". I've since changed it to use a more Atom-specific environment variable that gets set in the VSTS YAML config.

const glob = require('glob')
const path = require('path')

const CONFIG = require('../config')

module.exports = (packagedAppPath) => {
const archSuffix = process.arch === 'ia32' ? '' : '-' + process.arch
// const archSuffix = process.arch === 'ia32' ? '' : '-' + process.arch

This comment has been minimized.

@daviwil

daviwil Jun 19, 2018

Author Member

This and the other commented line below will be uncommented once atom.io changes are merged/deployed.

@@ -0,0 +1,30 @@
'use strict'

This comment has been minimized.

@daviwil

daviwil Jun 19, 2018

Author Member

This file wasn't supposed to make it in, will take it back out

@@ -0,0 +1,39 @@
name: 1.29.0-nightly$(Rev:r)

This comment has been minimized.

@daviwil

daviwil Jun 19, 2018

Author Member

Need to find a way to pull this version number from package.json on master so that it doesn't need to be updated manually each time we roll the railcars

@jasonrudolph
Copy link
Member

jasonrudolph left a comment

@daviwil: Thanks for getting this going. ⚡️

I've included a few questions inline in the review below.

In addition to those inline questions, I have a few higher-level questions/notes:

  1. What happens if I'm using the nightly release channel, and Atom's master branch hasn't had any new commits in more than 24 hours? Does a new nightly release still get created?

  2. What happens if the nightly build fails for one platform and passes for the other platforms? Does the nightly release get created for the platforms where the build succeeded?

  3. The task list says:

    • Fix or disable tests that fail on VSTS (more info below)

    I have a fairly strong preference that we fix those tests instead of disabling them. 🙏

const commitHash = result.stdout.toString().trim()
version += '-' + commitHash
} else if (channel === 'nightly') {
version = process.env.BUILD_BUILDNUMBER

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 20, 2018

Member

It looks like this uses the build number as Atom's version number. If that's the current behavior, how would you feel about changing it to append the build number to the version number (e.g., "1.30.0-nightly-42")?

fs.unlinkSync(nupkgPath)
} else {
if (process.arch === 'x64') {
const newNupkgPath = `${CONFIG.buildOutputPath}/atom-x64${path.basename(nupkgPath).slice(4)}`

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 20, 2018

Member

What's the significance of the number 4 on this line? Could that number be extracted to a constant whose name explains the significant of the number?

This comment has been minimized.

@daviwil

daviwil Jul 9, 2018

Author Member

4 is the length of the string atom, I'm basically ripping the front part of the original filename atom-1.30.0-nightly00-full.nupkg off so that I can replace it with atom-x64 in the build output path. I've simplified the logic here and added a comment so that the intent is clearer.

publishRelease({
token: process.env.GITHUB_TOKEN,
owner: 'atom',
repo: CONFIG.channel !== 'nightly' ? 'atom' : 'atom-nightly-releases',

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 20, 2018

Member

Can you remind me why it makes sense to have a separate repository for the nightly releases?

This comment has been minimized.

@daviwil

daviwil Jul 11, 2018

Author Member

Two reasons:

  1. atom/atom's Releases feed will get swamped with nightly releases and it'll be harder to scan through the list to find older stable/beta releases
  2. My goal was to use Electron's new update service for this and they use the Releases feed as the source of truth for updates. Seemed to make more sense to have it be a separate repo for now. Can always change it later!
steps:
- task: NodeTool@0
inputs:
versionSpec: 8.11.3

This comment has been minimized.

@jasonrudolph

jasonrudolph Jun 20, 2018

Member

I think the current builds all use Node 6.9.4 [example]. Can you share the motivation for using Node 8.11.3 here?

I'm thinking that we'd want all builds (including the new nightly builds) using the same Node version.

This comment has been minimized.

@daviwil

daviwil Jul 11, 2018

Author Member

Mainly because we now use Node 8 inside of Electron 2.0, so it's helpful to have the same JavaScript language features (async/await, improved perf) inside our build scripts. I started off using Node 6 in the builds and saw a noticable performance increase (~10 minutes saved) by switching to 8. More recently in this PR, I switched to the exact version of node in Electron 2.0, 8.9.3.

This comment has been minimized.

@jasonrudolph

jasonrudolph Jul 11, 2018

Member

@daviwil Thanks for the insight. Can I talk you into opening a follow-up PR to replace the existing uses of Node 6 with Node 8?

env: NODE_VERSION=6.9.4 DISPLAY=:99.0 CC=clang CXX=clang++ npm_config_clang=1

NODE_VERSION: 6.9.4

atom/circle.yml

Lines 20 to 21 in 3109958

- nvm install 6.9.4
- nvm use 6.9.4

This comment has been minimized.

@daviwil

daviwil Jul 11, 2018

Author Member

Definitely, I'll send that out later today!

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jun 21, 2018

Enlist @simurai's help to produce a special icon color for the Nightly release channel (purple shade?)

How about:

atom-nightly

Are the stars too cheesy? 😄 It's a hint for "nightly". It would also differ from the rest that only have different hues, maybe ok.

atom-icons

I was also thinking to replace all icons and redesign them with the "portal style" from atom.io. But that could also be done in a later PR.

@thomasjo

This comment has been minimized.

Copy link
Member

thomasjo commented Jun 21, 2018

@simurai I absolutely love that! Sure, it might be a little bit "cheesy", but 🧀 is delicious ❤️

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented Jun 21, 2018

OMG @simurai that is awesome.

@daviwil

This comment has been minimized.

Copy link
Member Author

daviwil commented Jun 21, 2018

Love it @simurai! Definitely makes it stand out in a clear way. I think doing an icon overhaul sometime this year in the new style would also be awesome, but this icon works for now!

@simurai

This comment has been minimized.

Copy link
Member

simurai commented Jun 22, 2018

I think doing an icon overhaul sometime this year in the new style would also be awesome, but this icon works for now!

Ok, let's keep it in mind, but no rush. For now I added the "purple with stars" version.

@daviwil daviwil force-pushed the dw-nightly-releases branch from 45789f8 to ed48f2d Jun 26, 2018

@daviwil daviwil referenced this pull request Jul 9, 2018

Closed

Iteration Plan: July 9 - July 20, 2018 #17660

8 of 16 tasks complete
@daviwil
Copy link
Member Author

daviwil left a comment

@jasonrudolph Thanks! To answer your questions:

  1. Nightly builds only get kicked off if there have been changes to master within the last 24 hours.

  2. If any part of the build process fails on any platform, nothing gets published. The "release" phase of the build only proceeds if all 3 platform builds and their tests pass.

  3. I've had to disable tests in the github and keybinding-resolver packages (only on VSTS) since they will take more time to investigate and fix. I've filed issues atom/github#1568 and atom/keybinding-resolver#60, respectively, to track the need to fix these. It turns out that they're both issues that appear on other CI platforms but VSTS happens to reproduce them more consistently.

fs.unlinkSync(nupkgPath)
} else {
if (process.arch === 'x64') {
const newNupkgPath = `${CONFIG.buildOutputPath}/atom-x64${path.basename(nupkgPath).slice(4)}`

This comment has been minimized.

@daviwil

daviwil Jul 9, 2018

Author Member

4 is the length of the string atom, I'm basically ripping the front part of the original filename atom-1.30.0-nightly00-full.nupkg off so that I can replace it with atom-x64 in the build output path. I've simplified the logic here and added a comment so that the intent is clearer.

@daviwil daviwil changed the title [WIP] Implement RFC 002: Atom Nightly Releases Implement RFC 002: Atom Nightly Releases Jul 9, 2018

@daviwil

This comment has been minimized.

Copy link
Member Author

daviwil commented Jul 9, 2018

This PR is ready for final review! If you'd like some overall context on the process, check out the documentation I wrote up here. I'm happy to continue making improvements to the code and documentation until we feel it's ready to be merged.

Thanks!

@maxbrunsfeld
Copy link
Contributor

maxbrunsfeld left a comment

This looks great!

@daviwil
Copy link
Member Author

daviwil left a comment

Missed some other comments from Jason, sorry about that!

steps:
- task: NodeTool@0
inputs:
versionSpec: 8.11.3

This comment has been minimized.

@daviwil

daviwil Jul 11, 2018

Author Member

Mainly because we now use Node 8 inside of Electron 2.0, so it's helpful to have the same JavaScript language features (async/await, improved perf) inside our build scripts. I started off using Node 6 in the builds and saw a noticable performance increase (~10 minutes saved) by switching to 8. More recently in this PR, I switched to the exact version of node in Electron 2.0, 8.9.3.

publishRelease({
token: process.env.GITHUB_TOKEN,
owner: 'atom',
repo: CONFIG.channel !== 'nightly' ? 'atom' : 'atom-nightly-releases',

This comment has been minimized.

@daviwil

daviwil Jul 11, 2018

Author Member

Two reasons:

  1. atom/atom's Releases feed will get swamped with nightly releases and it'll be harder to scan through the list to find older stable/beta releases
  2. My goal was to use Electron's new update service for this and they use the Releases feed as the source of truth for updates. Seemed to make more sense to have it be a separate repo for now. Can always change it later!
const commitHash = result.stdout.toString().trim()
version += '-' + commitHash
} else if (channel === 'nightly') {
version = process.env.BUILD_BUILDNUMBER

This comment has been minimized.

@daviwil

daviwil Jul 11, 2018

Author Member

This was actually the case but it wasn't super clear since I was using VSTS' environment variable :) At this point in the code, I had configured the build to use the full 1.30.0-nightly42 version as the "build number". I've since changed it to use a more Atom-specific environment variable that gets set in the VSTS YAML config.

@daviwil

This comment has been minimized.

Copy link
Member Author

daviwil commented Jul 11, 2018

Tests are green, issues are filed for future work. I think this is ready to be merged! Once it's in master I'll set up the scheduled build so that we start pumping out these releases on the regular :)

@daviwil daviwil merged commit 972b11c into master Jul 11, 2018

4 checks passed

VSTS: Atom Nightly 1.30.0-nightly3+20180711.2 succeeded
Details
ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@daviwil daviwil deleted the dw-nightly-releases branch Jul 11, 2018

@jasonrudolph

This comment has been minimized.

Copy link
Member

jasonrudolph commented Jul 11, 2018

🚚📦:atom:

@maxbrunsfeld

This comment has been minimized.

Copy link
Contributor

maxbrunsfeld commented Jul 11, 2018

That was a ton of work. Nice job powering through 💥 ⚡️

@daviwil

This comment has been minimized.

Copy link
Member Author

daviwil commented Jul 11, 2018

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.