Ability to store a built package in offline mirror #5314

bestander · 2018-02-03T08:46:21Z

Summary

Packages with install scripts will be saved into a .tgz file into offline mirror if experimental-pack-script-packages-in-mirror is set in .yarnrc.

Next time you run yarn install instead of unpacking the original .tgz package and running scripts yarn unpacks the previously built one and does not run any scripts.

Good for CI systems, slow machines and when you don't trust packages to do run shady download scripts.

Of course it adds platform specific suffixes to the .tgz file name, so that you can safely share the prebuilt offline mirror folder with other OS and Node versions.

Follow up: blog post and docs.

Test plan

Added tests for all common cases: rerunning install, switching Node version etc.

buildsize · 2018-02-03T08:48:41Z

This change will increase the build size from 10.46 MB to 10.48 MB, an increase of 22.83 KB (0%)

File name	Previous Size	New Size	Change
yarn-[version].noarch.rpm	906.53 KB	908.33 KB	1.8 KB (0%)
yarn-[version].js	3.94 MB	3.95 MB	8.78 KB (0%)
yarn-legacy-[version].js	4.09 MB	4.09 MB	8.96 KB (0%)
yarn-v[version].tar.gz	911.59 KB	913.6 KB	2.01 KB (0%)
yarn_[version]all.deb	673.41 KB	674.67 KB	1.27 KB (0%)

bestander · 2018-02-03T08:51:53Z

src/package-install-scripts.js

+
+  }
+
+  static getPrebuiltName(pkg: Manifest): string {


todo: put in a separate file

bestander · 2018-02-09T01:26:04Z

__tests__/commands/install/integration.js

@@ -145,22 +143,6 @@ test('reading a lockfile should not optimize it', async () => {
  });
 });



moved offline mirror tests out

bestander · 2018-02-09T01:27:47Z

__tests__/commands/install/offline-mirror.js

+  });
+});
+
+test('creates the file in the mirror when fetching a git repository', async () => {


following tests are copied from integration.js file

arcanis · 2018-02-15T00:44:35Z

src/util/package-name-utils.js

+/* @flow */
+
+export function getPlatformSpecificPackageFilename(pkg: {name: string, version: string}): string {
+  // TODO support hash for all subdependencies that have installs scripts


This will be problematic, yeah :/ Something I had in mind was that maybe we could implement nested lockfiles to solve this issue. Given a nested lockfile, we could guarantee that a package would always be built against a specific set of dependencies, and we could cache it more efficiently. Wdyt?

I think nested lockfile is an overkill and may be confusing considering that we don't support nested lockfiles in general.
We might generate something in between - a lock-like file that will only have dependencies section because we don't really need "version" and "resolved" fields as they are already defined in the parent yarn.lock

On the other hand having that file inside the .tgz file requires us to unzip it before checking whether the archive works for us, we need to encode all dependencies into the file suffix and hashing them seems a reasonable way to shorten the suffix

Anyway, we'll discuss it in V2 of this feature, we need to see how it will be used by the users if it will be :)

arcanis · 2018-02-15T00:45:07Z

src/util/package-name-utils.js

+  // TODO support hash for all subdependencies that have installs scripts
+  const normaliseScope = name => (name[0] === '@' ? name.substr(1).replace('/', '-') : name);
+  const suffix = getSystemParams();
+  return `${normaliseScope(pkg.name)}-v${pkg.version}-${suffix}`;


Nit: normalize

arcanis · 2018-02-15T00:46:05Z

src/config.js

@@ -328,6 +331,7 @@ export default class Config {
    this.enableMetaFolder = Boolean(this.getOption('enable-meta-folder'));
    this.enableLockfileVersions = Boolean(this.getOption('yarn-enable-lockfile-versions'));
    this.linkFileDependencies = Boolean(this.getOption('yarn-link-file-dependencies'));
+    this.packBuiltPackages = Boolean(this.getOption('pack-built-packages'));


Shouldn't the option be called experimental-build-mirror or something like this, to clearly state that it's experimental?

Yeah, makes sense.
How about experimental-pack-script-packages-in-mirror?

arcanis · 2018-02-15T00:47:39Z

src/package-install-scripts.js

+        if (this.packageCanBeInstalled(pkg)) {
+          const offlineMirrorPath = this.config.getOfflineMirrorPath();
+          if (!offlineMirrorPath) {
+            break;


If the packBuiltPackages option is set but no offline mirror has been set, the saveBuildArtifacts function won't be called. Is it expected?

Good point!
Not inteded

arcanis · 2018-02-15T00:48:59Z

src/package-install-scripts.js

+            },
+            this.reporter,
+          );
+          const stream = await pack(pkgConfig, loc);


Does pack also includes the build artifacts? :o

I pass loc which from local node_modules with built artifacts, pack just generates a .tgz from a folder

I'll name loc better to be clear

bestander · 2018-02-19T02:06:32Z

@arcanis, ready for another round

arcanis · 2018-02-20T16:23:08Z

src/package-install-scripts.js


+const fs2 = require('fs');


I find that a bit confusing - maybe createWriteStream should be simply reexported from util/fs (or maybe we should use fs-extra which takes care of exporting promises when we need them).

Yeah, fs2 is a poor choice for naming this variable.
Let's do it like in other places the Node.js fs module should be fs but out promise wrappers should be named fsUtil.

A year ago there was a proposal to reexport all exports from Node.js fs module there but we decided not to do it because it means we would have to manually list all functions of Node.js fs. Could be just extra trouble to worry when Node.js introduces a new function or breaks API

fs-extra automatically exports all the fs Node functions in such a way that they automatically support promises. Using it would allow us to let someone else care about keeping the list updated! :) Ok, not urgent, let's keep this for later

arcanis · 2018-02-20T16:24:12Z

src/package-install-scripts.js

+          });
+          pkg.prebuiltVariants = pkg.prebuiltVariants || {};
+          pkg.prebuiltVariants[prebuiltFilename] = hash;
+          // this.artifacts[`${pkg.name}@${pkg.version}`] = [prebuiltFilename];


Can we remove this line if it isn't used?

Doh, debugging code

arcanis · 2018-02-20T16:25:33Z

src/resolvers/registries/npm-resolver.js

+        const offlineMirrorPath = this.config.getOfflineMirrorPath();
+        if (prebuiltVariants[prebuiltName] && offlineMirrorPath) {
+          const filename = path.join(offlineMirrorPath, 'prebuilt', prebuiltName + '.tgz');
+          if (shrunk._remote && (await fs.exists(filename))) {


Fwiw the Node documentation recommends using fs.existsSync instead of fs.exists to prevent race condition as much as possible (fs.exists is actually deprecated).

We have 59 places where this wrapper is used.
If we migrate away from it we should do it in a separate PR, right?

arcanis · 2018-02-20T16:26:47Z

src/package-install-scripts.js

+      for (const pkg of pkgs) {
+        if (this.packageCanBeInstalled(pkg)) {
+          let prebuiltPath = path.join(offlineMirrorPath, 'prebuilt');
+          if (!await fs.exists(prebuiltPath)) {


We don't have to check it exists since we're using mkdirp which will do it for us, right?

Yeah, makes sense, fixed

bestander · 2018-02-21T06:22:31Z

@arcanis, ready for another round

arcanis

LGTM! Can't wait to see how people will use it!

hoodie · 2018-02-27T11:57:06Z

Question: does this depend on the engine being used? because some dependencies need to be rebuilt if installed for node or electron

bestander · 2018-03-04T18:13:46Z

@hoodie, currently platform, architecture and modules (Node libs version) are taken into account
https://github.com/yarnpkg/yarn/blob/master/src/util/package-name-utils.js#L12

Feel free to send a PR to add another parameter.

hoodie · 2018-03-04T23:13:41Z

@bestander that seems to be thorough enough, thank you :D

lxe · 2018-03-08T19:41:49Z

I absolutely love this feature. This adds a "supported" way to bring hermetic builds across platforms and environments.

tommilligan-plutoflume · 2018-03-19T14:14:01Z

For those wishing to enable this feature from a clean yarn installation, simply to speed up local development rebuilds - add the following in your project root:

.yarnrc

yarn-offline-mirror "./yarn-offline-cache"
yarn-offline-mirror-pruning true
experimental-pack-script-packages-in-mirror true

.gitignore

/yarn-offline-cache

Loving this feature already!

bbrzoska · 2018-03-19T17:05:00Z

We're having issues with this feature, as it saves references to prebuilt packages to yarn.lock and those change, I think based on the architecture of the computer (?). It caused yarn installs to fail in some scenarios for us, but we didn't had time to debug fully, and thus had to disable it.

theandrewlane · 2018-03-28T18:57:03Z

@bestander: Looks like yarn-offline-mirror-pruning true deletes./<yarn-offline-cache>/prebuilt/*, is this working as designed?

bestander · 2018-03-31T16:26:37Z

No :) Could you send a PR to fix that if you have time?

…

On 28 March 2018 at 11:57, Andrew Lane ***@***.***> wrote: Looks like yarn-offline-mirror-pruning true deletes./<yarn-offline-cache>/ prebuilt/*, is this as designed? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5314 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACBdWI9tPa6SkDUr4v6d6egLlvL7xIG3ks5ti90SgaJpZM4R4GRn> .

theandrewlane · 2018-04-02T16:45:13Z

@bestander - if you can confirm that the yarn-offline-mirror-pruning true config option is deleting the contents of /prebuilt for you, and shouldn't be, i'll submit a PR :)

bestander · 2018-04-02T16:46:08Z

Please do and thank you!

mikaello · 2018-04-04T13:31:12Z

@theandrewlane I have the same problem, that yarn-offline-mirror-pruning true removes the content of /prebuilt

theandrewlane · 2018-04-04T15:31:50Z

@mikaello: Cool, thanks for verifying! Waiting on #5616 merge 😎

bestander force-pushed the offline-mirror-for-scripts branch from dd4bb39 to d6a1646 Compare February 3, 2018 08:51

bestander commented Feb 3, 2018

View reviewed changes

src/package-install-scripts.js Outdated

}

static getPrebuiltName(pkg: Manifest): string {

Copy link

Member Author

bestander Feb 3, 2018

Choose a reason for hiding this comment

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

todo: put in a separate file

bestander mentioned this pull request Feb 4, 2018

Native packages rebuilt every time #932

Closed

bestander commented Feb 9, 2018

View reviewed changes

bestander changed the title ~~[WIP] Ability to store a built package in offline mirror~~ Ability to store a built package in offline mirror Feb 9, 2018

bestander requested review from BYK and arcanis February 9, 2018 01:29

arcanis reviewed Feb 15, 2018

View reviewed changes

bestander added 7 commits February 18, 2018 17:25

WIP: ability to store a built package in offline mirror

7e5d6fb

added tests

5d4428e

more tests

61dd958

flow fixes and tests

8436160

lockfile updates

0af2a4d

fixed more tests

1c92e61

feedback

c6fc640

bestander force-pushed the offline-mirror-for-scripts branch from a3a0ab4 to c6fc640 Compare February 19, 2018 01:47

fixed test

b8f8a81

arcanis reviewed Feb 20, 2018

View reviewed changes

feedback

11234fd

arcanis approved these changes Feb 22, 2018

View reviewed changes

bestander merged commit eab88b8 into yarnpkg:master Feb 22, 2018

katanacrimson mentioned this pull request Feb 28, 2018

yarn install --offline calls "scripts":"build" task inappropriately #5431

Closed

theandrewlane mentioned this pull request Apr 4, 2018

Offline Mirror Automatic Pruning Is Deleting Prebuilt Packages #5614

Closed

theandrewlane mentioned this pull request Apr 4, 2018

Bugfix #5614: When offline pruning, don't unlink prebuilt packages. #5616

Merged

felixrabe mentioned this pull request Apr 20, 2018

1.6.0: node-rdkafka@2.2.1 rebuilt every time anything is installed #5680

Closed

imfycc mentioned this pull request Sep 7, 2018

第 215 期《M站依赖安装问题》 by 杜松 helijia-web/micro_conf#213

Open

bestander mentioned this pull request Oct 21, 2018

Support yarn, pnpm and so on bestander/npm-git-lock#40

Closed

		@@ -145,22 +143,6 @@ test('reading a lockfile should not optimize it', async () => {
		});
		});

Ability to store a built package in offline mirror #5314

Ability to store a built package in offline mirror #5314

Conversation

bestander commented Feb 3, 2018 • edited Loading

buildsize bot commented Feb 3, 2018 • edited Loading

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

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

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

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 commented Feb 19, 2018

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

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

Choose a reason for hiding this comment

bestander commented Feb 21, 2018

arcanis left a comment

Choose a reason for hiding this comment

hoodie commented Feb 27, 2018

bestander commented Mar 4, 2018

hoodie commented Mar 4, 2018

lxe commented Mar 8, 2018

tommilligan-plutoflume commented Mar 19, 2018

bbrzoska commented Mar 19, 2018

theandrewlane commented Mar 28, 2018 • edited Loading

bestander commented Mar 31, 2018 via email

theandrewlane commented Apr 2, 2018

bestander commented Apr 2, 2018

mikaello commented Apr 4, 2018

theandrewlane commented Apr 4, 2018 • edited Loading

bestander commented Feb 3, 2018 •

edited

Loading

buildsize bot commented Feb 3, 2018 •

edited

Loading

theandrewlane commented Mar 28, 2018 •

edited

Loading

theandrewlane commented Apr 4, 2018 •

edited

Loading