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

Dev agda shellfor #98087

Open
wants to merge 3 commits into
base: master
from
Open

Dev agda shellfor #98087

wants to merge 3 commits into from

Conversation

@turion
Copy link
Contributor

turion commented Sep 16, 2020

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

CC @alexarice

#98084 should be merged first

@turion
Copy link
Contributor Author

turion commented Sep 16, 2020

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Sep 16, 2020

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@alexarice
Copy link
Contributor

alexarice commented Sep 16, 2020

What does this do that

mkShell {
  buildInputs = [
    (agda.withPackages (p: [ ... ])
  ];
}

doesn't?

@turion turion force-pushed the turion:dev_agda_shellfor branch from 5396194 to 1d6ff1e Sep 16, 2020
@turion turion force-pushed the turion:dev_agda_shellfor branch from 1d6ff1e to 5484e7e Sep 16, 2020
@turion
Copy link
Contributor Author

turion commented Sep 16, 2020

It doesn't build the package itself, but only its dependencies. See updated docs.

@turion turion force-pushed the turion:dev_agda_shellfor branch from 5484e7e to 7fb5665 Sep 16, 2020
@alexarice
Copy link
Contributor

alexarice commented Sep 16, 2020

Ah right I understand. Perhaps we should follow what haskell does and have this under packageName.passthru.env

@turion
Copy link
Contributor Author

turion commented Sep 16, 2020

Aha. I hadn't even heard of passthru.env until now. It seems undocumented. There is a shellFor as well for Haskell, though. Not sure which one is more general, but I believe shellFor. It at least has a test, it seems.

In Haskell's shellFor it's possible to add a list of packages, in fact, and also further dev tools like editors, linters etc., maybe we want that here as well?

We could actually have both, a shellFor and an env which calls shellFor. What do you think?

@alexarice
Copy link
Contributor

alexarice commented Sep 18, 2020

I think having the env attribute is nice, as then you can just put library.env in your shell

@turion turion force-pushed the turion:dev_agda_shellfor branch from 7fb5665 to f7f774d Sep 18, 2020
@turion
Copy link
Contributor Author

turion commented Sep 18, 2020

@alexarice Like this?

@turion
Copy link
Contributor Author

turion commented Sep 18, 2020

This doesn't work, it seems.

@alexarice
Copy link
Contributor

alexarice commented Sep 18, 2020

Does that actually work, I'd have thought you need some sort of mkShell command (as you did for the original shellFor). I thought the plan was to keep both

@turion
Copy link
Contributor Author

turion commented Sep 18, 2020

I thought the plan was to keep both

Ah ok, I misunderstood you then 😅 Ok I'll bring back both.

@alexarice
Copy link
Contributor

alexarice commented Sep 18, 2020

I was thinking of the sort of thing in pkgs/development/haskell-modules/generic-builder.nix

@alexarice
Copy link
Contributor

alexarice commented Sep 18, 2020

@turion
Copy link
Contributor Author

turion commented Sep 18, 2020

Ok, I should be able to make something from that, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.