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

Flake filesets + Flake check refactor + check Nix formatting #685

Merged
merged 8 commits into from
Feb 8, 2024

Conversation

toastal
Copy link
Contributor

@toastal toastal commented Jan 24, 2024

If Microsoft GitHub supported stacked diffs this would look less bad… this is meant to be read patch by patch, including the patch message (not just title) where it’s actually not too bad to read IMO.

Anyhow: I was looking to do a bit of hacking on this project, but the Nix build times & flake check timings were slow (CONTRIBUTING says to run nix flake check) & the Nix formatting was inconsistent unlike the PureScript.

Highlights:

  • upgrade Nixpkgs …to get:
  • use lib.fileset to pick just the files needed to check
  • add a flake check for nixfmt -- the formatter specified devShell (& looks like in the future will be defacto after it is tweaked)
  • actually run nixfmt accross the project (& add it to the blame ignore)
  • refactor 2 of checks to not use the heavy-duty mkDerivation which comes with a C compiler
  • run the Dhall checks in parallel

Questions / follow-ups:

  • 23.05 is the OS version for a VM & another system… should those upgrade or not? If no, I could pull in 23.11 just for lib.fileset & continue to use the older set for the packages
  • filesets should be used for the various apps (be wary of src = ./. as any time any file in the folder changes, the that derivation needs to rebuild, even when the changes don’t affect the files it touches (e.g. editing the project’s .purs file shouldn’t make users rebuild the Spago & Node packages)); I don’t understand the project enough to guess how those are set up to know what the correct fileset would be, & the scope creep is enough
  • It was a bonus, but I had to git stash a check for editorconfig-checker to verify .editorconfig since there is a bug with how purs-tidy handles 2-char symbols for indentation that makes files no longer actually 2-space indented. While I love the ‘fix’ of turning Unicode on & ASCII off, it seems many don’t like it :(. There are options to fix the situation, but it involves modifying .tidyrc.json or relaxing .editorconfig :| As it stands, the project isn’t compliant with its .editorconfig.

Notes:

  • I tested the new+refactored checks do pass, but also modifying (then reverting) code to make sure it fails too (no silent ✅)
  • The robust tests really help verify the state of the project!

namely to get access to `pkgs.lib.fileset`
@toastal
Copy link
Contributor Author

toastal commented Jan 24, 2024

Hold up… tiny tweak needed to delete a line.

@toastal toastal force-pushed the flake-check-refactor branch 2 times, most recently from 9726a24 to 9c2648b Compare January 24, 2024 16:42
@toastal
Copy link
Contributor Author

toastal commented Jan 24, 2024

check nix-format

Cute. The CI already shows it.

@thomashoneyman
Copy link
Member

Thanks for this work! I will give it a proper review this week.

In the meantime — totally fine to upgrade to 23.11, there isn’t a specific reason to avoid upgrades. The VM uses 23.11 so we can test the same configuration that the server uses. This repository deploys on commits to master.

At a glance I’m in favor of the changes you’ve made. I’m but a mere Nix dabbler so most of this looks like optimizations I ought to have made from the start.

being specific about the fileset will trigger less rebuilds
• name: check-format → purescript-format to reflect that it only targets
  PureScript files
• stdenv.mkDerivation → runCommand as most of the machinary isn’t used &
  we don’t need a C compiler (mkDerivationNoCC is for that)
• use fileset to not include directories that don’t need to be checked
• tee error output to file/$out
• stdenv.mkDerivation → runCommand as most of the machinary isn’t used &
  we don’t need a C compiler (mkDerivationNoCC is for that)
• use fileset to not include directories that don’t need to be checked
• tee error output to file/$out
• run all checks in parallel with GNU parallel
@toastal
Copy link
Contributor Author

toastal commented Jan 25, 2024

@thomashoneyman I ‘snuck’ in a commit about system.stateVersion moving to 23.11 as you mentioned. Tests passing locally.

@thomashoneyman
Copy link
Member

@toastal Oh, I thought you meant using nixpkgs 23.11 — while I think it’s fine to change the state version (we’re not using anything that relies on it that I know of, like postgres) I don’t see the harm in leaving that alone. What’s the advantage of changing it?

@toastal
Copy link
Contributor Author

toastal commented Jan 25, 2024 via email

@toastal
Copy link
Contributor Author

toastal commented Jan 28, 2024

@thomashoneyman hold on… I re-read & now I’m re-unclear. Do you want me to drop b2fa76d5a7cf5c6279afb480cce8378954ff47f3 (system.stateVersion: 23.05 → 23.11) or no?

@thomashoneyman
Copy link
Member

It’s ok to upgrade that field since we’re not using anything that’s reliant on the state version. (Yet, at least.)

@thomashoneyman
Copy link
Member

There are options to fix the situation, but it involves modifying .tidyrc.json or relaxing .editorconfig :| As it stands, the project isn’t compliant with its .editorconfig.

The source of truth is the .tidyrc so we should relax .editorconfig in that case. We don't use unicode symbols in the codebase.

Copy link
Member

@thomashoneyman thomashoneyman left a comment

Choose a reason for hiding this comment

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

Thank you! 💪

@thomashoneyman thomashoneyman merged commit 4d3ca5a into purescript:master Feb 8, 2024
16 checks passed
@toastal
Copy link
Contributor Author

toastal commented Feb 10, 2024

cool

@toastal toastal deleted the flake-check-refactor branch February 10, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants