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.

Sign up for GitHub

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

Properly skip installation of packages that do not need to be installed (incompatible optionalDependencies for example) #1997

Merged
merged 4 commits into from
Nov 26, 2016
Merged

Properly skip installation of packages that do not need to be installed (incompatible optionalDependencies for example) #1997

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3cc5337
Properly ignore package using ENVIRONMENT_IGNORE
mvestergaard Nov 23, 2016
09bdb77
Do not propagate ENVIRONMENT_IGNORE to dependencies
mvestergaard Nov 23, 2016
6a713fe
Remove unnecessary ignore check in linker
mvestergaard Nov 23, 2016
d414339
Add tests
mvestergaard Nov 24, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions __tests__/commands/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -794,3 +794,17 @@ test('install a scoped module from authed private registry', (): Promise<void> =
);
});
});

test.concurrent('install a module with incompatible optional dependency should skip dependency',
(): Promise<void> => {
return runInstall({}, 'install-should-skip-incompatible-optional-dep', async (config) => {
assert.ok(!(await fs.exists(path.join(config.cwd, 'node_modules', 'dep-incompatible'))));
});
});

test.concurrent('install a module with incompatible optional dependency should skip transient dependencies',
(): Promise<void> => {
return runInstall({}, 'install-should-skip-incompatible-optional-dep', async (config) => {
assert.ok(!(await fs.exists(path.join(config.cwd, 'node_modules', 'dep-a'))));
});
});
1 change: 1 addition & 0 deletions __tests__/fixtures/install/install-should-skip-incompatible-optional-dep/.npmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
yarn-offline-mirror=./mirror-for-offline
Binary file added .../install/install-should-skip-incompatible-optional-dep/mirror-for-offline/dep-a-1.0.0.tgz
View file
Open in desktop
Binary file not shown.
Binary file added ...stall-should-skip-incompatible-optional-dep/mirror-for-offline/dep-incompatible-1.0.0.tgz
View file
Open in desktop
Binary file not shown.
5 changes: 5 additions & 0 deletions __tests__/fixtures/install/install-should-skip-incompatible-optional-dep/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
{
"optionalDependencies": {
"dep-incompatible": "1.0.0"
}
}
11 changes: 11 additions & 0 deletions __tests__/fixtures/install/install-should-skip-incompatible-optional-dep/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
# yarn lockfile v1
[email protected]:
version "1.0.0"
resolved dep-incompatible-1.0.0.tgz#d06b95482893472aabc08e86e76580cfae058333
dependencies:
dep-a "1.0.0"

[email protected]:
version "1.0.0"
resolved dep-a-1.0.0.tgz#13cb7dfcabff56d3ce39c10c1b9a0900134e615d
2 changes: 1 addition & 1 deletion src/cli/commands/check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export async function run(
}

// get patterns that are installed when running `yarn install`
const [, rawPatterns] = await install.hydrate();
const [, rawPatterns] = await install.hydrate(true);
Copy link
Member

Choose a reason for hiding this comment

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

Why is fetch true for a check operation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for this bit:

await this.fetcher.init();
await this.compatibility.init();

It seems that if the fetcher is not run beforehand, the compatibility check does not have the os and cpu data for each package to properly do the check.

Copy link
Member

Choose a reason for hiding this comment

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

Would this work offline?
I am afraid that initing fetcher may cause a longer check

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good question. I mean if the package is in the cache, it doesn't actually fetch anything, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This happens if the fetch is not a part of the check:

yarn check v0.18.0-0
error chokidar#fsevents not installed
error fsevents#nan not installed
error fsevents#node-pre-gyp not installed
error node-pre-gyp#npmlog not installed
error node-pre-gyp#rc not installed
error node-pre-gyp#tar-pack not installed
error rc#deep-extend not installed
error tar-pack#fstream-ignore not installed
error rc#minimist not installed
error tar-pack#once not installed
error tar-pack#uid-number not installed
error Found 11 errors.

Because the check does not exclude those package due to incompatibility, it expects them to be there, which it shouldn't.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, it makes sense.
For cross platform modules - they are installed in node_modules and could be verified in place but the missing ones could be either "missing" or not installed because they are "optional".

Adding "optional" property to lockfile makes more sense but this needs a discussion involving more people, maybe an RFC?

Copy link
Member

Choose a reason for hiding this comment

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

As a matter of fact, I think there should be a third mode for check - #1934.
I would rather rely on that and the integrity check than the default one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think adding whether a package is optional to the lock file is enough alone.
You still want it to be installed on an OS where it is supported, but skipped on an OS where it isn't.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder, if package is optional but it failed to install because you don't have a compiler, is it an error or is it ok?

Copy link
Contributor Author

@mvestergaard mvestergaard Nov 24, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

That should be an error.
In my opinion, if it is determined by the OS and CPU requirements of the package that your system is compatible, then any installation error that occurs, are out of the scope of what yarn should handle.


// check if patterns exist in lockfile
for (const pattern of rawPatterns) {
Expand Down
2 changes: 2 additions & 0 deletions src/cli/commands/install.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -624,6 +624,7 @@ export class Install {
async writeIntegrityHash(lockSource: string, patterns: Array<string>): Promise<void> {
const loc = await this.getIntegrityHashLocation();
invariant(loc, 'expected integrity hash location');
await fs.mkdirp(path.dirname(loc));
await fs.writeFile(loc, this.generateIntegrityHash(lockSource, patterns));
}

Expand Down Expand Up @@ -671,6 +672,7 @@ export class Install {
if (fetch) {
// fetch packages, should hit cache most of the time
await this.fetcher.init();
await this.compatibility.init();

// expand minimal manifests
for (const manifest of this.resolver.getManifests()) {
Expand Down
4 changes: 4 additions & 0 deletions src/package-hoister.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,10 @@ export default class PackageHoister {
const ref = pkg._reference;
invariant(ref, 'expected reference');

if (ref.ignore) {
return undefined;
}

//
const loc: string = this.config.generateHardModulePath(ref);
const parts = parentParts.concat(pkg.name);
Expand Down
10 changes: 9 additions & 1 deletion src/package-reference.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,11 @@ export default class PackageReference {
nowIgnore = true;
}

// if the package is determinted to be incompatible with the environment, it should be ignored
if (stack[ENVIRONMENT_IGNORE] === 1) {
nowIgnore = true;
}

this.ignore = nowIgnore;
}

Expand All @@ -165,7 +170,10 @@ export default class PackageReference {

const ref = pkg._reference;
invariant(ref, 'expected package reference');
ref.addVisibility(action, ancestry);

if (action !== ENVIRONMENT_IGNORE) {
ref.addVisibility(action, ancestry);
}
}
}
}