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

kpm install then kpm check reports errors #86

Closed
shayne opened this issue Jun 29, 2016 · 9 comments
Closed

kpm install then kpm check reports errors #86

shayne opened this issue Jun 29, 2016 · 9 comments
Labels

Comments

@shayne
Copy link
Contributor

shayne commented Jun 29, 2016

$ kpm install
fbkpm install v0.4.0
info Read lockfile fbkpm.lock
[1/5] ⏩  Rehydrating dependency graph...
[2/5] πŸ“¦  Fetching packages...
[3/5] βœ…  Checking package compatibility...
[4/5] πŸ”—  Linking dependencies...
[5/5] πŸ“ƒ  Running install scripts...
✨  Done in 25.80s. Peak memory usage 205.83MB.
$ kpm check
fbkpm check v0.4.0
error Module not installed: node_modules/mkdirp/node_modules/minimist
error Module not installed: node_modules/escodegen/node_modules/source-map
error Module not installed: node_modules/jsbn
error Module not installed: node_modules/tweetnacl
error Module not installed: node_modules/jodid25519
error Module not installed: node_modules/ecc-jsbn
@sebmck
Copy link
Contributor

sebmck commented Jun 29, 2016

Looks like there's some differences between the deduping for install and check...

@sebmck
Copy link
Contributor

sebmck commented Jun 29, 2016

Looks like the difference is caused between strict and loose module installing modes.

@sebmck sebmck closed this as completed in b2254b4 Jun 29, 2016
@shayne
Copy link
Contributor Author

shayne commented Jul 1, 2016

Still an issue, unfortunately. I've troubleshooted but the hoisting is taking me a while to wrap my head around.

Repo steps:

  • remove ~/.fbkpm
  • remove ~/fbsource/fbcode/react-native/offile-files (typo, i know)
  • remove react-native-github/fbkpm.lock and react-native-github/node_modules
  • follow output using master fbfkpm code
 ~/Code/fbkpm/bin/kpm.js install
fbkpm install v0.4.4
info Read lockfile fbkpm.lock
[1/4] 🚚  Resolving and fetching packages...
[2/4] βœ…  Checking package compatibility...
[3/4] πŸ”—  Linking dependencies...
[4/4] πŸ“ƒ  Running install scripts...
✨  Done in 22.48s. Peak memory usage 154.70MB.

$ ~/Code/fbkpm/bin/kpm.js check
fbkpm check v0.4.4
error Module not installed: node_modules/jest-runtime/node_modules/camelcase
error Module not installed: node_modules/jest-runtime/node_modules/cliui
error Module not installed: node_modules/jest-runtime/node_modules/window-size

@shayne shayne reopened this Jul 1, 2016
@shayne
Copy link
Contributor Author

shayne commented Jul 1, 2016

Subsequent installs are successful while check continues to fail. I've looked at commands/check.js and commands/install.js and they appear to be following similar code paths. I've merged differences with no luck.

@bestander
Copy link
Member

The deps that fail check command are dev dependencies.
If package.json does not have dev dependencies then check works correctly for react-native

@bestander
Copy link
Member

More info, the problem is in jest-runtime and jest dependencies.
This is the minimum package.json that I can reproduce with:

{
  "name": "react-native",
  "version": "1000.0.0",
  "description": "A framework for building native apps using React",
  "dependencies": {
    "jest": "^13.0.0",
    "jest-runtime": "^13.0.0"
  }
}

Just removing jest-runtime from the above or the whole react-native package.json makes check great again.

I'll investigate more tomorrow

@bestander
Copy link
Member

bestander commented Jul 2, 2016

Last info for today, there is a bug in hoisting algorithm for the above package.json:

jest-cli@13.0.0 -> yargs@4.7.1 -> camelcase@3.0.0
jest-runtime@13.0.0 -> yargs@4.7.1 -> camelcase@3.0.0
yargs@3.10.0 -> camelcase@1.0.2

This installs jest-runtime/node_modules/yargs but not jest-runtime/node_modules/camelcase.
However jest-cli/node_modules/yargs and jest-cli/node_modules/camelcase do get installed

@bestander
Copy link
Member

Added #89 with an e2e test.

@bestander
Copy link
Member

@shayne, there is a way around this bug for now.
Just remove jest-runtime from dev dependencies, it will get installed anyway because jest depends on it while check will succeed.

@sebmck sebmck closed this as completed in fd30c0b Jul 4, 2016
connectdotz added a commit to connectdotz/yarn that referenced this issue Nov 22, 2017
- nohoist implementation
- 'why' command fixes
- 'add' command fixes
- tests and test fixtures

see [RFC yarnpkg#86](yarnpkg/rfcs#86) for detail
bestander pushed a commit that referenced this issue Jan 28, 2018
* nohoist impl check point

- nohoist implementation
- 'why' command fixes
- 'add' command fixes
- tests and test fixtures

see [RFC #86](yarnpkg/rfcs#86) for detail

* fix not adding devDependencies

* add nohoist flag and eligibility check

1. added a new flags 'workspaces-nohoist-experimental' to disable nohoist.
2. added eligibility validation in Config.getWorkspaces, violation will be reported and config be ignored.
3. update test fixtures to add private flag for nohoist tests

* revert path separator to '#' for display

* pass through private flag in root manifest

* fix lint error

* addressing @bestander review comments on 1/8

* fix merge conflict

* fix merge lint issues

* address @arcanis comments

* update snapshot after merge

* one more snapshot update
arcanis added a commit that referenced this issue Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants