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

order platforms alphabetically #480

Closed
ashleygwilliams opened this issue Jun 27, 2023 · 6 comments · Fixed by #544
Closed

order platforms alphabetically #480

ashleygwilliams opened this issue Jun 27, 2023 · 6 comments · Fixed by #544

Comments

@ashleygwilliams
Copy link
Member

Screenshot 2023-06-27 at 6 26 32 PM
@Plecra
Copy link
Contributor

Plecra commented Jul 26, 2023

Hiya :) Going to have a look at this

@Plecra
Copy link
Contributor

Plecra commented Jul 26, 2023

Aria poked around this a couple weeks back, and I think they saw the same sorted list of platforms I do on main:

image

/// A map from TargetTriples to Installers that support that platform
///
/// In theory this should be BTreeMap or IndexMap but something in the pipeline from here to
/// jinja seems to be forcing a sorting so it's deterministic..? Can't find docs for this.
type Platforms = HashMap<TargetTriple, Vec<InstallerIdx>>;

I'll see if I can confirm where this is being sorted, and check we're not just getting lucky hashes ;D

@shadows-withal
Copy link
Contributor

this PR #542 should guarantee that anything ordered passed into minijinja stays sorted, so hopefully that should help

@Plecra
Copy link
Contributor

Plecra commented Jul 26, 2023

Hm, I think that may not work alone :) I just found the same code and I think the BTreeMap representation was actually saving us? The HashMap repr of platforms_with_downloads: Platforms might shuffle the platforms, and the BTreeMap was sorting them correctly. Using an IndexMap (which might happen if another crate enabled the preserve_order feature in the dependency tree) will maintain the incorrect sort produced by the HashMap.

@Plecra
Copy link
Contributor

Plecra commented Jul 26, 2023

Oh even then we were getting lucky that the target triples had the same sort order as the display names! Adding a post process filter in the template is likely the easiest solution: https://docs.rs/minijinja/latest/minijinja/filters/fn.sort.html

I can PR that if you like?

@shadows-withal
Copy link
Contributor

that sounds good, go for it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants