Add stack_install #887

mboes · 2019-05-22T22:29:55Z

With haskell_cabal_library (added in #882), we have the ability to make
one-shot calls to Cabal, assuming,

that the sdist for the Cabal package has been fetched and unpacked, and
that dependencies have been declared correctly.

Declaring these dependencies by hand for all packages on Hackage is a
fool's errand. This commit uses Stack to,

resolve package names to package versions using the given snapshot,
fetch the sdist and unpack it,
find out the dependency graph and generate a BUILD file encoding it.

Stack only outputs dependency information in GraphViz format. So have to
parse that.

This assumes Stack is in the PATH (hence build is non-hermetic). But it's
pretty unreasonable to build our own Stack inside a workspace rule (Bazel's
build engine doesn't work there). The best we can do is download prebuilt
binaries, which we can do in the future (after figuring out how to
seamlessly work on NixOS).

BTW @snoyberg: I noticed the upcoming --global-hints feature of
Stack is marked as "experimental" and "could be removed". Just FTR
that will be a very useful feature for us (we'll be able to generate
a dependency graph without first downloading a GHC toolchain).

@regnat this PR removes the "new-style" haskell_import rule. We now
no longer need that rule in the tests and I believe no one else is
using it either.

Closes #874.

With `haskell_cabal_library` (added in #882), we have the ability to make one-shot calls to Cabal, assuming, 1. that the sdist for the Cabal package has been fetched and unpacked, and 2. that dependencies have been declared correctly. Declaring these dependencies by hand for all packages on Hackage is a fool's errand. This commit uses Stack to, 1. resolve package names to package versions using the given snapshot, 2. fetch the sdist and unpack it, 3. find out the dependency graph and generate a `BUILD` file encoding it. Stack only outputs dependency information in GraphViz format. So have to parse that. This assumes Stack is in the `PATH` (hence build is non-hermetic). But it's pretty unreasonable to build our own Stack inside a workspace rule (Bazel's build engine doesn't work there). The best we can do is download prebuilt binaries, which we can do in the future (after figuring out how to seamlessly work on NixOS). Closes #874.

This commit removes all use of the "new" `haskell_import` based mechanism and the Nixpkgs code to support it. This is effectively a revert of #442. The new stack_install mechanism is not Nixpkgs specific and has better performance characteristics.

mboes · 2019-05-23T07:09:46Z

We can remove dead code separately (and later if downstream depends on this experimental feature). @regnat I removed 2984010 from this PR and will push it in a followup PR.

thufschmitt · 2019-05-23T11:57:06Z

haskell/cabal.bzl

+        _execute_or_fail_loudly(repository_ctx, stack + ["unpack"] + unversioned_packages)
+    exec_result = _execute_or_fail_loudly(repository_ctx, ["ls"])
+    unpacked_sdists = exec_result.stdout.splitlines()
+    stack_yaml_content = "\n".join(["resolver: none", "packages:"]) + "\n" + "\n".join([


Wouldn't it be cleaner and easier to read to use to_json rather than writing the yaml file by hand?

Great idea. Will try that.

It seems that even at the 5th point release, GHC still has packaging bugs on Windows.

thufschmitt

Looks great!

I have absolutely zero objection to the removal of haskell_import_new, I don't think we could reasonably make it work efficiently enough (and I don't think there's any user of it left now that I've switched back to a plain ghcWithPackages).

I just have two (minor) concerns:

With this approach, we still have to explicitely list all our dependencies in the WORKSPACE file (though I assume any attempt at lifting this would result in a need of a "load a generated file" thing in the workspace which is always painful)
The external dependencies are shared amongst all the packages, which means that adding a new one will require rebuilding the whole dependency set

thufschmitt · 2019-05-24T06:57:47Z

WORKSPACE


-haskell_nixpkgs_package(
+nixpkgs_package(
    name = "ghc",


The ghc.nix file pinned the ghc version regardless of the nixpkgs version used, which we don't do anymore by using whichever ghc nixpkgs provides us. Is that intended?

This is hard to achive in practice: Nixpkgs is pretty aggressive about prunning old minor releases when a new one comes out. Either way we're still just as reproducible.

thufschmitt · 2019-05-24T07:03:38Z

tests/haddock/LibB.hs

 import TH (foo)

 -- | Doc for 'x' using 'f' and 'a' and 'Int'.
 x :: Int
 x = const f a

-- | This uses a type from an undocumented package


Don't we remove a potentially useful test here?

I assume that's because we can't easily extend the stackage snapshot whith some lib which isn't on hackage but maybe we could use an existing library with no documentation instead

We are. But the value of the test needs to be measured against the cost of migrating it. It's quite difficult.

maybe we could use an existing library with no documentation instead

Good idea. If you can find one then we could revive this test.

aherrmann

Looks great!

I have added a number of questions. But they're mostly about future direction of this feature, and out of scope for this PR.

WORKSPACE

aherrmann · 2019-05-24T08:23:54Z

WORKSPACE

 )
 """,
    repository = "@nixpkgs",
 )

+nixpkgs_package(
+    name = "c2hs",
+    attribute_path = "haskellPackages.c2hs",


Why pull these from nixpkgs rather than stackage?

Because we don't yet have haskell_cabal_binary.

aherrmann · 2019-05-24T08:27:06Z

haskell/cabal.bzl

+    "time",
+    "transformers",
+    "unix",
+    "xhtml",


Interestingly hazel/packages.bzl does not list xhtml as a core-package. Did this change between stackage releases?

xhtml has been a core library for a long time. Probably an omission in Hazel. Or maybe an omission in Stackage.

aherrmann · 2019-05-24T08:33:11Z

haskell/cabal.bzl

+    stack = [stack_cmd, "--no-nix", "--skip-ghc-check", "--system-ghc"]
+    if versioned_packages:
+        _execute_or_fail_loudly(repository_ctx, stack + ["unpack"] + versioned_packages)
+    stack = [stack_cmd, "--no-nix", "--skip-ghc-check", "--system-ghc", "--resolver", repository_ctx.attr.snapshot]


As this is a repository rule this just picks whatever ghc is in $PATH, correct? Does this have any impact on the generated dependency graph? E.g. conditional dependencies based on the ghc version, or similar? If so then this seems like another iteration of FormationAI/hazel#80.

The nice thing about using Stackage is that in principle they deal with this for us, provided we stick to using the same GHC version as Stackage does for any given snapshot.

aherrmann · 2019-05-24T08:35:50Z

haskell/cabal.bzl

+        tokens = [w.strip('";') for w in line.split(" ")]
+
+        # All lines of the form `"foo" -> "bar";` declare edges of the
+        # dependency graph in the Graphviz format.


It would be great if stack could spit this out in a more robust and easier parseable format.

Agreed. I'll file a ticket upstream.

aherrmann · 2019-05-24T08:43:47Z

haskell/cabal.bzl

+
+def _compute_dependency_graph(repository_ctx, versioned_packages, unversioned_packages):
+    """Given a list of root packages, compute a dependency graph.
+


As I understand it this does not just return the dependency graph of the transitive closure of the requested packages, but of all of Stackage, correct? That's probably fine, but not very clear from the docstring.

No not all of Stackage. It computes only the transitive closure.

aherrmann · 2019-05-24T08:46:11Z

shell.nix

@@ -16,6 +16,9 @@ mkShell {
    perl
    python3
    bazel
+    # For stack_install. TODO Remove ghc when move to Stack 2.


What does Stack 2 improve in this respect?

It generates the dependency graph without the spurious requirement that GHC be in the environment.

aherrmann · 2019-05-24T08:55:29Z

haskell/cabal.bzl

+    build_file_content = "\n".join(build_file_builder)
+    repository_ctx.file("BUILD.bazel", build_file_content, executable = False)
+
+stack_install = repository_rule(


Comparing to Hazel, here are a few things that come to mind which stack_install and haskell_cabal_library don't seem to offer, yet. Those might be good future extensions.

Override individual packages with custom Bazel builds. Probably easiest by injecting a mapping from package name to Bazel targets.

Stack's extra-deps feature to override a package version or just it's source repository. Also to add additional Hackage packages, or packages from other sources.

Apply source patches. This is very useful if a package just requires minor adjustment to build within Bazel. E.g. file paths to embedded data files. In such cases a full fork imposes a lot of overhead.

Overriding cabal flags. E.g. integer-simpl vs. integer-gmp.

Binary targets. As I understand haskell_cabal_library does currently not expose any binary targets. I suspect this has two consequences. A) Users cannot use e.g. a Stackage provided c2hs. B) Stackage packages that depend on other Stackage packages' binaries, e.g. happy, will probably not work.

Override individual packages with custom Bazel builds.

Not currently possible, but it's possible to override the version at least.

Stack's extra-deps feature to override a package version or just it's source repository.

See above. Not possible to override with one's own source repository. Good future feature.

Also to add additional Hackage packages, or packages from other sources.

Outside of the cases listed above, these wouldn't be part of the transitive closure, right? So don't need to be part of the stackage external repository?

Apply source patches ... Overriding cabal flags ... Binary targets

Agreed. Let's create tickets about those.

Stack's extra-deps feature [...]

I haven't tested any of these new features yet, however in practice one can make a custom snapshot file, host it remotely, and point stack to it (e.g. stack build --resolver=<link_to_snapshot.>).

Depending on how rules_haskell and stack interact here, this could be a neat workaround for extra-deps at the moment.

Hm, so you're saying #910 could be unnecessary? It hasn't been merged yet. Not too late to block it. :)

Unfortunately I can't say for sure, since I've been unable to get the quickstart instructions for rules_haskell working on macOS; I'm basing this purely off of my knowledge of how stack treats resolvers.

I do think that having something like #910 would probably be useful, regardless of this, though. I just wanted to suggest it just in case it actually did work, and could help unblock someone in the meantime 😄

@jkachmar You can use rules_haskell master for now, on MacOS there was a minor bugfix, see #893

I’m gonna push a point-release later today.

There's a singular point missing in our CI matrix currently, which I guess is the point you're in: macOS + ghc bindists (as opposed to GHC-from-Nixpkgs).

https://github.com/tweag/rules_haskell/releases/tag/v0.9.1

mboes · 2019-05-24T09:30:15Z

@regnat

With this approach, we still have to explicitely list all our dependencies in the WORKSPACE file (though I assume any attempt at lifting this would result in a need of a "load a generated file" thing in the workspace which is always painful)

Yes, either what you mention (à la Hazel), or we could just download all packages in the snapshot. In fact I'm pondering making it so that if no packages are specified, or if using some boolean flag, then all of Stackage is downloaded, and all of it becomes available. For a small project that's a comparatively big download. But for e.g. DAML, which is a large repo with lots of dependencies anyways, downloading all of Stackage isn't all that crazy.

The external dependencies are shared amongst all the packages, which means that adding a new one will require rebuilding the whole dependency set

That's true. See discussion with @aherrmann above. This seems inevitable to me given a design flaw in Cabal. But we could consider trying to do better. I'm just not sure it'll pan out all that well.

mboes requested review from thufschmitt and aherrmann May 22, 2019 22:30

mboes force-pushed the stack_install branch from 6160824 to 9440659 Compare May 22, 2019 22:36

mboes force-pushed the stack_install branch from 9440659 to 15598ef Compare May 22, 2019 22:52

mboes force-pushed the stack_install branch 2 times, most recently from 2984010 to 7912560 Compare May 23, 2019 07:06

mboes mentioned this pull request May 23, 2019

haskell_register_ghc_nixpkgs fails when package db contains multiples version of the same package #885

Closed

thufschmitt reviewed May 23, 2019

View reviewed changes

mboes added 2 commits May 23, 2019 19:27

cabal.bzl: Use to_json() to serialize package list.

3305999

Apply 8.6.4 patch to 8.6.5 GHC Windows bindist as well.

f04cd5f

It seems that even at the 5th point release, GHC still has packaging bugs on Windows.

thufschmitt approved these changes May 24, 2019

View reviewed changes

aherrmann approved these changes May 24, 2019

View reviewed changes

mboes merged commit 647ddc8 into master May 24, 2019

mboes deleted the stack_install branch May 24, 2019 09:31

This was referenced May 27, 2019

stack_install: let user apply source patches #902

Closed

stack_install: Override Cabal flags #903

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add stack_install #887

Add stack_install #887

mboes commented May 22, 2019 •

edited

Loading

mboes commented May 23, 2019

thufschmitt May 23, 2019 •

edited

Loading

mboes May 23, 2019

thufschmitt left a comment

thufschmitt May 24, 2019

mboes May 24, 2019

thufschmitt May 24, 2019

mboes May 24, 2019

aherrmann left a comment

aherrmann May 24, 2019

mboes May 24, 2019

aherrmann May 24, 2019

mboes May 24, 2019

aherrmann May 24, 2019

mboes May 24, 2019

aherrmann May 24, 2019

mboes May 24, 2019

aherrmann May 24, 2019

mboes May 24, 2019

aherrmann May 24, 2019

mboes May 24, 2019

aherrmann May 24, 2019

mboes May 24, 2019

symbiont-joseph-kachmar May 31, 2019

mboes May 31, 2019

jkachmar Jun 1, 2019

Profpatsch Jun 3, 2019

mboes Jun 3, 2019

Profpatsch Jun 3, 2019

mboes commented May 24, 2019


		def _compute_dependency_graph(repository_ctx, versioned_packages, unversioned_packages):
		"""Given a list of root packages, compute a dependency graph.

Add stack_install #887

Add stack_install #887

Conversation

mboes commented May 22, 2019 • edited Loading

mboes commented May 23, 2019

thufschmitt May 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thufschmitt left a 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

aherrmann left a 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

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

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

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

mboes commented May 24, 2019

mboes commented May 22, 2019 •

edited

Loading

thufschmitt May 23, 2019 •

edited

Loading