Fix: write package manifest.pathenvs in deterministic order#6882
Fix: write package manifest.pathenvs in deterministic order#6882waruqi merged 2 commits intoxmake-io:devfrom
Conversation
Summary of ChangesHello @romeoxbm, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where package manifest files ( Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the non-deterministic ordering of manifest.pathenvs by using an ordered iterator, which is a great improvement for build reproducibility. My review includes a suggestion to refactor the implementation for better conciseness and to leverage an existing helper function, which should improve code maintainability.
xmake/core/package/package.lua
Outdated
| -- ensure pathenvs are written deterministically: use ordered iteration | ||
| do | ||
| local arr = {} | ||
| for name in self:_pathenvs():orderitems() do | ||
| if name ~= nil then | ||
| table.insert(arr, name) | ||
| end | ||
| end | ||
| manifest.pathenvs = arr | ||
| end |
There was a problem hiding this comment.
This block of code can be simplified by using the existing table.to_array helper function, which converts an iterator to an array. This makes the code more concise and readable.
-- ensure pathenvs are written deterministically
manifest.pathenvs = table.to_array(self:_pathenvs():orderitems())
There was a problem hiding this comment.
That should do the trick as well. Will update the code.
Problem
The
manifest.pathenvsarray written tomanifest.txtwas generated from a hashset viato_array().hashset:to_array()iterates with Lua'snext()and therefore the element order is not deterministic.This produced non-deterministic
manifest.txtcontent across builds, which caused package hashes to changeeven when the set of path env names was logically identical.
Change
When saving package manifests,
manifest.pathenvsis now constructed using the ordered iteratorself:_pathenvs():orderitems()so the resulting array is written in a stable, deterministic order.This eliminates spurious manifest diffs and unstable package hashes caused only by iteration order.
Why this fix
Notes
PATHappears first), I can adjust the sorting to a custom comparator. For now,orderitems()sorts keys in a consistent manner.Tests
manifest.txtcontents caused solely by theorder of
pathenvs.