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

[TRIVIAL] Ensure all sorting is ASCII-order consistently across platforms #5072

Merged
merged 3 commits into from
Jul 29, 2024

Conversation

ximinez
Copy link
Collaborator

@ximinez ximinez commented Jul 24, 2024

High Level Overview of Change

This change explicitly specifies LANG=C in the levelization script, which causes it to use plain ASCII ordering, and should cause the levelization results to be consistent across environments.

Context of Change

This is a follow up to the physical redesign PR #4997. The merge / migration instructions in that PR note:

Check the sort order in Builds/levelization/results/ordering.txt. [...] Different versions of sort have been inconsistent on this matter in my experience. [...]

The cause of this inconsistency is not the version of sort, but the language settings of the shell, which varies depending on local configuration.

Type of Change

  • Chore (no impact to binary, e.g. .gitignore, formatting, dropping support for older tooling)

Before / After

Before:
On some machines, running ./Builds/levelization/levelization.sh will change the order of the results in ordering.txt.

After:
Running that script anywhere will give consistent results. Run against an unmodified develop branch, it should result in no changes.

Copy link

codecov bot commented Jul 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.4%. Comparing base (8fc805d) to head (dc6fbe6).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##           develop   #5072   +/-   ##
=======================================
  Coverage     71.3%   71.4%           
=======================================
  Files          796     796           
  Lines        67039   67039           
  Branches     10868   10868           
=======================================
+ Hits         47832   47833    +1     
+ Misses       19207   19206    -1     

see 1 file with indirect coverage changes

Impacted file tree graph

@ximinez ximinez added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Jul 25, 2024
@Bronek Bronek self-requested a review July 29, 2024 10:43
Copy link
Collaborator

@Bronek Bronek left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this !

@ximinez ximinez merged commit 2820feb into XRPLF:develop Jul 29, 2024
19 of 20 checks passed
@ximinez ximinez deleted the pr/levelization branch July 29, 2024 23:02
vlntb pushed a commit to vlntb/rippled that referenced this pull request Aug 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. Trivial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants