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

lib: log as yes/no whether build dir was created #2370

Merged

Conversation

DeeDeeG
Copy link
Contributor

@DeeDeeG DeeDeeG commented Apr 11, 2021

Checklist
Description of change

When logging "build" dir needed to be created?, put "Yes" or "No" instead of [path that was created] or undefined .

There is a bit of verbose logging in the package that informs the user whether the build dir needed to be created (as opposed to having already been there from a previous node-gyp configure run).

This bit of logging apparently expected to be given a boolean, but was receiving either a path or undefined based on the result of fs.mkdir(). So it was logging either the path of the build dir that was just created or undefined, somewhat nonsensically if you don't know what's going on behind the scenes.

Now it prints either "Yes" or "No".

Before:

gyp verb build dir "build" dir needed to be created? /Users/[user]/[package-name]/build,
gyp verb build dir "build" dir needed to be created? undefined

After:

gyp verb build dir "build" dir needed to be created? Yes,
gyp verb build dir "build" dir needed to be created? No

Note: The CI failure was a timeout on a single Ubuntu run. All other runs passed. This happens to me seemingly at random/as flakiness. I don't think it is caused by this tiny PR. CI passed for this commit at my fork: https://github.com/DeeDeeG/node-gyp/actions/runs/739328726 I believe it would pass here if re-run.

This bit of logging apparently expected to be given a boolean, but was
receiving either a path or undefined based on the result of fs.mkdir.

Now it prints either "Yes" or "No",
rather than printing either a path or "undefined", respectively.
@imatlopez
Copy link
Contributor

I think the path is more helpful? undefined might be ugly but is it worth fixing it for just that case?

@DeeDeeG
Copy link
Contributor Author

DeeDeeG commented Apr 12, 2021

I think the path is more helpful?

Logging the path is very helpful, but it is already logged immediately before, so printing it again is redundant.

gyp verb build dir attempting to create "build" dir: /Users/[user]/[package-name]/build
gyp verb build dir "build" dir needed to be created? /Users/[user]/[package-name]/build

worth fixing [...] ?

I think that's a subjective decision for the maintainers to make. But I think the current logging is confusing, in a way that seems like an accident or oversight. (When printing needed to be created? [path], It seems the log poses a question and then does not answer it. When printing needed to be created? undefined, I as a user have no idea what that means.) It makes the logs more confusing to read and comes across to me as a (relatively trivial) bug.

If maintainers prefer it to print something more-easily understood, like I have done in this PR, they can click a button to merge this. (Okay they also have to figure out the PR-URL: and Reviewed-By: stuff for the commit message. Which I can agree is the kind of tedious thing that might make me think twice about acting on a smaller or less critical PR. It's the little things that can make maintenance a chore...) Anyhow, despite its relative triviality, I felt this was worth at least proposing to maintainers.

P.S. Now that I am thinking explicitly about maintainers' time... I have hidden away the long explanation in the PR body behind a <details> expandable key, and put a much simpler summary there. That should save the maintainers a few minutes of reading! It's a one-line change, plus some formatting whitespace. Reviewing the code would probably take less than half time than reading what I've written here!

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, given the earlier log.verbose('build dir', 'attempting to create "build" dir: %s', buildDir) a few lines above.

Copy link
Contributor

@imatlopez imatlopez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah overlooked that you said it is dupe printing it, you've got a point :)

@gengjiawen gengjiawen merged commit 245dee5 into nodejs:master May 19, 2021
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.

4 participants