Skip to content
This repository has been archived by the owner on Aug 11, 2022. It is now read-only.

Add silly logging for lifecycle.js #9227

Closed
wants to merge 1 commit into from
Closed

Add silly logging for lifecycle.js #9227

wants to merge 1 commit into from

Conversation

saper
Copy link
Contributor

@saper saper commented Aug 11, 2015

This is to reduce the amount of frustraction while troubleshooting various custom lifecycle script environment.

Cons: user's privacy, if any

@iarna
Copy link
Contributor

iarna commented Aug 24, 2015

FYI: We update the authors file automatically as part of our release process, so we don't look for that in pull requests.

As for this pull request, I'm happy to take this, but I think I'm going to remove the env logging as I fear that would make our already large logs much too big. If you'd like to pick out some key env vars, I'd probably be up for that though.

@iarna iarna added this to the 3.x-next milestone Aug 24, 2015
@saper saper force-pushed the ddd branch 2 times, most recently from e77b7c8 to ff6319c Compare August 24, 2015 10:28
@saper
Copy link
Contributor Author

saper commented Aug 24, 2015

I had this doubt too, but first I have no idea whether PATH or LD_LIBRARY_PATH or PATHEXT or COMSPEC or who-knows-what breaks pretty fragile lifecycle scripts. Additionally, having thousands of addNameRange logged already doesn't make it so bad in comparison, given that mostly up to three lifecycle scripts are invoked per npm run.

This is to reduce the amount of frustraction
while troubleshooting various custom
lifecycle script environment.

Cons: user's privacy, if any
iarna pushed a commit that referenced this pull request Aug 25, 2015
To make debugging of lifecycle scripts easier

PR-URL: #9227
@othiym23
Copy link
Contributor

Landed, with a bit of tweaking by @iarna, as d088b7d, included in npm@3.3.1. Thanks for putting this together and your time, @saper!

@othiym23 othiym23 closed this Aug 28, 2015
@othiym23 othiym23 removed the review label Aug 28, 2015
@saper
Copy link
Contributor Author

saper commented Aug 28, 2015

Thanks, commented on one little nit. Can we have this in 2.x as well?

@othiym23
Copy link
Contributor

Can we have this in 2.x as well?

The team decided that it wasn't worth the time to backport this to 2.xnpm@3 is very close to coming out of beta, and going forward, we want to switch to mostly fixing only critical or security issues in npm@2, not adding new functionality.

@saper
Copy link
Contributor Author

saper commented Sep 11, 2015

Can we discuss the 2.x issue again? This was a missing feature from the start. I got another issue with running lifecycle script in some kind of restricted environment (sass/node-sass#1138) and the log says only "17721 info node-sass@3.3.2 Failed to exec install script". npm 2.x will be around for some time and it is really difficult to troubleshoot anything related to custom lifecycle scripts this way....

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants