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

Build spago from source #981

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Build spago from source #981

wants to merge 3 commits into from

Conversation

deemp
Copy link
Contributor

@deemp deemp commented Aug 1, 2023

Depends on thomashoneyman/purescript-overlay#71

Description of the change

  • Set spago built from sources to be the default package
  • Add a dev devshell to check spago completions and version

@thomashoneyman
Copy link
Member

See also: #963

The big question here is whether @f-f wants to build spago with Nix or not. If so, then the next question is what to do about the generated BuildInfo file, which is necessary for the build. In the linked PR I made a fake one and included it in the build with writeText, but of course that's quite hacky and if the format of the file falls out of date then the build will be incorrect.

If there isn't a desire to build Spago with Nix, then all that's really needed is a dev shell and this PR could be trimmed down to only include that.

Comment on lines 19 to 22
easy-purescript-nix = {
url = "github:justinwoo/easy-purescript-nix";
inputs.flake-utils.follows = "flake-utils";
};
Copy link
Member

Choose a reason for hiding this comment

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

Presumably this and purescript-overlay are both included because purifix depends on easy-purescript-nix. Maybe if the easy-purescript-nix dependency is forced then it should just be used in place of purescript-overlay.

@deemp
Copy link
Contributor Author

deemp commented Aug 2, 2023

  • I removed Spago.Generated.BuildInfo mport because it's not necessary for a build.
  • I rebased onto Switch to optparse #980 to try completions.

Now, nix build complains about ✘ [ERROR] Could not resolve "fs-extra"

@f-f
Copy link
Member

f-f commented Aug 2, 2023

@deemp a few notes:

  • we'll need the Spago.Generated.BuildInfo import for the spago --version flag to work (see the review of #980)
  • @thomashoneyman's purescript-overlay has a derivation for building Spago. That's the "canonical" way (for now) to build Spago with Nix, so if we want such a Nix recipe in this repo instead then we should just port that one over. I entrust Thomas to be able to give the best guidance on this

@thomashoneyman
Copy link
Member

thomashoneyman commented Aug 2, 2023

This is the derivation that was used to build Spago, which I believe @f-f is referring to:
https://github.com/thomashoneyman/purescript-overlay/blob/6363b8129ea057adf69eb65650ed4035c1a7e48d/nix/mkSpagoDerivation.nix

Now, though, Spago is built by recording the hash and url of its tarball from NPM:
https://github.com/thomashoneyman/purescript-overlay/blob/339dc45f5f883644ce59bd95d23753c93845093e/manifests/spago.json#L152-L155

The overlay just unpacks the tarball. So that wouldn't be suitable for building Spago from source here (but the first derivation could be).

@deemp
Copy link
Contributor Author

deemp commented Aug 3, 2023

@f-f, thank you for reviews. I haven't noticed the usage of Spago.Generated.BuildInfo because I commented out the version flag 🤦.

I'll try to write a fixed-output derivation to build spago with spago. Probably, it'll be possible to automate hash production.

@deemp
Copy link
Contributor Author

deemp commented Aug 4, 2023

I tried to use dream2nix to provide node_modules.

js = (inputs.dream2nix.lib.makeFlakeOutputs {
  systems = [ system ];
  config.projectRoot = ./.;
  source = ./.;
  # autoProjects = true;
  projects = {
    spago = {
      name = "spago";
      relPath = "";
      subsystem = "nodejs";
      translator = "package-lock";
      subsystemInfo = {
        nodejs = 18;
      };
    };
  };
});

js-modules = js.packages.${system}.spago.overrideAttrs (x: {
  installPhase = ''
    mkdir $out
    cp -r node_modules/* $out
  '';
});

However, when I tried to copy them into a FOD, several node packages triggered an error. I found no info about this error.

error: illegal path references in fixed-output derivation '/nix/store/rjl96zjihzfwinb2l84rpnmqbxn12bv5-spago.drv'

These packages were cpu-features ssh2 mkdirp nan rimraf semver. The packages cpu-features and ssh2 had symlinks to python3. I don't know what was wrong with other packages.

I found bad packages via a ternary search by rm -rf the packages ls node_modules | head -n $H | tail -n $T.

Doing patchShebangs on the resulting bundle.js also led to that error.

I decided to just bundle in the FOD and apply any other improvements in other derivations.

@thomashoneyman
Copy link
Member

The previous stab at building spago with nix used slimlock and that worked just fine:
https://github.com/thomashoneyman/slimlock

@deemp
Copy link
Contributor Author

deemp commented Aug 6, 2023

I added a pre-commit hook that builds a spago FOD and writes its outputHash into the bin/outputHash file. This way, the FOD is always in sync with code.

The hook can be installed by entering the default devshell in ./spaghetto and running pre-commit install

On my computer, this hook took from several seconds to two minutes.

The FOD used slimlock and buildSpagoLock.

slimlock worked fine and fast.

spago bundle took most of the build time. I copied modules produced by pkgs.purix.buildSpagoLock, but spago ignored them and compiled everything from scratch. Copying was introduced in 74750e0.

@thomashoneyman, how can I tell spago to use pre-built modules?

@deemp
Copy link
Contributor Author

deemp commented Aug 6, 2023

Completions work in a devshell.

nix develop .#spago
spago ini<TAB>

@deemp
Copy link
Contributor Author

deemp commented Aug 7, 2023

Now, I believe the pre-commit part is excessive.
The FOD can be automatically generated from the source code, so the FOD should go into purescript-overlay.

The FOD should be available as a function of a commit hash and that FOD outputHash with the default outputHash being an empty string. This way, a user will be able to build spago from sources.

@deemp deemp marked this pull request as draft September 27, 2023 12:19
@f-f
Copy link
Member

f-f commented Sep 28, 2023

@deemp are you still interested in this? I would like to have a flake in Spago itself, if it's possible to keep it simple

@deemp
Copy link
Contributor Author

deemp commented Sep 28, 2023

@f-f, yes, I am. I have a couple of questions, though.

Does spago support offline builds? (related issue)

If no, we'll have to use a fixed-output derivation and store its hash somewhere. Main options:

  1. Store in purescript/spago. Since we want to associate a hash with a commit, we'll need to:
    1. In CI, build FOD to get its hash.
    2. Add this hash to the last commit, e.g., reset the last commit, save derivation hash in a file, stage the file, commit again, force push.
  2. Store in thomashoneyman/purescript-overlay. Then, we can:
    1. In CI, fetch the latest commit from purescript/spago.
    2. Build FOD, get its hash, and save it.
    3. Commit, push.

In 1, we'll have a derivation for each commit, but will have to force-push extended commits.
In 2, we'll have a derivation for each commit that was latest in purescript/spago before running a CI in thomashoneyman/purescript-overlay.

Please, let me know if there are other options.

Which one do you prefer?

@f-f
Copy link
Member

f-f commented Sep 28, 2023

@deemp we can (and should) definitely support offline building (which is my preferred option) - but that means that we would need the build inputs (the registry and registry-index repo, and the dependencies) to come via nix (not sure what's the best way to do it today, but we have a spago.lock file these days so it should be possible)

@thomashoneyman
Copy link
Member

They can just be inputs to the flake (see how purifix does it). They could then be copied into a .spago directory or spago could accept flags to pass the paths of the two repos, which in this case would be store paths.

@f-f
Copy link
Member

f-f commented Sep 29, 2023

Ah, I see. Then I guess #575 is the next blocker for this

@thomashoneyman
Copy link
Member

For what it's worth, you don't need to bring in the registry or registry-index to build Spago from its dependencies, because the lock file includes everything Nix needs. In purescript-overlay I was previously building Spago from source from the lockfile and package-lock.json alone.

@f-f f-f added the blocked label Oct 2, 2023
@f-f
Copy link
Member

f-f commented Oct 6, 2023

We now have a --offline flag (merged in #1044) so this is unblocked - time to figure out what's the next blocker 🙂

@f-f f-f removed the blocked label Oct 6, 2023
@f-f
Copy link
Member

f-f commented Oct 12, 2023

This comment is likely relevant to this PR: thomashoneyman/purescript-overlay#47 (comment)

@f-f
Copy link
Member

f-f commented Oct 15, 2023

@deemp still interested in contributing this or shall we close it?

@deemp
Copy link
Contributor Author

deemp commented Oct 15, 2023

@f-f, sorry, was busy with studies. I'm interested in this. I'll have time next week so please don't close it yet.

@f-f
Copy link
Member

f-f commented Oct 15, 2023

No worries at all. Thanks!

@deemp
Copy link
Contributor Author

deemp commented Oct 24, 2023

I updated the flake to bundle spago with --offline. @thomashoneyman, how to fix this problem?

nix build fails.

nix log /nix/store/bs9rr1bbc5fy4i15ds5dq9skjp1v38aw-ordered-collections.drv produces this:

Fetching dependencies...
Merge cache-db.json files...
Error 1 of 2:

  in module Data.Map.Internal
  at /nix/store/n61fnbpabgyvaz563sbhykqg4hxqra26-ordered-collections/src/Data/Map/Internal.purs:72:1 - 72:112 (>

    Module Data.Function.Uncurried was not found.
    Make sure the source file exists, and that it has been provided as an input to the compiler.


  See https://github.com/purescript/documentation/blob/master/errors/ModuleNotFound.md for more information,
  or to contribute content related to this error.

Error 2 of 2:

  in module Data.Set.NonEmpty
  at /nix/store/n61fnbpabgyvaz563sbhykqg4hxqra26-ordered-collections/src/Data/Set/NonEmpty.purs:32:1 - 32:39 (l>

    Module Data.Function.Uncurried was not found.
    Make sure the source file exists, and that it has been provided as an input to the compiler.


  See https://github.com/purescript/documentation/blob/master/errors/ModuleNotFound.md for more information,
  or to contribute content related to this error.


purs compile failed with exit code 1.
Error 1 of 2:

  in module Data.Map.Internal
  at /nix/store/n61fnbpabgyvaz563sbhykqg4hxqra26-ordered-collections/src/Data/Map/Internal.purs:72:1 - 72:112 (>

    Module Data.Function.Uncurried was not found.
    Make sure the source file exists, and that it has been provided as an input to the compiler.


  See https://github.com/purescript/documentation/blob/master/errors/ModuleNotFound.md for more information,
  or to contribute content related to this error.

Error 2 of 2:

  in module Data.Set.NonEmpty
  at /nix/store/n61fnbpabgyvaz563sbhykqg4hxqra26-ordered-collections/src/Data/Set/NonEmpty.purs:32:1 - 32:39 (l>

    Module Data.Function.Uncurried was not found.
    Make sure the source file exists, and that it has been provided as an input to the compiler.


  See https://github.com/purescript/documentation/blob/master/errors/ModuleNotFound.md for more information,
  or to contribute content related to this error.


lines 42-55/55 (END)

@deemp
Copy link
Contributor Author

deemp commented Jan 20, 2024

I've updated the PR.

Now, I get these errors:

Error found:
in module Spago.BuildInfo
at /nix/store/00hmlncdnz5pzvk6p10baaszjya4ywl4-spago/src/Spago/BuildInfo.purs:78:25 - 78:43 (line 78, column 25 - line 78, >

  Type of expression lacks required label "spago-bin".

while checking that type Record (() @Type)
  is at least as general as type { "spago-bin" :: t0
                                 | t1
                                 }
while checking that expression packages
  has type { "spago-bin" :: t0
           | t1
           }
while checking type of property accessor packages."spago-bin"
in value declaration mkBuildInfo

where t0 is an unknown type
      t1 is an unknown type

See https://github.com/purescript/documentation/blob/master/errors/PropertyIsMissing.md for more information,
or to contribute content related to this error.


...skipping...


Error found:
in module Spago.BuildInfo
at /nix/store/00hmlncdnz5pzvk6p10baaszjya4ywl4-spago/src/Spago/BuildInfo.purs:78:25 - 78:43 (line 78, column 25 - line 78, >

  Type of expression lacks required label "spago-bin".

while checking that type Record (() @Type)
  is at least as general as type { "spago-bin" :: t0
                                 | t1
                                 }
while checking that expression packages
  has type { "spago-bin" :: t0
           | t1
           }
while checking type of property accessor packages."spago-bin"
in value declaration mkBuildInfo

where t0 is an unknown type
      t1 is an unknown type

See https://github.com/purescript/documentation/blob/master/errors/PropertyIsMissing.md for more information,
or to contribute content related to this error.

@f-f
Copy link
Member

f-f commented Jan 20, 2024

@deemp the format of the BuildInfo module has changed - previously the packages were contained in a record buildInfo.packages, now they are just in packages

@deemp
Copy link
Contributor Author

deemp commented Jan 20, 2024

Seems like this is a problem with purix.buildSpagoLock. I see a comment by @thomashoneyman there.

But now there is spago.

I don't get what's the problem with using it directly.

@deemp
Copy link
Contributor Author

deemp commented Jan 21, 2024

@f-f, building spago from source works (see thomashoneyman/purescript-overlay#71). Tests fail so far.

Should I move the code for bulding spago here?

build.nix Outdated
inherit system;
overlays = [
purescript-overlay.overlays.default
(self: super: { purescript = self.purs-unstable; nodejs = super.nodejs_18; })
Copy link
Contributor

Choose a reason for hiding this comment

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

This node version is now the default & will eventually be deprecated. It’d be better to throw an error if the version is less than 18 if you wanted to check I think.

build.nix Outdated
chmod +w -R .

cp -r ${nodeDependencies}/js/node_modules .
cp -r ${spagoDependencies.spago-bin}/output .
Copy link
Contributor

Choose a reason for hiding this comment

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

You could use ln -s to not copy the files in.

flake.nix Outdated
@@ -3,6 +3,9 @@
nixpkgs.url = "github:NixOS/nixpkgs/nixos-23.05";
purescript-overlay.url = "github:thomashoneyman/purescript-overlay";
purescript-overlay.inputs.nixpkgs.follows = "nixpkgs";
flake-utils.url = "github:numtide/flake-utils";
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn’t used

@deemp
Copy link
Contributor Author

deemp commented Jan 22, 2024

@toastal, thank you for a review.

I'm going to finish thomashoneyman/purescript-overlay#71.

When it works, I'll move the spago derivation from there to here.

@deemp deemp changed the title add flake Build spago from source Jan 25, 2024
@deemp
Copy link
Contributor Author

deemp commented Jan 28, 2024

@toastal, @thomashoneyman, the spago derivation works. However, I used the not yet merged PR thomashoneyman/purescript-overlay#71. Could you please check both PRs?

In this PR (to spago repo), you can run spago and see completions:

nix develop .#dev
spago <TAB>
--help     build      graph      publish    sources    
--version  bundle     init       registry   test/      
-h         docs       install    repl       uninstall  
-v         fetch      ls         run        upgrade 

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.

4 participants