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

stylua: 0.14.3 -> 0.15.0 #192279

Merged
merged 2 commits into from
Sep 22, 2022
Merged

stylua: 0.14.3 -> 0.15.0 #192279

merged 2 commits into from
Sep 22, 2022

Conversation

figsoda
Copy link
Member

@figsoda figsoda commented Sep 21, 2022

Description of changes

I don't know what the default feature set should be. The original repo has no default features, but I haven't been following the original. I also don't want to break backwards compatibility in nixpkgs, but 5.2 is kinda a weird version to stop at, so I changed to default to 5.4

JohnnyMorganz/StyLua@v0.14.3...v0.15.0
https://github.com/JohnnyMorganz/StyLua/blob/main/CHANGELOG.md

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.11 Release Notes (or backporting 22.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@zowoq
Copy link
Contributor

zowoq commented Sep 21, 2022

https://github.com/JohnnyMorganz/StyLua#with-github-releases
By default, these are built with all syntax variants enabled (Lua 5.2, 5.3, 5.4 and Luau), to cover all possible codebases

Upstream's binaries are built with all-features, seems reasonable to follow that?

@figsoda
Copy link
Member Author

figsoda commented Sep 21, 2022

I will turn on lua54 and luau then, not turning on lua52 and lua53 by default so it is easier to do e.g. lua51 only

@zowoq
Copy link
Contributor

zowoq commented Sep 21, 2022

I'd rather we follow upstream binaries and do all-features or follow the crate default and have none.

@figsoda
Copy link
Member Author

figsoda commented Sep 22, 2022

the lua54 feature implies lua53, and the same for lua52
I don't think something like allFeatures ? true fits here

@zowoq
Copy link
Contributor

zowoq commented Sep 22, 2022

How about something like this so we don't need to pick and/or track specific features and people can still override it if they need to?

diff --git a/pkgs/development/tools/stylua/default.nix b/pkgs/development/tools/stylua/default.nix
index 492263f2c36..ee997a6839c 100644
--- a/pkgs/development/tools/stylua/default.nix
+++ b/pkgs/development/tools/stylua/default.nix
@@ -1,10 +1,7 @@
 { lib
 , rustPlatform
 , fetchFromGitHub
-, lua52Support ? false
-, lua53Support ? false
-, lua54Support ? true
-, luauSupport ? true
+, buildFeatures ? "all-features"
 }:
 
 rustPlatform.buildRustPackage rec {
@@ -25,10 +22,7 @@ rustPlatform.buildRustPackage rec {
     rm .cargo/config.toml
   '';
 
-  buildFeatures = lib.optional lua52Support "lua52"
-    ++ lib.optional lua53Support "lua53"
-    ++ lib.optional lua54Support "lua54"
-    ++ lib.optional luauSupport "luau";
+  inherit buildFeatures;
 
   meta = with lib; {
     description = "An opinionated Lua code formatter";

@figsoda
Copy link
Member Author

figsoda commented Sep 22, 2022

I don't think all-features would have worked in this case so I just switched to [ "lua54" "luau" ], also named it features instead of buildFeatures

@zowoq
Copy link
Contributor

zowoq commented Sep 22, 2022

I don't think all-features would have worked in this case

Why not?

@figsoda
Copy link
Member Author

figsoda commented Sep 22, 2022

error: Package stylua v0.15.0 (/build/source) does not have the feature all-features

in ci it is taken as a flag (--all-features) and --features all-features doesn't work

@figsoda
Copy link
Member Author

figsoda commented Sep 22, 2022

added a release notes entry since this breaks backwards compatibility

@zowoq
Copy link
Contributor

zowoq commented Sep 22, 2022

error: Package stylua v0.15.0 (/build/source) does not have the feature all-features

in ci it is taken as a flag (--all-features) and --features all-features doesn't work

Set buildflags/checkflags to use --all-features if features is unset?

@figsoda
Copy link
Member Author

figsoda commented Sep 22, 2022

that sounds like a lot for not a lot of benefit, the newest lua version is 5.4 and there probably won't be much more new features

@zowoq
Copy link
Contributor

zowoq commented Sep 22, 2022

the lua54 feature implies lua53, and the same for lua52

Apart from this there a reason not to have 52 and 53 here? If we not going to actually use all-features I think we should set the equivalent as the default.

@figsoda
Copy link
Member Author

figsoda commented Sep 22, 2022

because it is shorter and this is the equivalent, lua54 implies lua53 and lua52
https://github.com/JohnnyMorganz/StyLua/blob/v0.15.0/Cargo.toml#L29-L30

@zowoq
Copy link
Contributor

zowoq commented Sep 22, 2022

because it is shorter and this is the equivalent, lua54 implies lua53 and lua52

Yes, I was asking if that was the only reason. For clarity I'd rather we set all of the features.

Co-authored-by: zowoq <59103226+zowoq@users.noreply.github.com>
@figsoda
Copy link
Member Author

figsoda commented Sep 22, 2022

it was the only reason, conditionally setting buildFlags just seemed kinda hacky to me when the current solution works well enough

@zowoq zowoq merged commit 2bf91a6 into NixOS:master Sep 22, 2022
@figsoda figsoda deleted the update-stylua branch September 22, 2022 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants