Skip to content

Fixes for #1188; custom_commands broken #1230

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

GideonBear
Copy link
Member

@GideonBear GideonBear commented Jul 17, 2025

@themadprofessor You had some mistakes in #1188 which we both didn't spot:

  • CustomCommands is broken and running pre_commands instead.
  • NixHelper and Remotes are missing from default_steps
  • Pkg is missing from default_steps on target_os = "android"

A few other changes:

  • I added a few more *self where I could.
  • Combined step-enum-sorted and step-match-sorted into custom-checks
  • Added default_steps custom check to check if it contains every step
  • Refactored default_steps to contain less overlapping cfg calls

@themadprofessor please review when you can.

@GideonBear
Copy link
Member Author

Never mind, I cannot merge this myself. @SteveLauC could you review and merge as soon as possible?

@GideonBear GideonBear added the U-0 Bug affecting users across all/multiple steps label Jul 17, 2025
Copy link
Contributor

@AThePeanut4 AThePeanut4 left a comment

Choose a reason for hiding this comment

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

IMO, I prefer the way step ordering was defined before #1188, with separate blocks for each OS, even if they have overlap. It makes it easier to see at a glance which steps will be run for each OS.

So I think the system-specific section of default_steps() should look more like this:

#[cfg(windows)]
steps.extend_from_slice(&[Wsl, WslUpdate, Chocolatey, Scoop, Winget, System, MicrosoftStore]);

#[cfg(target_os = "macos")]
steps.extend_from_slice(&[BrewFormula, BrewCask, Macports, Xcodes, Sparkle, Mas, System]);

#[cfg(target_os = "dragonfly")]
steps.extend_from_slice(&[Pkg, Audit]);

#[cfg(target_os = "freebsd")]
steps.extend_from_slice(&[Pkg, System, Audit]);

#[cfg(target_os = "openbsd")]
steps.extend_from_slice(&[Pkg, System]);

#[cfg(target_os = "android")]
steps.push(System);

#[cfg(target_os = "linux")]
steps.extend_from_slice(&[
    System,
    ConfigUpdate,
    AM,
    AppMan,
    DebGet,
    Toolbx,
    Snap,
    Pacstall,
    Pacdef,
    Protonup,
    Distrobox,
    DkpPacman,
    Firmware,
    Restarts,
    Flatpak,
    BrewFormula,
    Lure,
    Waydroid,
    AutoCpufreq,
    CinnamonSpices,
]);

So even though the System step is duplicated several times, it is really easy to see the list of steps that any particular system will run.

@GideonBear
Copy link
Member Author

GideonBear commented Jul 20, 2025

IMO, I prefer the way step ordering was defined before #1188, with separate blocks for each OS, even if they have overlap. It makes it easier to see at a glance which steps will be run for each OS.

I agree actually, I did not realize that #1188 added any overlap, I thought that was there before.

I noticed "android" should run Pkg (#1188 forgot that), not System, but I mostly used your example. Thx!

@GideonBear GideonBear requested a review from AThePeanut4 July 20, 2025 10:10
@themadprofessor
Copy link
Contributor

Good spots. Having the target_os steps split up is definitely a better choice. All looks good to me.

@GideonBear GideonBear requested review from SteveLauC and removed request for AThePeanut4 July 21, 2025 16:51
Copy link
Contributor

@AThePeanut4 AThePeanut4 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
U-0 Bug affecting users across all/multiple steps
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants