Update buildSpagoLock and refactor #71

deemp · 2024-01-21T23:46:28Z

Update buildSpagoLock
Set mainProgram in packages so that lib.getExe works correctly (closes Set meta.mainProgram, use lib.getExe #68).
Update the generate package
- Update registry version
- Update code
Refactor flake.nix
Set alejandra as the default formatter
- Run nix fmt to format Nix files

deemp · 2024-01-22T23:14:09Z

@toastal, could you please review? IDK why garnix is unhappy. nix flake check works on my Linux.

toastal · 2024-01-23T18:01:16Z

flake.nix

  };

-  outputs = {
+  outputs = inputs@{


Doesn’t the { xx, ... }@inputs need to come after? Regardless, long-term these are easier to maintain as just calling inputs.spago in the one places you needed.

It can be before or after arguments, according to docs. Anyway, 6d5c973 doesn't introduce it anymore

toastal · 2024-01-23T18:03:07Z

flake.nix

+        overlays = [
+          self.overlays.default
+          slimlock.overlays.default
+          (final: prev: rec {


You want to avoid using rec

Sure. 6d5c973 doesn't introduce rec anymore.

toastal · 2024-01-23T18:05:37Z

generate/default.nix

@@ -31,7 +34,8 @@ in

    buildPhase = ''
      ln -s ${npmDependencies}/js/node_modules .
-      cp -r ${packages.${name}}/output .
+      cp -r ${packages.${name}}/{*,.*} .


Should this be ln -s too?

Yes. Fixed in 1638337

toastal · 2024-01-23T18:08:15Z

nix/build-spago-lock.nix

+          ${mkBuildCommands}
+
+          mkdir -p $out
+          cp -r .spago $out


same about ln -s. There are some trade offs, but I think it saves disk space more generally & an occasional optimization of the storage can clean up loose links.

AFAIU, it's only possible to symlink something in a derivation $out.

Hence, to cache the built output, it's necessary to copy it to $out at least once.

toastal · 2024-01-23T18:09:36Z

spago-dev.nix

+    spago
+    python3
+    nodejs
+    git


Git is needed!?

Yes. From the spago README:

Minimal dependencies: users should not be expected to install a myriad of tools on their system to support various workflows. Spago only expects git and purs to be installed.

It's needed because Spago uses git to interact with the registry repositories. If we only use spago in offline mode (ie. spago build --offline) then I believe the calls to fetch the registry will be skipped and we don't need git.

toastal · 2024-01-23T18:13:33Z

Generally things look okay, but I’m not entirely sure on much of the context here. So I can’t really comment on the larger picture here. One thing that feels like something that could be looked at is shrinking the srcs with lib.fileset https://nix.dev/tutorials/file-sets.html which would make the derivations smaller by removing files that aren’t used, but I can’t really tell where exactly--just spider sense.

deemp · 2024-01-24T21:38:17Z

One thing that feels like something that could be looked at is shrinking the srcs with lib.fileset

Yes, I had the same idea about lib.fileset.

However, I don't know whether it's possible to determine a "working set" of files that are necessary to build a package. Consider a case when a fileset is determined based on a local project directory. If that project contains symlinks to files located outside of that directory, symlinked files won't be included into the derivation src.

Thus, I think we should tell users to pass to buildSpagoLock an src built via lib.fileset so that:

they ensure all necessary paths are present
they have smaller derivations
they rebuild their derivations less often due to ignoring some irrelevant paths in ./.

toastal · 2024-01-25T06:11:46Z

Ah ha. That makes sense, especially with better context of how this is actually working now.

thomashoneyman · 2024-01-25T13:10:46Z

Hey @deemp! I’m happy to optimize the buildSpagoLock stuff, and we can use Spagonto build projects now that it has a working offline mode and the new lock file format.

However, I don’t want to build Spago from source if we can stick with the current approach of grabbing “binaries” from NPM. Would you mind explaining the motivation to build from source here? To be blunt it will make for a much faster review on my part if we narrowed to just changes to how buildSpagoLock etc works.

I apologize for slow responses this week, it’s a hectic one!

deemp · 2024-01-25T14:18:38Z

Hello, @thomashoneyman!

Would you mind explaining the motivation to build from source here?

My goal is to be able to incrementally package PureScript projects using Nix.
I want to separate a derivation for a package and a derivation for its dependencies.

I build spago here because:

I can debug the spago derivation and buildSpagoLock in the same flake
I can check the PR against CI

However, I don’t want to build Spago from source if we can stick with the current approach of grabbing “binaries” from NPM.

I think it's not necessary to build or provide the spago-dev (built from source) derivation here.
So, I can remove the commit that adds the spago derivation.

However, I think it may be convenient to have a derivation in the spago repo so that people can access the dev version without having to wait for a new release and an update of purescript-overlay.

I see that the spago repo uses purescript-overlay as an input (link). Hence, if buildSpagoLock can build a package using the spago executable in offline mode, the spago repo can provide a derivation at almost zero cost.

To be blunt it will make for a much faster review on my part if we narrowed to just changes to how buildSpagoLock etc works.

Currently,

buildSpagoPackages builds given packages because AFAIU spago can't build dependencies only (Build dependencies only purescript/spago#1181)
buildSpagoLock builds a given package given .spago and writable output of the package.

Now, I see that my code doesn't account for ~/.cache/spago-nodejs as dream2nix does (link).

Would you like to finish buildSpagoLock? If no, I'd like to ask several other questions.
Is interaction with that directory still necessary?
Where can I find the docs on it?
Why is garnix unhappy?
What else should be considered?

deemp · 2024-01-28T11:44:34Z

I've just realized I won't be able to build each package separately because spago wants ~/.cache/spago-nodejs and will insert registry entries for each package into an sqlite database (it'll take too long).

I can build all dependencies at once, but it's less granular than the current solution that uses just the purs compiler.

So, I'd rather fix the generated BuildInfo.purs format.

toastal · 2024-01-31T06:55:00Z

flake.nix

+        overlays = [
+          self.overlays.default
+          slimlock.overlays.default
+          (_: prev: { nodejs = prev.nodejs_20; })


What is the point of this? Do you need a specific feature from 20? Even if you did, you should do the check to see if it’s the current version is already greater as this will be unnecessary in the future or fail when v20 is deprecated.

fixed in 2b33591

toastal · 2024-01-31T06:58:02Z

generate/bin/src/Run.purs

@@ -434,7 +434,7 @@ writePursTidyUpdates updates = do
      pure (updateStable (updateUnstable named))

  case named' of
-    Nothing -> Console.log "Failed to update named manifest" *> liftEffect (Process.exit 1)
+    Nothing -> Console.log "Failed to update named manifest" *> liftEffect (Process.exit' 1)


Shouldn’t these be like Console.error?

IDK. @thomashoneyman?

BTW, there are quite many

Console.log "something" liftEffect $ Process.exit' 1

It may be better to use a dedicated function.

Sure, it probably ought to be Console.error; the way this is run all the output is going to the console so there won't be an observable difference, but it's the proper thing to do.

@deemp we could add a die function that wraps this up, I'm happy with that. It's what Spago does.

applied the suggestion in 62ce26e

thomashoneyman · 2024-02-01T15:46:12Z

I'm sorry I haven't looked at this yet — I'm having an exceptionally busy work + life period at the moment.

bakhtiyarneyman · 2024-02-01T20:31:32Z

@deemp Thank you for your contributions! I'm excitedly waiting to see it land to be able to send my own PRs.

I believe the maintainers of this repo are using alejandra for formatting (although I see a few exceptions here and there). I think you are using a different formatter? Is it possible for you to reformat the code you touched with alejandra?

toastal · 2024-02-01T20:51:56Z

A check could be added like in <purescript/registry-dev@bc61b98>, but oof does alejandra have the worst of the formats IMO vs. nixpkgs-fmt or nixfmt. Upstream in Nixpkgs they are looking to make modifications to nixfmt to look more like nixpkgs-fmt, but that hasn’t yet landed -- toastal ไข่ดาว | https://toast.al PGP: 7944 74b7 d236 dab9 c9ef e7f9 5cce 6f14 66d4 7c9e

deemp · 2024-02-01T21:32:27Z

I believe the maintainers of this repo are using alejandra for formatting (although I see a few exceptions here and there).

@bakhtiyarneyman, I preferred nixpkgs-fmt because of kamadorueda/alejandra#372. However, as that issue is resolved now, I should check alejandra again.

In this repo, I initially used my hands ™️ to not break the surrounding formatted code. I noticed that mostly only my code was re-formatted after I added alejandra as the default formatter and formatted the code. Hence, I think you're right that maintainers used alejandra.

deemp · 2024-02-01T21:37:05Z

A check could be added like in purescript/registry-dev@bc61b98

Neat trick!

thomashoneyman · 2024-02-01T21:41:05Z

For what it’s worth I don’t have an opinion on what formatter is used so long as we use one. If it’s easier for yall to use a different one then I’m fine with that.

deemp · 2024-02-01T21:43:56Z

The default formatter for now is alejandra.

One can run nix fmt to format all files.

toastal · 2024-02-02T14:54:38Z

NixOS/rfcs#166

Sorry. To be clear I was referring to this future Nixpkgs RFC/merge request which looks to to fork nixfmt. https://github.com/piegamesde/nixpkgs/tree/nixfmt-2

deemp · 2024-02-03T12:34:36Z

@toastal, I see the implementation isn't merged yet. Could you please create a separate issue that suggests to use the standard formatter once it's ready?

toastal · 2024-02-03T14:40:58Z

No, it’s not merged, but the style the style from the RFC will eventually apply to all of Nixpkgs & by proxy trickle down as a largely defacto style. Deviating from that tends to make just another hurdle to contribution & alejandra’s output is the most dissimilar to what is actively being worked on. Their fork of nixfmt will look a bit closer to nixpkgs-fmt’s output… which won’t have trailing commas, won’t try to cuddle let, space inside inline lists/attrsets, etc. We can see here alejandra will need to wrestle with adopting the style, deprecating themselves, or remaining a niche formatter for those that prefer it to the community standard. Unless its format is truly desired by this package’s maintainer, it doesn’t seem like the wisest choice at present.

deemp · 2024-02-03T18:22:09Z

Could you please create a separate issue that suggests to use the standard formatter once it's ready?

I've created one (#75).

Unless its format is truly desired by this package’s maintainer, it doesn’t seem like the wisest choice at present.

I think it's enough for my PR to keep the de-facto used formatting to have relatively small diff.

Hence, if you merge into main another PR that uses a different formatter, I'll rebase my PR on main and format the code using that formatter.

generate/lib/src/Utils.purs

generate/spago.yaml

thomashoneyman · 2024-02-08T21:26:45Z

manifests/build-support/mkDerivationBasic.nix

@@ -33,5 +33,5 @@ stdenv.mkDerivation {
    ln -s $BIN $out/bin/${name}
  '';

-  meta = meta lib;
+  meta = meta lib // {mainProgram = name;};


Is this record merge necessary if the meta function is already creating a meta attr with mainProgram = pname;? This looks like a redundant merge to me.

The meta function didn't set mainProgram. Hence, there was this merge. I feel it's better to set mainProgram where mkDerivationBasic.nix function is called (09bdc08) because then,mainProgram can be changed independently of name.

thomashoneyman

Looks good to me, thank you! Just a couple comments and, once addressed, we can merge.

Co-authored-by: Thomas Honeyman <hello@thomashoneyman.com>

deemp mentioned this pull request Jan 21, 2024

Build spago from source purescript/spago#981

Draft

deemp force-pushed the main branch from 98687ab to a3fb4f0 Compare January 22, 2024 22:57

deemp force-pushed the main branch from a3fb4f0 to 14abdfb Compare January 22, 2024 23:22

toastal reviewed Jan 23, 2024

View reviewed changes

deemp force-pushed the main branch from 14abdfb to b63e929 Compare January 24, 2024 23:40

deemp changed the title ~~Allow building packages via spago~~ Use spago in package derivations Jan 25, 2024

deemp force-pushed the main branch 2 times, most recently from 1e9e362 to 7f2a045 Compare January 28, 2024 14:29

deemp changed the title ~~Use spago in package derivations~~ Update buildSpagoLock and refactor Jan 28, 2024

deemp force-pushed the main branch 3 times, most recently from e6de7e3 to 6def68f Compare January 28, 2024 22:07

deemp added 7 commits January 29, 2024 11:51

chore: update spago locks

e28680e

chore: update npm dependencies

bc84bc1

feat: set mainProgram

b097886

chore: update registry version

4ff0584

fix: exits

95f5786

fix: handling process result

8163986

fix: buildInfo

dc96b49

hack: set spagoVersion

6ba7bb3

deemp force-pushed the main branch from 290f512 to 6ba7bb3 Compare January 29, 2024 08:51

deemp requested a review from toastal January 29, 2024 12:25

toastal reviewed Jan 31, 2024

View reviewed changes

fix: use the default nodejs nixpkgs package

2b33591

refactor: use die

62ce26e

deemp added 3 commits February 2, 2024 00:16

refactor: use eachDefaultSystem

3e21529

feat: set alejandra as the default formatter

b84267a

chore: format nix files

25e7b1b

deemp mentioned this pull request Feb 3, 2024

Use a standard Nix formatter as the default formatter #75

Open

thomashoneyman reviewed Feb 8, 2024

View reviewed changes

generate/lib/src/Utils.purs Outdated Show resolved Hide resolved

thomashoneyman reviewed Feb 8, 2024

View reviewed changes

generate/spago.yaml Outdated Show resolved Hide resolved

thomashoneyman reviewed Feb 8, 2024

View reviewed changes

thomashoneyman approved these changes Feb 8, 2024

View reviewed changes

deemp and others added 3 commits February 9, 2024 17:06

refactor: add newline at the end

4b81bcc

Co-authored-by: Thomas Honeyman <hello@thomashoneyman.com>

refactor: set mainProgram in individual derivations

09bdc08

refactor: add newline at the end

be88f37

Co-authored-by: Thomas Honeyman <hello@thomashoneyman.com>

thomashoneyman merged commit 4e151be into thomashoneyman:main Feb 9, 2024
295 checks passed

Update buildSpagoLock and refactor #71

Update buildSpagoLock and refactor #71

Conversation

deemp commented Jan 21, 2024 • edited Loading

deemp commented Jan 22, 2024

Choose a reason for hiding this comment

deemp Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deemp Jan 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deemp Jan 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toastal commented Jan 23, 2024

deemp commented Jan 24, 2024 • edited Loading

toastal commented Jan 25, 2024

thomashoneyman commented Jan 25, 2024

deemp commented Jan 25, 2024 • edited Loading

deemp commented Jan 28, 2024 • edited Loading

toastal Jan 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomashoneyman commented Feb 1, 2024

bakhtiyarneyman commented Feb 1, 2024 • edited Loading

toastal commented Feb 1, 2024 via email

deemp commented Feb 1, 2024

deemp commented Feb 1, 2024

thomashoneyman commented Feb 1, 2024 • edited Loading

deemp commented Feb 1, 2024

toastal commented Feb 2, 2024

deemp commented Feb 3, 2024 • edited Loading

toastal commented Feb 3, 2024

deemp commented Feb 3, 2024 • edited Loading

thomashoneyman Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomashoneyman left a comment

Choose a reason for hiding this comment

deemp commented Jan 21, 2024 •

edited

Loading

deemp Jan 25, 2024 •

edited

Loading

deemp Jan 25, 2024 •

edited

Loading

deemp Jan 24, 2024 •

edited

Loading

deemp commented Jan 24, 2024 •

edited

Loading

deemp commented Jan 25, 2024 •

edited

Loading

deemp commented Jan 28, 2024 •

edited

Loading

toastal Jan 31, 2024 •

edited

Loading

bakhtiyarneyman commented Feb 1, 2024 •

edited

Loading

thomashoneyman commented Feb 1, 2024 •

edited

Loading

deemp commented Feb 3, 2024 •

edited

Loading

deemp commented Feb 3, 2024 •

edited

Loading

thomashoneyman Feb 8, 2024 •

edited

Loading