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

chore: refactor the creation of config.gypi file #2491

Merged
merged 1 commit into from
Sep 10, 2021

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Sep 6, 2021

Checklist
  • npm install && npm test passes
  • tests are included
  • documentation is changed or added
  • commit message follows commit guidelines
Description of change

This PR refactors the configure.js file to move the creation of config.gypi into a separate create-config-gypi.js file, as part of the refactoring I have also added a few tests. There should be no affects to users.

Note that there is a small behavior change that the existence of process.config is no longer tested, the property was added in Node.js v0.7.7 and node-gyp no longer runs on those ancient versions.

This refactoring is preparation work to implement #2490.

/cc @rvagg

@zcbenz zcbenz force-pushed the refactor-config-gypi-creation branch from ccfbf13 to 78c4bd9 Compare September 6, 2021 05:39
Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

yeah, looks good to me, I don't see any behaviour changes here other than the process.config existence check

anyone else want to weigh in?

@rvagg rvagg merged commit f2ad87f into nodejs:master Sep 10, 2021
@rvagg
Copy link
Member

rvagg commented Sep 10, 2021

good work @zcbenz, thanks for spending the time to do this, I look forward to your additional changes but you'll have to be patient with us (me mainly) due to bandwidth constraints

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