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

Update buildSpagoLock and refactor #71

Merged
merged 21 commits into from
Feb 9, 2024
Merged

Conversation

deemp
Copy link
Contributor

@deemp deemp commented Jan 21, 2024

Related to purescript/spago#981.

  • 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
Copy link
Contributor Author

deemp commented Jan 22, 2024

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

flake.nix Outdated
};

outputs = {
outputs = inputs@{
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@deemp deemp Jan 25, 2024

Choose a reason for hiding this comment

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

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

flake.nix Outdated
overlays = [
self.overlays.default
slimlock.overlays.default
(final: prev: rec {
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to avoid using rec

Copy link
Contributor Author

@deemp deemp Jan 25, 2024

Choose a reason for hiding this comment

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

Sure. 6d5c973 doesn't introduce rec anymore.

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

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

Choose a reason for hiding this comment

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

Should this be ln -s too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Fixed in 1638337

${mkBuildCommands}

mkdir -p $out
cp -r .spago $out
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

spago-dev.nix Outdated
spago
python3
nodejs
git
Copy link
Contributor

Choose a reason for hiding this comment

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

Git is needed!?

Copy link
Contributor Author

@deemp deemp Jan 24, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Owner

Choose a reason for hiding this comment

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

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
Copy link
Contributor

toastal commented Jan 23, 2024

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
Copy link
Contributor Author

deemp commented Jan 24, 2024

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
Copy link
Contributor

toastal commented Jan 25, 2024

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

@deemp deemp changed the title Allow building packages via spago Use spago in package derivations Jan 25, 2024
@thomashoneyman
Copy link
Owner

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
Copy link
Contributor Author

deemp commented Jan 25, 2024

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,

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
Copy link
Contributor Author

deemp commented Jan 28, 2024

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.

@deemp deemp force-pushed the main branch 2 times, most recently from 1e9e362 to 7f2a045 Compare January 28, 2024 14:29
@deemp deemp changed the title Use spago in package derivations Update buildSpagoLock and refactor Jan 28, 2024
@deemp deemp force-pushed the main branch 3 times, most recently from e6de7e3 to 6def68f Compare January 28, 2024 22:07
flake.nix Outdated
overlays = [
self.overlays.default
slimlock.overlays.default
(_: prev: { nodejs = prev.nodejs_20; })
Copy link
Contributor

@toastal toastal Jan 31, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 2b33591

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn’t these be like Console.error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IDK. @thomashoneyman?

BTW, there are quite many

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

It may be better to use a dedicated function.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied the suggestion in 62ce26e

@thomashoneyman
Copy link
Owner

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

@bakhtiyarneyman
Copy link
Contributor

bakhtiyarneyman commented Feb 1, 2024

@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
Copy link
Contributor

toastal commented Feb 1, 2024 via email

@deemp
Copy link
Contributor Author

deemp commented Feb 1, 2024

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
Copy link
Contributor Author

deemp commented Feb 1, 2024

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

Neat trick!

@thomashoneyman
Copy link
Owner

thomashoneyman commented Feb 1, 2024

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
Copy link
Contributor Author

deemp commented Feb 1, 2024

The default formatter for now is alejandra.

One can run nix fmt to format all files.

@toastal
Copy link
Contributor

toastal commented Feb 2, 2024

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
Copy link
Contributor Author

deemp commented Feb 3, 2024

@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
Copy link
Contributor

toastal commented Feb 3, 2024

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
Copy link
Contributor Author

deemp commented Feb 3, 2024

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/spago.yaml Outdated Show resolved Hide resolved
@@ -33,5 +33,5 @@ stdenv.mkDerivation {
ln -s $BIN $out/bin/${name}
'';

meta = meta lib;
meta = meta lib // {mainProgram = name;};
Copy link
Owner

@thomashoneyman thomashoneyman Feb 8, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Owner

@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.

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

@thomashoneyman thomashoneyman merged commit 4e151be into thomashoneyman:main Feb 9, 2024
295 checks passed
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.

Set meta.mainProgram, use lib.getExe
4 participants