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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

apm upgrade #17675

Merged
merged 14 commits into from Jul 30, 2018

Conversation

Projects
None yet
3 participants
@smashwilson
Member

smashwilson commented Jul 12, 2018

馃挜 This brings in a prerelease version of apm based on atom/apm#796 to see how horribly it breaks everything. 馃挜

To verify

apm's test suite is actually fairly comprehensive. Just the same, we should manually verify that major functionality that shells out to or calls into npm still works.

Test case packages

  • Packages without native dependencies: teletype; atom-lcov
  • Packages with native dependencies: atom-ide-ui; hydrogen; latex; github
  • Packages with a package-lock.json: git-plus
  • macOS
    • apm --version
    • apm install: from atom.io
    • ensure proxy settings are read from .apmrc
    • apm install: with native dependencies
    • apm install: from local directory
    • apm install: from git repository
    • apm uninstall
    • apm config
    • apm clean
    • apm dedupe
    • apm rebuild
    • apm publish
  • Linux
    • apm --version
    • apm install: from atom.io
    • ensure proxy settings are read from .apmrc
    • apm install: with native dependencies
    • apm install: from local directory
    • apm install: from git repository
    • apm uninstall
    • apm config
    • apm clean
    • apm dedupe
    • apm rebuild
    • apm publish
  • Windows
    • apm --version
    • apm install: from atom.io
    • ensure proxy settings are read from .apmrc
    • apm install: with native dependencies
    • apm install: from local directory
    • apm install: from git repository
    • apm uninstall
    • apm config
    • apm clean broken on master
    • apm dedupe
    • apm rebuild
    • apm publish

To fix

  • (node:82366) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.

  • Windows apm clean deletes all scoped packages in an infinite loop replicated on master: atom/apm#746

To ship

  • Merge atom/apm#796.
  • Publish a major version bump release of apm.
  • 猬嗭笍 to the non-prerelease version of apm on this branch.
@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil
Member

daviwil commented Jul 12, 2018

my body is ready

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Jul 12, 2018

Member

It seems to be working... 馃檹

Member

daviwil commented Jul 12, 2018

It seems to be working... 馃檹

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2018

Member

Yes indeed:

smashwilson @ hubtop ~
$ apm --version
(node:65261) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
apm  1.19.0-0
npm  6.1.0
node 8.9.3 x64
atom 1.30.0-dev-27a6e1158
python 2.7.15
git 2.18.0

Let's work on a checklist of things we should verify before continuing. I'll toss a checklist into the PR description.

What timing would you have in mind for a real release? Should we try to get it into 1.30-beta, or hold off and merge right after 1.30-beta ships so that we give it the maximum amount of time to percolate? On one hand, it's a highly risky change; on the other hand, we aren't likely to notice breakage while it's only on dev - most of us have somewhat similar dev environments and (I assume) don't install lots of packages every day. Maybe we should just rip the band-aid off and be ready to hotfix?

Member

smashwilson commented Jul 12, 2018

Yes indeed:

smashwilson @ hubtop ~
$ apm --version
(node:65261) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
apm  1.19.0-0
npm  6.1.0
node 8.9.3 x64
atom 1.30.0-dev-27a6e1158
python 2.7.15
git 2.18.0

Let's work on a checklist of things we should verify before continuing. I'll toss a checklist into the PR description.

What timing would you have in mind for a real release? Should we try to get it into 1.30-beta, or hold off and merge right after 1.30-beta ships so that we give it the maximum amount of time to percolate? On one hand, it's a highly risky change; on the other hand, we aren't likely to notice breakage while it's only on dev - most of us have somewhat similar dev environments and (I assume) don't install lots of packages every day. Maybe we should just rip the band-aid off and be ready to hotfix?

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Jul 12, 2018

Member

@smashwilson I think we need to test some (major) community packages that use native node modules, to verify that they still compile (and run) with the upgrade.

Member

thomasjo commented Jul 12, 2018

@smashwilson I think we need to test some (major) community packages that use native node modules, to verify that they still compile (and run) with the upgrade.

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2018

Member

@thomasjo Absolutely.

Did you have any specific ones in mind that have caused us challenges in the past? I was planning to do a find ~/.atom/packages -name '*.node' to find some good candidates.

Member

smashwilson commented Jul 12, 2018

@thomasjo Absolutely.

Did you have any specific ones in mind that have caused us challenges in the past? I was planning to do a find ~/.atom/packages -name '*.node' to find some good candidates.

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2018

Member

It looks like atom-ide-ui uses a few, but they're installed from prebuilts... other than that, atom/github certainly uses keytar...

Member

smashwilson commented Jul 12, 2018

It looks like atom-ide-ui uses a few, but they're installed from prebuilts... other than that, atom/github certainly uses keytar...

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Jul 12, 2018

Member

@smashwilson I think Nuclide has native stuff? My own LaTex package has a native module dependency, though hardly a major community package.

Member

thomasjo commented Jul 12, 2018

@smashwilson I think Nuclide has native stuff? My own LaTex package has a native module dependency, though hardly a major community package.

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2018

Member

Hydrogen uses 0mq which is a native dependency...

smashwilson @ hubtop ~/src/atom/apm (aw/massive-npm-version-bumps=)
$ apm install hydrogen
(node:82123) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
Installing hydrogen to /Users/smashwilson/.atom/packages 鉁
smashwilson @ hubtop ~/src/atom/apm (aw/massive-npm-version-bumps=)
$ find /Users/smashwilson/.atom/packages/Hydrogen/ -name '*.node'
/Users/smashwilson/.atom/packages/Hydrogen//node_modules/zeromq/build/Release/zmq.node
Member

smashwilson commented Jul 12, 2018

Hydrogen uses 0mq which is a native dependency...

smashwilson @ hubtop ~/src/atom/apm (aw/massive-npm-version-bumps=)
$ apm install hydrogen
(node:82123) [DEP0022] DeprecationWarning: os.tmpDir() is deprecated. Use os.tmpdir() instead.
Installing hydrogen to /Users/smashwilson/.atom/packages 鉁
smashwilson @ hubtop ~/src/atom/apm (aw/massive-npm-version-bumps=)
$ find /Users/smashwilson/.atom/packages/Hydrogen/ -name '*.node'
/Users/smashwilson/.atom/packages/Hydrogen//node_modules/zeromq/build/Release/zmq.node
@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2018

Member

Okay, I've identified a few packages as good test candidates for a variety of criteria. More are always welcome 馃憤

Member

smashwilson commented Jul 12, 2018

Okay, I've identified a few packages as good test candidates for a variety of criteria. More are always welcome 馃憤

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Jul 12, 2018

Member

@smashwilson Cool. Did you check the PlatformIO ecosystem?

Member

thomasjo commented Jul 12, 2018

@smashwilson Cool. Did you check the PlatformIO ecosystem?

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2018

Member

Nope! 馃槅 Do we test there? Is there someone we can ping (or a repo we can file an issue on) to check it out?

Member

smashwilson commented Jul 12, 2018

Nope! 馃槅 Do we test there? Is there someone we can ping (or a repo we can file an issue on) to check it out?

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Jul 12, 2018

Member

I think the main repo is platformio/platformio-atom-ide. Haven't followed the dependency chain all the way, but might be some native modules via platformio/platformio-node-helpers maybe.

Member

thomasjo commented Jul 12, 2018

I think the main repo is platformio/platformio-atom-ide. Haven't followed the dependency chain all the way, but might be some native modules via platformio/platformio-node-helpers maybe.

@thomasjo

This comment has been minimized.

Show comment
Hide comment
@thomasjo

thomasjo Jul 12, 2018

Member

Just noticed that git-plus has a package-lock.json file. Probably a good candidate.

Member

thomasjo commented Jul 12, 2018

Just noticed that git-plus has a package-lock.json file. Probably a good candidate.

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2018

Member

Oh huh! I just noticed that npm added package dependencies to package.json in atom/atom:

  "dependencies": {
    "@atom/nsfw": "^1.0.18",
    "@atom/source-map-support": "^0.3.4",
    "@atom/watcher": "1.0.3",
    "about": "https://www.atom.io/api/packages/about/versions/1.10.0/tarball",
    "archive-view": "https://www.atom.io/api/packages/archive-view/versions/0.65.1/tarball",
    "async": "0.2.6",
    "atom-dark-syntax": "https://www.atom.io/api/packages/atom-dark-syntax/versions/0.29.0/tarball",
    "atom-dark-ui": "https://www.atom.io/api/packages/atom-dark-ui/versions/0.53.2/tarball",
    "atom-keymap": "8.2.10",
  }
Member

smashwilson commented Jul 12, 2018

Oh huh! I just noticed that npm added package dependencies to package.json in atom/atom:

  "dependencies": {
    "@atom/nsfw": "^1.0.18",
    "@atom/source-map-support": "^0.3.4",
    "@atom/watcher": "1.0.3",
    "about": "https://www.atom.io/api/packages/about/versions/1.10.0/tarball",
    "archive-view": "https://www.atom.io/api/packages/archive-view/versions/0.65.1/tarball",
    "async": "0.2.6",
    "atom-dark-syntax": "https://www.atom.io/api/packages/atom-dark-syntax/versions/0.29.0/tarball",
    "atom-dark-ui": "https://www.atom.io/api/packages/atom-dark-ui/versions/0.53.2/tarball",
    "atom-keymap": "8.2.10",
  }
@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 12, 2018

Member

Here's a preliminary set of performance data based on timing a bunch of apm commands from a cold cache:

Command 1.19.0 1.19.0-0
[ '--version' ] 822ms 686ms (-136ms)
[ 'uninstall', 'teletype', 'atom-lcov', 'atom-ide-ui', 'hydrogen', 'latex', 'github', 'git-plus' ] 16330ms 11872ms (-4458ms)
[ 'install', 'teletype' ] 5597ms 4880ms (-717ms)
[ 'install', 'atom-lcov' ] 3637ms 4281ms (+644ms)
[ 'install', 'atom-ide-ui' ] 25641ms 19351ms (-6290ms)
[ 'install', 'hydrogen' ] 71410ms 33564ms (-37846ms)
[ 'install', 'latex' ] 19115ms 13936ms (-5179ms)
[ 'install', 'github' ] 29325ms 15613ms (-13712ms)
[ 'install', 'git-plus' ] 16313ms 10738ms (-5575ms)
[ 'install', 'atom/github' ] 81752ms 40520ms (-41232ms)
[ 'rebuild', 'hygrogen' ] 1553ms 1111ms (-442ms)
[ 'rebuild' ] 1524ms 1160ms (-364ms)

Not bad, not bad.

Member

smashwilson commented Jul 12, 2018

Here's a preliminary set of performance data based on timing a bunch of apm commands from a cold cache:

Command 1.19.0 1.19.0-0
[ '--version' ] 822ms 686ms (-136ms)
[ 'uninstall', 'teletype', 'atom-lcov', 'atom-ide-ui', 'hydrogen', 'latex', 'github', 'git-plus' ] 16330ms 11872ms (-4458ms)
[ 'install', 'teletype' ] 5597ms 4880ms (-717ms)
[ 'install', 'atom-lcov' ] 3637ms 4281ms (+644ms)
[ 'install', 'atom-ide-ui' ] 25641ms 19351ms (-6290ms)
[ 'install', 'hydrogen' ] 71410ms 33564ms (-37846ms)
[ 'install', 'latex' ] 19115ms 13936ms (-5179ms)
[ 'install', 'github' ] 29325ms 15613ms (-13712ms)
[ 'install', 'git-plus' ] 16313ms 10738ms (-5575ms)
[ 'install', 'atom/github' ] 81752ms 40520ms (-41232ms)
[ 'rebuild', 'hygrogen' ] 1553ms 1111ms (-442ms)
[ 'rebuild' ] 1524ms 1160ms (-364ms)

Not bad, not bad.

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Jul 12, 2018

Member

That is hella fast, should shave a decent chunk of time off of full Atom builds!

Member

daviwil commented Jul 12, 2018

That is hella fast, should shave a decent chunk of time off of full Atom builds!

smashwilson added some commits Jul 12, 2018

@smashwilson

This comment has been minimized.

Show comment
Hide comment
@smashwilson

smashwilson Jul 19, 2018

Member

No regressions detected across all three platforms other than that apm clean thing. 馃帀

Member

smashwilson commented Jul 19, 2018

No regressions detected across all three platforms other than that apm clean thing. 馃帀

smashwilson added some commits Jul 19, 2018

@daviwil

This comment has been minimized.

Show comment
Hide comment
@daviwil

daviwil Jul 28, 2018

Member

Railcars are rolled... Perhaps this can be merged on Monday? :)

Member

daviwil commented Jul 28, 2018

Railcars are rolled... Perhaps this can be merged on Monday? :)

@smashwilson smashwilson merged commit e814f49 into master Jul 30, 2018

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@smashwilson smashwilson deleted the aw/apm-up branch Jul 30, 2018

@50Wliu 50Wliu referenced this pull request Aug 2, 2018

Closed

Missing dependency to libonig-dev in build instructions #17758

1 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment