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

Refactor and Readme #15

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

Conversation

m-bock
Copy link

@m-bock m-bock commented Sep 1, 2019

This PR enhances the README example to use the newly created mkBuildProjectOutput function.
It contains also some refactoring on the other build functions in the generated file.

Michael Bock added 2 commits September 1, 2019 07:36
- Easier to read, no ">>> echo"
- Less boilerpate (chmod, shebang)
- More like convention (binaries are inside /bin) of an expression
${spagoPkgs.buildSpagoStyle} # == spago2nix build
installSpagoStyle # == spago2nix install

${spagoPackages.mkBuildProjectOutput { inherit src purescript; }}
Copy link
Author

Choose a reason for hiding this comment

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

see below why I think purescript and not purs. Somehow it would be nice to enhance the example how to obtain this package. As easy-purescript is not yet in NIXOS stable, maybe a this should be somehow noted.

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, really anyone who uses this tool should also be consuming easy-purescript-nix https://github.com/justinwoo/easy-purescript-nix/

Copy link
Collaborator

Choose a reason for hiding this comment

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

This didn't work for me, build fails with /nix/store/whpnlfj79vd8p175rp1bwg0x7wz5lmj1-stdenv-linux/setup: line 81: /nix/store/q3m23c4r1zyvd40d5x4p2gfnmm04mdfx-build-project-output: Is a directory

>>$out echo "echo done."
chmod +x $out
buildSpagoStyle = pkgs.writeShellScriptBin "build-spago-style" ''
EXTRA_FILES=$@
Copy link
Author

Choose a reason for hiding this comment

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

I find it a good practice to declare explicitly what you do with the arguments in a bash script.

'';

mkBuildProjectOutput =
{ src, purs }:
{ src, purescript }:
Copy link
Author

Choose a reason for hiding this comment

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

Isn't it better to depend on purescript, as that's the package that contains the purs binary?

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's just a name change

Copy link
Owner

Choose a reason for hiding this comment

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

EXTRA_FILES=$@

echo "echo building project..."
echo "purs compile ${builtins.toString (
Copy link
Author

Choose a reason for hiding this comment

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

Actually this should also depend on purs/purescript. Currently it circumvents nix. But I did not change it as maybe the whole function is not needed anymore now that we have mkBuildProjectOutput.
Otherwise it could be ${purescript}/bin/purs.

Copy link
Owner

Choose a reason for hiding this comment

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

i actually do use this script quite often, since mkBuildProjectOutput is the opposite of what i want to use when developing. but yes, changing it to using purescript from easy-purescript-nix is probably fine

@@ -55,10 +55,15 @@ let
in
pkgs.stdenv.mkDerivation rec {
# < ... >
src = ./.;
Copy link
Owner

Choose a reason for hiding this comment

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

this should be src = ./src;, because the idea is that you directly consume sources without any directory structure. in addition, because requiring root sources is just not what i think anyone should do without a filter

@tbenst
Copy link
Collaborator

tbenst commented Dec 18, 2019

Thanks for this great package!

The current mock derivation on README doesn't work for me as it fails to build .purs from ./src. Copying script from mkBuildProjectOutput works however.

In case you're interested, here's an example for using spago2nix with mkYarnPackage: https://github.com/tbenst/purescript-nix-example

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.

3 participants