Fix self-update #676

danez · 2016-10-11T18:00:21Z

This fixes self-update by doing:

enable the tests
use npm run build in the test instad of make build
invalidated roadrunner cache, so that the symlink to current gets reevaluated
do not do the request to github for the assets as they are already included in the release response
pick the correct asset from the asset list by doing a name-check (content-type is sometimes set to octet-stream for the tar file, so we can't use that)

The roadrunner change is necessary as the tests need to invalidate the path cache so that the next test can reevaluate the symlinks. Same for the actual self-update command which also changes the symlink. This depends on kittens/roadrunner#1

Fixes #612

Docs: yarnpkg/website#173

bestander · 2016-10-12T10:02:01Z

Nice!
Should we wait for roadrunner fix to be propagated to upstream?

danez · 2016-10-12T10:03:11Z

Yes I think so. @kittens can you have a look at kittens/roadrunner#1 ?

danez · 2016-10-12T10:04:47Z

src/constants.js

@@ -38,6 +38,7 @@ export function getModuleCacheDirectory(): string {
 export const MODULE_CACHE_DIRECTORY = getModuleCacheDirectory();
 export const LINK_REGISTRY_DIRECTORY = `${MODULE_CACHE_DIRECTORY}/.link`;
 export const GLOBAL_MODULE_DIRECTORY = `${MODULE_CACHE_DIRECTORY}/.global`;
+export const CACHE_FILENAME = path.join(userHome, '.yarn', '.roadrunner.json');


I would like to use this variable in bin/yarn.js, but afaics I cannot simple import this file there right? Currently this line is duplicate here and in bin/yarn.js

.yarn is prone to change afaik.
There should be a constant with the folder name

I can extract that to an other constant, but then still it wouldn't be usable in bin/yarn.js I think.

danez · 2016-10-12T10:05:14Z

__tests__/commands/self-update.js

  return run(async (reporter, config) => {
    // mock an existing self-update
-    await child.exec('make build');
+    await child.exec('npm run build');


It is probably safe to assume that npm is available when running tests?

Can we do "kpm run build"? :)
For now, yeah, we can assume there is some npm version on the dev environment

you mean yarn ? :P

Oh yeah, kpm has been engraved in my brain now :)

danez · 2016-10-12T10:05:49Z

__tests__/commands/self-update.js

    const version = await child.exec('node bin/yarn.js -V');
-    expect(version[0].trim(), `0.11.0`);
+    expect(version[0].trim(), '0.14.1');


Had to update the version as <0.14 was using the name kpm in the release

danez · 2016-10-12T10:06:11Z

__tests__/commands/self-update.js


    expect(await fs.exists(path.resolve(updatesFolder, 'v0.98.0'))).toBe(false);
    expect(await fs.exists(path.resolve(updatesFolder, 'v0.99.0'))).toBe(true);
-    expect(await fs.exists(path.resolve(updatesFolder, 'v0.11.0'))).toBe(true);
+    expect(await fs.exists(path.resolve(updatesFolder, 'v0.15.0'))).toBe(true);


This is updated to use 0.15 to check it picks the correct asset.

Daniel15

I think we can remove all the Github API stuff from this. Instead of using the Github API, you can hit https://yarnpkg.com/latest-version to get the version number. Then you can simply hit https://yarnpkg.com/latest.tar.gz to get the latest tarball. Much simpler 😄 . There's also https://yarnpkg.com/downloads/{version}/{file} for specific version.

bestander · 2016-10-12T19:26:08Z

Good point, @Daniel15, dropping Github wrapper from dependencies is good for size and startup performance.
Limiting self-update to latest is a limitation but I guess reasonable.

bestander · 2016-10-12T19:27:06Z

A good RFC discussion is how to enable self-update to be automatic.

Daniel15 · 2016-10-12T19:29:21Z

A good RFC discussion is how to enable self-update to be automatic.

For what it's worth, updates are handled along with the rest of the operating system if you use the Linux package (Ubuntu/Debian/CentOS), that's one major advantage of using them and one of the main reasons we built them :)

bestander · 2016-10-12T19:36:56Z

Well, I mean the other 99.5 percent of users who don't use Linux :)

On 12 October 2016 at 20:29, Daniel Lo Nigro notifications@github.com
wrote:

A good RFC discussion is how to enable self-update to be automatic.

For what it's worth, updates are handled along with the rest of the
operating system if you use the Linux package (Ubuntu/Debian/CentOS),
that's one major advantage of using them and one of the main reasons we
built them :)

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
#676 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACBdWKGAZSYIUIj8mYuKaKfjL6sgUFdKks5qzTUXgaJpZM4KT7EC
.

Daniel15 · 2016-10-12T19:39:05Z

@bestander Chocolatey handles updates too I believe, so Windows users are covered too once our Chocolatey package is approved. The Windows dev userbase is larger than the Mac OS dev userbase 😛 . The Windows installer probably needs a proper auto-update too, that downloads the latest MSI and installs it.

bestander · 2016-10-12T19:59:41Z

You don't expect every version of windows to install yarn with chocolatey?
But bikeshedding aside, I think we need to decide whether we want to keep this feature here or ditch it. If we keep it then let's offer users whether to enable it by default.

Some history: I added it here when we found that maintaining packages via yummy and Chef was too much hassle and it seemed like a reasonable idea to have a lightweight mechanism to get everyone on the same version within the org.
In the end we ended up checking in yarn.js instead of relying on Chef or building our own Chef for yarn self-updates.

As for the community project, I think we get quite a lot of benefits if most of users are getting updated to the latest stable whether it is done via package managers or via self-update.

bestander · 2016-10-12T20:01:39Z

Also I am not a huge fan of this implementation, a bit too hacky

danez · 2016-10-12T21:13:08Z

For macOS probably adding yarn to brew might probably also make it easy to install and update there.

How do we want to proceed with this PR? Should I do all the mentioned changes (not github etc.) and later you can still decide to remove/simplify it?

bestander · 2016-10-12T22:33:26Z

@danez, let's do as @Daniel15 suggests and use the website API to get latest version.
Are you up to it?
We can bikeshed about whether we need this feature or enable it later.

Daniel15 · 2016-10-12T22:35:26Z

Should I do all the mentioned changes (not github etc.) and later you can still decide to remove/simplify it?

Yeah, that sounds good to me! Sorry I went a little off-topic in the comments. Removing all the Github stuff should simplify it quite a bit :)

ithinkihaveacat · 2016-10-13T10:22:54Z

@Daniel15 I don't know the implications of this but right now https://yarnpkg.com/latest-version does not list the latest version:

$ yarn -V
0.15.1
$ curl -s https://yarnpkg.com/latest-version
0.15.0

bestander · 2016-10-13T10:29:06Z

Michael, could you file a bug?

On 13 October 2016 at 11:23, Michael Stillwell notifications@github.com
wrote:

@Daniel15 https://github.com/Daniel15 I don't know the implications of
this but right now https://yarnpkg.com/latest-version does not list the
latest version:

$ yarn -V
0.15.1
$ curl -s https://yarnpkg.com/latest-version
0.15.0

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#676 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ACBdWM511I6iJJToPDjKCRw8WUbM6W2Gks5qzgaFgaJpZM4KT7EC
.

danez · 2016-10-13T10:43:21Z

This is probably because there was no release (in as a consequence no new tarball) created on github, only the tag in git.

https://github.com/yarnpkg/yarn/releases

ithinkihaveacat · 2016-10-13T10:50:47Z

@bestander Sure, reported as #989.

Daniel15 · 2016-10-13T15:54:10Z

0.15.1 is not a "real" release, it was just a patch to the tarball and npm
package. All the other installation methods are still at 0.15.0

Sent from my phone.

On Oct 13, 2016 3:51 AM, "Michael Stillwell" notifications@github.com
wrote:

@bestander https://github.com/bestander Sure, reported as #989
#989.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#676 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAFnHQsKcGtR4pvU3NNbVc7ZxPH8-xr3ks5qzg0OgaJpZM4KT7EC
.

Daniel15

This looks good to me!

Daniel15 · 2016-10-13T21:56:10Z

package.json

@@ -33,7 +32,7 @@
    "request": "^2.75.0",
    "request-capture-har": "^1.1.4",
    "rimraf": "^2.5.0",
-    "roadrunner": "^1.0.6",
+    "roadrunner": "danez/roadrunner#patch-1",


Should we wait for a release of roadrunner before merging this?

Daniel15 · 2016-10-13T21:57:07Z

yarn.lock

@@ -2237,14 +2223,6 @@ getpass@^0.1.1:
  dependencies:
    assert-plus "^1.0.0"

-github@2.5.1:


Nice 👍 🎉

danez · 2016-10-13T21:59:07Z

The test are still failing, because of 0.15.1.

The problem is that https://yarnpkg.com/latest-version says to 0.15.0, but when downloading latest.tar.gz the package.json in this archive says 0.15.1. Im gonna try instead of relying on the package.json instead see where the current symlink points.

Daniel15 · 2016-10-13T22:00:57Z

It's tricky because 0.15.1 is not a "real" release - It was just a fixed re-release of the npm package and tarball, but everything else (Windows installer, Linux packages) are still on 0.15.0

GantMan · 2016-10-13T22:19:28Z

looking forward to this 👍

danez · 2016-10-13T22:34:23Z

bin/yarn.js

-var path = require('path');
-var CACHE_FILENAME = path.join(userHome, '.yarn', '.roadrunner.json');
-mkdirp.sync(path.dirname(CACHE_FILENAME));
+var constants = require('../lib-legacy/constants');


I now also require constants here from lib-legacy, so that I can use the folder constants. I think that shouldn't be a problem.

bestander · 2016-10-14T09:58:15Z

yarn.lock

  version "1.0.6"
-  resolved "https://registry.yarnpkg.com/roadrunner/-/roadrunner-1.0.6.tgz#e466a15f9844289413d09943ad263574623ec1c9"
+  resolved "https://codeload.github.com/danez/roadrunner/tar.gz/2ead9501e3681fa7584b79e6e6546fb046a83f54"


Let's push the change upstream before merging this.

danez · 2016-10-14T12:13:21Z

I noticed that it is possible to download a certain version of yarn see #751 (comment)
Should I add back the possibility to specify the version?

Daniel15 · 2016-10-14T17:52:56Z

Should I add back the possibility to specify the version?

Let's keep it simple - It's totally fine to only support updating to the most recent version 😄

jamiebuilds · 2016-10-17T08:02:15Z

@danez you have a broken test:

 FAIL  __tests__/commands/self-update.js (10.597s)
  ● Self-update should work from self-updated location

    Error: Command failed: yarn run build
    /bin/sh: 1: yarn: not found

    Console output:


      at __tests__/commands/self-update.js:57:13
      at throw (native)
      at step (node_modules/babel-runtime/helpers/asyncToGenerator.js:17:30)
      at node_modules/babel-runtime/helpers/asyncToGenerator.js:30:20

  ✓ Self-update should download a release and symlink it as "current" (10365ms)
  ✕ Self-update should work from self-updated location (73ms)

danez · 2016-10-17T10:05:21Z

CircleCI is not updating the dependencies :(

bestander · 2016-10-17T10:14:32Z

I'll restart with cache clean

danez · 2016-10-17T10:19:32Z

Seems there is no step on circleCI to install dependencies? At least I can't see one.

bestander · 2016-10-17T10:20:25Z

Yeah, odd, looks like cleaning all cache lost eslint and webpack.
Update: oh, yeah we were missing yarn install in circle.yml

bestander · 2016-10-17T10:42:14Z

Created this one #1157

bestander · 2016-10-17T12:46:54Z

@danez, could you please rebase it?
This should work now

Pick the correct file to download in self-update We cannot rely that the first asset is always the tar file This picks the correct file from the assets Use dev version of roadrunner and reset it correctly Enable self-update tests

danez · 2016-10-17T13:09:42Z

Seems circleCI is to slow to run the tests :/ The test is anyway really ugly, and involves building yarn, so not sure if we should keep it.

bestander · 2016-10-17T13:19:03Z

Hm, I would assume that download gets stuck.
Building yarn should be quite fast though.
This is an important test because it checks that self-update did not degrade because of the symlinks and yarn inside yarn situation.

What do you think of keeping this test disabled for a while and merge the rest?
Great work anyway!

danez · 2016-10-17T13:40:46Z

Okay I disabled it now, even after 3 minutes it was running into a timeout.

As all the other downloads seem to be quiet fast it might also be the FS which is slow, and we copy a lot of stuff in this test.

bestander · 2016-10-17T14:56:07Z

Thanks, @danez!

danez mentioned this pull request Oct 11, 2016

Remove github OAuth token #641

Closed

cpojer assigned bestander Oct 12, 2016

danez changed the title ~~Enable self-update tests~~ Fix self-update Oct 12, 2016

bestander mentioned this pull request Oct 12, 2016

Remove outdated kpm parts #758

Merged

danez commented Oct 12, 2016

View reviewed changes

Daniel15 requested changes Oct 12, 2016

View reviewed changes

Daniel15 approved these changes Oct 13, 2016

View reviewed changes

dlong500 mentioned this pull request Oct 13, 2016

Documentation and questions on methods to keep yarn up to date on Windows #1021

Closed

danez commented Oct 13, 2016

View reviewed changes

danez mentioned this pull request Oct 13, 2016

Update docs for self-update yarnpkg/website#173

Merged

bestander reviewed Oct 14, 2016

View reviewed changes

danez added 4 commits October 17, 2016 14:47

Fix self-update

22b36a6

Pick the correct file to download in self-update We cannot rely that the first asset is always the tar file This picks the correct file from the assets Use dev version of roadrunner and reset it correctly Enable self-update tests

Remove github resolving from self-update

792ad85

Fix tests.

c89bb06

Update roadrunner and fix tests

eb71e87

danez added 2 commits October 17, 2016 15:23

Increase test timeout to 3 minutes

cee3d14

disable test because to slow on circleCI

8d72189

bestander merged commit b584b4f into yarnpkg:master Oct 17, 2016

danez deleted the enable-self-update-tests branch October 17, 2016 14:56

troglotit mentioned this pull request Oct 19, 2016

self-update errors on authentication #1187

Closed

Fix self-update #676

Fix self-update #676

Conversation

danez commented Oct 11, 2016 • edited

bestander commented Oct 12, 2016

danez commented Oct 12, 2016 • edited

danez Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bestander Oct 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danez Oct 12, 2016 • edited

Choose a reason for hiding this comment

Daniel15 left a comment

Choose a reason for hiding this comment

bestander commented Oct 12, 2016

bestander commented Oct 12, 2016

Daniel15 commented Oct 12, 2016

bestander commented Oct 12, 2016

Daniel15 commented Oct 12, 2016

bestander commented Oct 12, 2016 • edited

bestander commented Oct 12, 2016

danez commented Oct 12, 2016

bestander commented Oct 12, 2016

Daniel15 commented Oct 12, 2016

ithinkihaveacat commented Oct 13, 2016

bestander commented Oct 13, 2016

danez commented Oct 13, 2016 • edited

ithinkihaveacat commented Oct 13, 2016

Daniel15 commented Oct 13, 2016

Daniel15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danez commented Oct 13, 2016

Daniel15 commented Oct 13, 2016

GantMan commented Oct 13, 2016

danez Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danez commented Oct 14, 2016 • edited

Daniel15 commented Oct 14, 2016 • edited

jamiebuilds commented Oct 17, 2016

danez commented Oct 17, 2016

bestander commented Oct 17, 2016

danez commented Oct 17, 2016

bestander commented Oct 17, 2016 • edited

bestander commented Oct 17, 2016

bestander commented Oct 17, 2016

danez commented Oct 17, 2016

bestander commented Oct 17, 2016

danez commented Oct 17, 2016

bestander commented Oct 17, 2016

danez commented Oct 11, 2016 •

edited

danez commented Oct 12, 2016 •

edited

danez Oct 12, 2016 •

edited

bestander Oct 12, 2016 •

edited

danez Oct 12, 2016 •

edited

bestander commented Oct 12, 2016 •

edited

danez commented Oct 13, 2016 •

edited

danez Oct 13, 2016 •

edited

danez commented Oct 14, 2016 •

edited

Daniel15 commented Oct 14, 2016 •

edited

bestander commented Oct 17, 2016 •

edited