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

[Windows] use npm config instead of npm_config_cache #2153

Merged
merged 1 commit into from
Jan 14, 2021
Merged

[Windows] use npm config instead of npm_config_cache #2153

merged 1 commit into from
Jan 14, 2021

Conversation

dr-js
Copy link
Contributor

@dr-js dr-js commented Nov 29, 2020

Description

This commit change the way to set npm cache path from env to config file on Windows runners.

The npm_config_cache env, if set, will have highest priority, and harder to change.

One way to un-set this is add a global workflow env, like:

env:
  npm_config_cache: ''

This commit change the cache config to use npm config set cache $CachePath --global,
which should save the path under the global npmrc at: C:\npm\prefix\etc\npmrc,
and allow easier later reset with user/repo level .npmrc files.

For the record, my usage is to unify all platform's npm cache to ~/.npm/,
then use the same cache action config on all platform to cache the folder.

Related issue:

Check list

  • Related issue / work item is attached
  • Tests are written (if applicable)
  • Documentation is updated (if applicable)
  • Changes are tested and related VM images are successfully generated

The `npm_config_cache` env, if set, will have highest priority, and harder to change.

One way to un-set this is add a global workflow env, like:
```
env:
  npm_config_cache: ''
```

This commit change the cache config to use `npm config set cache $CachePath --global`,
which should save the path under the global npmrc at: `C:\npm\prefix\etc\npmrc`,
and allow easier later reset with user/repo level `.npmrc` files.

For the record, my usage is to unify all platform's npm cache to `~/.npm/`,
then use the same cache action config on all platform to cache the folder.
@asklar
Copy link
Contributor

asklar commented Nov 29, 2020

Please make sure this doesn't regress #1566

@dr-js
Copy link
Contributor Author

dr-js commented Nov 29, 2020

@asklar I do not think this will make #1566 worse, since the env is removed.
But I also don't know if this will affect yarn in your case.

@dr-js
Copy link
Contributor Author

dr-js commented Nov 29, 2020

Also on why preset env is better removed:
The windows runner will run each step from the same job under the same process, for example compare the log of steps from this run:
https://github.com/dr-js/dr-dev/runs/1469095654?check_suite_focus=true#step:4:224
https://github.com/dr-js/dr-dev/runs/1469095654?check_suite_focus=true#step:5:198

    5412 |    │  │  ├─ C:\actions\runner-provisioner-Windows\provisioner.exe --agentdirectory c:...\settings\.cache (+30)
    1416 |    │  │  │  ├─ "c:\runners\2.274.2\bin\Runner.Listener.exe" run
    3292 |    │  │  │  │  └─ "c:\runners\2.274.2\bin\Runner.Worker.exe" spawnclient 1864 1876
    2080 |    │  │  │  │     ├─ \??\C:\windows\system32\conhost.exe 0x4
-    4656 |    │  │  │  │     └─ "C:\Program Files\PowerShell\7\pwsh.EXE" -command ". 'D:\a\_temp...dfc0606bba.ps1'" (+27)
-    7152 |    │  │  │  │        └─ C:\windows\system32\cmd.exe /c ""C:\hostedtoolcache\windows\n...dev.7 --ps tree" (+43)
-    6448 |    │  │  │  │           └─ "C:\hostedtoolcache\windows\node\14.15.0\x64\\node.exe"  "...-dev.7 --ps tree (+95)
-    5788 |    │  │  │  │              └─ WMIC PROCESS get Commandline,ParentProcessId,Processid
+    4888 |    │  │  │  │     └─ "C:\Program Files\PowerShell\7\pwsh.EXE" -command ". 'D:\a\_temp...5220dee320.ps1'" (+27)
+    5444 |    │  │  │  │        └─ C:\windows\system32\cmd.exe /c ""C:\hostedtoolcache\windows\n...dev.7 --ps tree" (+43)
+    4328 |    │  │  │  │           └─ "C:\hostedtoolcache\windows\node\14.15.0\x64\\node.exe"  "...-dev.7 --ps tree (+95)
+     896 |    │  │  │  │              └─ WMIC PROCESS get Commandline,ParentProcessId,Processid

You'll see the same Runner.Worker.exe with pid 3292 will spawn command for each step. (npx @dr-js/core@0.4.1-dev.7 --ps tree in this case)

So the problem with changing the env is:

  • changing the global env won't work, since a "snapshot" of env is hold by the worker 3292, it won't pick up the change
  • changing the process env in the inner runner won't worker either, since it'll only last for current step and exit

To change the "snapshot" env hold by the worker (for all step of all job) can only be done with the workflow env config,
but that won't work well for dynamically calculated env, or platform specific ones.

And with current workflow syntax, I don't see how to set persisting dynamically calculated env in one step, and be able to use that in follow up steps.

So I'd suggest use conf files instead of env for persisting global settings.

@miketimofeev miketimofeev requested a review from a team November 30, 2020 10:04
dr-js added a commit to dr-js/dr-dev that referenced this pull request Dec 1, 2020
notable change:
- ci: cache: wait for: actions/runner-images#2153
- add: ci: `.github/ci-patch.js` for doing ci patch in one place
- ci & script sort
- package update
@maxim-lobanov
Copy link
Contributor

/azp run windows2016, windows2019

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@dr-js
Copy link
Contributor Author

dr-js commented Dec 3, 2020

That's some long builds!

Not sure how to test the build result, but I think a few check should pass:

  • no npm_config_cache set in env
  • the user can change cache path with user level .npmrc or repo level .npmrc, and npm config get cache in corresponding path should print the most relevant setting
  • there's a global npmrc file at: C:\npm\prefix\etc\npmrc with content cache=C:\npm\cache (the $CachePath)
  • the npm config ls should pick up and list the content of the global npmrc and log something like:
; "builtin" config from C:\Program Files\nodejs\node_modules\npm\npmrc

; prefix = "C:\\Users\\Dr\\AppData\\Roaming\\npm" ; overridden by env

; "global" config from C:\npm\prefix\etc\npmrc

cache = "c:\\npm\\cache" ; NOTE: the cache path from install script

; "user" config from C:\Users\...\.npmrc

; "env" config from environment

prefix = "C:\\npm\\prefix\\" ; NOTE: the prefix path from ENV

; "cli" config from command line options

omit = []
user-agent = "npm/7.0.8 node/v15.1.0 win32 x64"

; node bin location = C:\...\node.exe
; cwd = C:\...
; HOME = C:\Users\...
; Run `npm config ls -l` to show all defaults.

NOTE: the command npm config set registry http://registry.npmjs.org/ in the install script is strange in some ways, and maybe don't set it is better:

  • the default registry for npm@6 and npm@7 is HTTPS not HTTP
  • this set is saved to the install user's .npmrc, but the install user may not be the runner user (runneradmin, from my test), so the change may not be pick up

dr-js added a commit to dr-js/dr-dev that referenced this pull request Jan 5, 2021
notable change:
- deprecate: `fileProcessorWebpack`
- temp-fix: ci: wait for npm/cli#2411
- temp-fix: ci: cache: wait for: actions/runner-images#2153
- fix: use `@dr-js/core@0.4.3-dev.1` with fix for nodejs@15.5 caused responder send noop
- fix: output: log git diff in git status check
- fix: test: treat `before|after` the same as `it`
- add: `copyAfterEdit` to `node/file`
- add: bin: `is-hash-changed` to `cache-step`
- add: ci: `.github/ci-patch.js` for doing ci patch in one place
- node: use `getToggle` for `Preset.Toggle`
- bin: suppot `checksum-file-only` for `cache-step`
- bin: sort babel resolve path & test
- bin: sort `cache-step` options
- ci & script sort
- package update
@maxim-lobanov maxim-lobanov merged commit e202793 into actions:main Jan 14, 2021
@dr-js dr-js deleted the patch-1 branch January 15, 2021 00:22
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.

None yet

5 participants