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

fix: major version dependency upgrades, oclif v2 updates #364

Merged
merged 23 commits into from
Sep 8, 2022
Merged

Conversation

shazron
Copy link
Member

@shazron shazron commented Aug 31, 2022

Description

  • updates to major versions of plugin dependencies
  • added @adobe/aio-cli-plugin-app-templates plugin as core plugin
  • added @adobe/aio-cli-plugin-telemetry as core plugin
  • updated other dependencies (via npm outdated, only modules that don't have ESM-only updates)
  • removed custom "space" as command topic separator code and tests (oclif v2 has this built in now, via package.json/oclif/topicSeparator key)

How Has This Been Tested?

  • npm test
  • npm run e2e
  • further tests via pre-release

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@shazron
Copy link
Member Author

shazron commented Aug 31, 2022

node-14 has arch issues in the CI, but node-16 doesn't have a problem. Looks like a lockfile version compatibility issue -- it was generated using lockfileVersion@2 (via the npm included in node-16) and that fails in node14. The fix is to use a common npm (we always use the latest)

@codecov
Copy link

codecov bot commented Aug 31, 2022

Codecov Report

Merging #364 (ba0a74b) into master (364d440) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #364   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         4    -1     
  Lines          247       225   -22     
  Branches        49        43    -6     
=========================================
- Hits           247       225   -22     

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@shazron
Copy link
Member Author

shazron commented Aug 31, 2022

linux tests pass, now we have issues with Windows and node-gyp 🤷🏼‍♂️

@shazron
Copy link
Member Author

shazron commented Aug 31, 2022

looks like there are issues with npm@7+ nodejs/node-gyp#508 and node-gyp

@shazron
Copy link
Member Author

shazron commented Aug 31, 2022

I think we need node-gyp@9 to support windows-latest OR use windows-2019.

Also actions/setup-node#280
https://github.com/nodejs/node-gyp/blob/main/docs/Updating-npm-bundled-node-gyp.md

@shazron
Copy link
Member Author

shazron commented Aug 31, 2022

This issue seems similar nodejs/node-gyp#2683

@shazron
Copy link
Member Author

shazron commented Sep 1, 2022

windows-2019 works for node-16 but not for node-14 (npm ERR! D:\a\aio-cli\aio-cli\node_modules\lmdb\src\v8-functions.cpp(5,10): fatal error C1083: Cannot open include file: 'v8.h': No such file or directory [D:\a\aio-cli\aio-cli\node_modules\lmdb\build\lmdb.vcxproj]) - clearly that's not the solution, so reverting

@shazron
Copy link
Member Author

shazron commented Sep 5, 2022

One alternative is (see #364 (comment)) we drop support for node-14 testing on Windows and use the windows-2019 image

@shazron
Copy link
Member Author

shazron commented Sep 7, 2022

I believe one of the parcel packages is triggering node-gyp to build native interfaces (probably @parcel/transformer-json):

2233 timing build:run:install:node_modules/msgpackr-extract Completed in 6542ms
2234 info run lmdb@2.2.4 install node_modules/@parcel/transformer-html/node_modules/lmdb node-gyp-build
2235 info run lmdb@2.5.2 install { code: 1, signal: null }
2236 info run lmdb@2.2.4 install { code: 0, signal: null }
2237 timing build:run:install:node_modules/@parcel/transformer-html/node_modules/lmdb Completed in 2366ms
2238 info run lmdb@2.2.4 install node_modules/@parcel/transformer-image/node_modules/lmdb node-gyp-build
2239 info run lmdb@2.2.4 install { code: 0, signal: null }
2240 timing build:run:install:node_modules/@parcel/transformer-css/node_modules/lmdb Completed in 2582ms
2241 info run lmdb@2.2.4 install node_modules/@parcel/transformer-js/node_modules/lmdb node-gyp-build
2242 info run lmdb@2.2.4 install { code: 0, signal: null }
2243 timing build:run:install:node_modules/@parcel/transformer-babel/node_modules/lmdb Completed in 3150ms
2244 info run lmdb@2.2.4 install node_modules/@parcel/transformer-json/node_modules/lmdb node-gyp-build
2245 info run lmdb@2.2.4 install { code: 1, signal: null }
2246 info run lmdb@2.2.4 install { code: 1, signal: null }
2247 info run lmdb@2.2.4 install { code: 1, signal: null }
2248 timing reify:rollback:createSparse Completed in 14753ms
2249 timing reify:rollback:retireShallow Completed in 0ms
2250 timing command:ci Completed in 109747ms
2251 verbose stack Error: command failed
2251 verbose stack     at ChildProcess.<anonymous> (C:\Program Files\nodejs\node_modules\npm\node_modules\@npmcli\promise-spawn\lib\index.js:63:27)
2251 verbose stack     at ChildProcess.emit (node:events:513:28)
2251 verbose stack     at maybeClose (node:internal/child_process:1093:16)
2251 verbose stack     at Process.ChildProcess._handle.onexit (node:internal/child_process:302:5)
2252 verbose pkgid lmdb@2.5.2
2253 verbose cwd C:\Users\shazron\Documents\git\aio-cli
2254 verbose Windows_NT 10.0.22598
2255 verbose node v16.17.0
2256 verbose npm  v8.15.0
2257 error code 1
2258 error path C:\Users\shazron\Documents\git\aio-cli\node_modules\lmdb
2259 error command failed

@shazron
Copy link
Member Author

shazron commented Sep 7, 2022

this might help: parcel-bundler/parcel#8152

@shazron
Copy link
Member Author

shazron commented Sep 7, 2022

Hmm 591b1c5 passed, now re-running it, it is failing. seems very flaky

@shazron
Copy link
Member Author

shazron commented Sep 7, 2022

Looks like, on local testing on Windows, if I delete package-lock.json before npm i --package-lock --package-lock-only --legacy-peer-deps , npm ci --peer-legacy-deps works. Must be a Windows quirk.

@purplecabbage
Copy link
Member

Okay, this all looks reasonable, however it is still very hard to test.
I would like to merge it, and do a prerelease with it ( without using @next versions of plugins ) so we can test it in real(ish) conditions before releasing.

@shazron shazron merged commit ae56a6a into master Sep 8, 2022
@shazron shazron deleted the dep-updates branch September 8, 2022 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants