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

tools: make utils.SearchFiles deterministic #44496

Merged
merged 1 commit into from Oct 4, 2022

Conversation

brjsp
Copy link
Contributor

@brjsp brjsp commented Sep 2, 2022

glob.glob on Linux returns files in the order returned by the filesystem driver,
and the output from this function is stuffed by the Electron build process
straight into the config.gypi header, causing non-reproducible builds.

See this log for an example of the nondeterminism:
https://rb.zq1.de/compare.factory-20220901/diffs/nodejs-electron-compare.out

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. python PRs and issues that require attention from people who are familiar with Python. labels Sep 2, 2022
lpinca
lpinca approved these changes Sep 2, 2022
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 2, 2022
@nodejs-github-bot
Copy link
Contributor

Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

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

cc @nodejs/build

@richardlau richardlau added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Sep 5, 2022
@nodejs-github-bot
Copy link
Contributor

@tniessen
Copy link
Member

tniessen commented Sep 5, 2022

@brjsp Would you mind force-pushing the same commit with an amended commit message that adheres to the commit message guidelines?

`glob.glob` on Linux returns files in the order returned by the
filesystem driver, and the output from this function is stuffed by
the Electron build process straight into the `config.gypi` header,
causing non-reproducible builds.

See this log for an example of the nondeterminism:
https://rb.zq1.de/compare.factory-20220901/diffs/nodejs-electron-compare.out
@brjsp
Copy link
Contributor Author

brjsp commented Sep 5, 2022

@tniessen done

@tniessen tniessen added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 5, 2022
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot
Copy link
Contributor

sxa
sxa approved these changes Oct 4, 2022
Copy link
Member

@sxa sxa left a comment

Choose a reason for hiding this comment

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

Is the function used guaranteed to be deterministic or can it be influenced by the locale setting on the machine? Either way, this will at least allow a build environment with a documented, fixed, locale to produce deterministic output, so I'm happy to approve, although there could be a small performance overhead by doing this frequently on larger directories.

@nodejs-github-bot
Copy link
Contributor

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 4, 2022
@nodejs-github-bot nodejs-github-bot merged commit e2ca29f into nodejs:main Oct 4, 2022
51 checks passed
@nodejs-github-bot
Copy link
Contributor

Landed in e2ca29f

@brjsp
Copy link
Contributor Author

brjsp commented Oct 4, 2022

@sxa the default string comparator in Python sorts by codepoint value, ignoring any locale settings.

danielleadams pushed a commit that referenced this pull request Oct 11, 2022
`glob.glob` on Linux returns files in the order returned by the
filesystem driver, and the output from this function is stuffed by
the Electron build process straight into the `config.gypi` header,
causing non-reproducible builds.

See this log for an example of the nondeterminism:
https://rb.zq1.de/compare.factory-20220901/diffs/nodejs-electron-compare.out

PR-URL: #44496
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Stewart X Addison <sxa@redhat.com>
@richardlau richardlau added lts-watch-v14.x PRs that may need to be released in v14.x. lts-watch-v16.x PRs that may need to be released in v16.x. labels Dec 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. lts-watch-v14.x PRs that may need to be released in v14.x. lts-watch-v16.x PRs that may need to be released in v16.x. needs-ci PRs that need a full CI run. python PRs and issues that require attention from people who are familiar with Python.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants