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

Ensure Consistent Order of build_files in WriteAutoRegenerationRule #3062

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ben-zalekta-lmnd
Copy link

@ben-zalekta-lmnd ben-zalekta-lmnd commented Aug 19, 2024

Fixes #3061

This PR addresses an issue with the WriteAutoRegenerationRule function in node-gyp, where the build_files set was passed without sorting, leading to an inconsistent order of dependencies in the generated Makefile.

Changes:

Wrapped the build_files set in the sorted() function before it is processed in WriteAutoRegenerationRule. This ensures that the order of files is consistent across all runs, preventing issues in CI environments that rely on file hash comparisons.
Impact:
This change stabilizes the output of the Makefile generation process, resolving the issue of inconsistent file order and reducing false positives in CI checks.

Checklist
  • npm install && npm run lint && npm test passes
  • tests are included NO, There are no tests for this code area
  • documentation is changed or added There is not documantation for this code area
  • commit message follows commit guidelines
Description of change

Wrapped the build_files set in the sorted() function before it is processed in WriteAutoRegenerationRule. This ensures that the order of files is consistent across all runs, preventing issues in CI environments that rely on file hash comparisons.
@cclauss
Copy link
Contributor

cclauss commented Aug 19, 2024

I added the first line to the comment message to link the pull request to the issue as discussed in

There is a small performance decrease because of the "function all overhead" but I think it negligible if others agree.

@cclauss cclauss self-requested a review August 19, 2024 15:46
@ben-zalekta-lmnd
Copy link
Author

@cclauss Can you approve and merge?

@zhan9san
Copy link

It would be great if this PR can be merged and released.

Thanks for your work.

@ben-zalekta-lmnd
Copy link
Author

@cclauss How can we proceed and merge those changes

Copy link
Member

@lukekarrys lukekarrys left a comment

Choose a reason for hiding this comment

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

Requesting changes because I think this change would be overwritten the next time gyp-next is updated.

@cclauss Should this change be landed in that repo instead? https://github.com/nodejs/gyp-next/blob/db890891192c9b3d19dc2f576b81fafde280d1aa/pylib/gyp/generator/make.py#L2386

If I'm wrong here I will approve instead.

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.

Inconsistent Order of build_files in WriteAutoRegenerationRule Causes CI Failures
4 participants