Rewrite the cabal wrapper in python #1097

thufschmitt · 2019-09-12T14:52:30Z

Solves #1096 by not needing access to the standard unix tools
(and also make it easier to maintain)

Profpatsch

This really makes the code a lot easier to read, great idea.

I compared the old and new file side-by-side and noted a few things that I couldn’t see in the new version.

For review it’s easier to have a 1:1 rewrite in the first commit and then simplifications in further commits. This was still fine, but for more complex rewrites that’s an important thing to keep in mind.

Anyway, a few things:

haskell/private/cabal_wrapper.py.tpl

haskell/cabal.bzl

haskell/private/cabal_wrapper.sh.tpl

haskell/private/cabal_wrapper.py.tpl

Profpatsch · 2019-09-16T11:36:10Z

haskell/private/cabal_wrapper.py.tpl

+ghc_pkg = os.path.join(execroot, "%{ghc_pkg}")
+
+extra_args = []
+current_arg = sys.argv.pop(1)


All the argv-modifying code should really be in one block, like in the original script.

It wasn't in the original script, which is why I left it that way

It appeared to me like it was though. Maybe I missed something.
Would make the logic a lot easier to follow.

haskell/private/cabal_wrapper.py.tpl

thufschmitt · 2019-09-16T14:16:21Z

For review it’s easier to have a 1:1 rewrite in the first commit and then simplifications in further commits. This was still fine, but for more complex rewrites that’s an important thing to keep in mind.

Well that's what I did as much as it was reasonable to do: There were quite a number of bashisms in the original script that didn't really translated to python or too painfully for what it's worth. But apart from that I kept the structure as identical as possible.

Anyways, I should have answered your other comments, PTAL

thufschmitt · 2019-09-16T15:10:04Z

The bindist failure is a bit mysterious to me. If someone has an idea of why bazel can't find python at that point I'm all ears

haskell/cabal.bzl

mboes · 2019-09-18T12:49:33Z

examples/WORKSPACE

@@ -97,6 +106,7 @@ stack_snapshot(
    snapshot = "lts-14.0",
    vendored_packages = {"split": "@split//:split"},
    deps = ["@zlib.dev//:zlib"],
+    tools = [ "@stdenv//:bin/{}".format(tool) for tool in ["sh", "grep", "sed", "awk"]],


It's going to be quite a cost for all Nixpkgs users if they have to specify this any time they use stack_snapshot. What's the reason for this and are there no alternatives?

Yes I agree, I've included this out of despair, but I'd really like another solution. The issue is that some cabal packages need these tools at build time (the ones with a configure script), but afaik there's no clean way of getting them in bazel (It really sucks that there's no "bash" toolchain 😕 ).

Outside of NixOS, these tools are incidentally put into the PATH by

rules_haskell/haskell/private/cabal_wrapper.py.tpl

Line 57 in 2c4dc52

base_path = subprocess.check_output(["/bin/sh", "-c", "echo $PATH"], env={}).strip().decode()

, but that's an awful hack that doesn't work on NixOS.

If you know any way of giving the rule access to a standard bash environment, I'm all ears

Profpatsch

Mostly good from my side'

commented

aherrmann · 2019-09-30T08:00:37Z

@regnat @Profpatsch Regarding the problem of missing shell utils in ./configure style packages:

Using /bin/sh -c "echo $PATH" is a nice trick, but it fails on NixOS, because the default PATH is empty, and fails on Windows, because /bin/sh does not exist. On Windows we might be able to work around this using @bazel_tools//tools/sh:toolchain_type to find the shell. However, on Nix that still fails due to empty PATH.

Another approach would be a shell utils toolchain. Sadly there's no standard toolchain for this. I gave it a shot to create one here. The toolchain captures standard Unix tools and creates a PATH that is passed into the Cabal wrapper template here and appended to the PATH environment variable here.

Currently, the toolchain just plucks those tools out of PATH in a repository rule, but it should be straight forward to create a rules_nixpkgs variant. Another issue with this is Cabal might discover cc and similar from this new PATH, so we'll have to explicitly tell Cabal to use the cc_toolchain cc.

Otherwise, this seems to be working. Windows passes, and the missing Unix tools issue is solved on nixpkgs. Darwin fails because we're not pointing to the cc_toolchain.

Let me know what you think of that approach.

thufschmitt · 2019-10-01T12:15:00Z

Let me know what you think of that approach.

That definitely looks like the right approach, although it's a shame that we have to implement this unix_toolchain ourselves

Another issue with this is Cabal might discover cc and similar from this new PATH, so we'll have to explicitly tell Cabal to use the cc_toolchain cc

Wasn't it already the case with use_default_shell_env ?

aherrmann · 2019-10-02T10:22:45Z

Let me know what you think of that approach.

That definitely looks like the right approach, although it's a shame that we have to implement this unix_toolchain ourselves

Indeed, maybe it could be factored out at some point and made reusable.

Another issue with this is Cabal might discover cc and similar from this new PATH, so we'll have to explicitly tell Cabal to use the cc_toolchain cc

Wasn't it already the case with use_default_shell_env ?

Good point, in principle yes. I think it didn't cause failures so far because it kept picking a compatible compiler. E.g. with use_default_shell_env under nix-shell we were getting the default PATH defined in the Nix expression, which includes a Nix provided gcc.

Profpatsch · 2019-10-07T11:01:14Z

@regnat should we add @aherrmann’s proposal to this, I guess once that is done we can merge.

aherrmann · 2019-10-07T11:07:53Z

@Profpatsch @regnat I have the nixpkgs integration of that unix toolchain almost ready. I'll ping here when it's up.

aherrmann · 2019-10-15T11:52:43Z

@regnat As discussed, I've rebased this PR on #1117 and fixed a few small issues. It should be green on CI now (see https://github.com/tweag/rules_haskell/commits/cabal_python). PTAL

thufschmitt

Awesome! I can't approve because I'm the original author, but 👍

haskell/cabal.bzl

aherrmann

Great! I can approve, but I added a bunch of changes myself. So, I think it would be good to have another pair of eyes on this. Maybe @Profpatsch

Solves #1096 by not needing access to the standard unix tools (and also make it easier to maintain)

- Rewrite the script to be compatible with python2 - Add a big hack to get back a "standard" path so that the wrapper scripts for ghc still work

network has a configure phase and expects some base unix tools to be available

Drop EXTRA_PATH hack

thufschmitt force-pushed the cabal_wrapper_no_default_env branch from 03bf097 to 4422da6 Compare September 15, 2019 17:24

Profpatsch previously requested changes Sep 16, 2019

View reviewed changes

thufschmitt mentioned this pull request Sep 16, 2019

haskell_cabal_library: Add haddocks #1102

Merged

aherrmann reviewed Sep 17, 2019

View reviewed changes

haskell/cabal.bzl Show resolved Hide resolved

thufschmitt force-pushed the cabal_wrapper_no_default_env branch from af30215 to de05cd1 Compare September 18, 2019 08:55

mboes reviewed Sep 18, 2019

View reviewed changes

Profpatsch reviewed Sep 20, 2019

View reviewed changes

mboes mentioned this pull request Sep 23, 2019

Only build required cabal component #1095

Merged

aherrmann mentioned this pull request Oct 7, 2019

Define Unix shell toolchain #1117

Merged

aherrmann force-pushed the cabal_wrapper_no_default_env branch from 2c4dc52 to db43c6f Compare October 15, 2019 11:51

thufschmitt commented Oct 16, 2019

View reviewed changes

haskell/cabal.bzl Show resolved Hide resolved

aherrmann approved these changes Oct 16, 2019

View reviewed changes

aherrmann mentioned this pull request Oct 23, 2019

Windows support for Cabal python wrapper #1133

Merged

mboes added the merge-queue label Oct 23, 2019

Théophane Hufschmitt and others added 7 commits October 23, 2019 12:56

Adress @Profpatsch's review

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

862ac3b

Make cabal_wrapper compatible with non-nixos linuxes

289b31f

- Rewrite the script to be compatible with python2 - Add a big hack to get back a "standard" path so that the wrapper scripts for ghc still work

fixup! Make cabal_wrapper compatible with non-nixos linuxes

35e4deb

fixup! Rewrite the cabal wrapper in python

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

72e1707

examples: Add some required tools to stack_packages

cc6fb33

network has a configure phase and expects some base unix tools to be available

Cabal wrapper: use unix toolchain

e728b6f

Drop EXTRA_PATH hack

aherrmann added 7 commits October 23, 2019 12:56

cabal wrapper: use cc-wrapper

ecb948a

cabal wrapper: fix ar on macOS

8b9badb

examples: drop nix stdenv

5e805c9

remove dead code

668dee2

cabal wrapper: document component argument

81c1fca

cabal wrapper: missing comment about chdir

eb578cd

cabal wrapper: missing datasubdir flag

35abfbf

jwiegley force-pushed the cabal_wrapper_no_default_env branch from db43c6f to 35abfbf Compare October 23, 2019 12:56

mergify bot merged commit e6b2e2b into master Oct 23, 2019

mergify bot deleted the cabal_wrapper_no_default_env branch October 23, 2019 13:38

mergify bot removed the merge-queue label Oct 23, 2019

jkachmar mentioned this pull request Oct 29, 2019

stack_snapshot get invalidated too often #1096

Closed

aherrmann mentioned this pull request Mar 13, 2020

release 0.12: add changelog #1279

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite the cabal wrapper in python #1097

Rewrite the cabal wrapper in python #1097

thufschmitt commented Sep 12, 2019

Profpatsch left a comment

Profpatsch Sep 16, 2019

thufschmitt Sep 16, 2019

Profpatsch Sep 20, 2019

thufschmitt commented Sep 16, 2019

thufschmitt commented Sep 16, 2019

mboes Sep 18, 2019

thufschmitt Sep 18, 2019

Profpatsch left a comment

aherrmann commented Sep 30, 2019

thufschmitt commented Oct 1, 2019

aherrmann commented Oct 2, 2019

Profpatsch commented Oct 7, 2019

aherrmann commented Oct 7, 2019

aherrmann commented Oct 15, 2019

thufschmitt left a comment

aherrmann left a comment

Rewrite the cabal wrapper in python #1097

Rewrite the cabal wrapper in python #1097

Conversation

thufschmitt commented Sep 12, 2019

Profpatsch left a comment

Choose a reason for hiding this comment

Profpatsch Sep 16, 2019

Choose a reason for hiding this comment

thufschmitt Sep 16, 2019

Choose a reason for hiding this comment

Profpatsch Sep 20, 2019

Choose a reason for hiding this comment

thufschmitt commented Sep 16, 2019

thufschmitt commented Sep 16, 2019

mboes Sep 18, 2019

Choose a reason for hiding this comment

thufschmitt Sep 18, 2019

Choose a reason for hiding this comment

Profpatsch left a comment

Choose a reason for hiding this comment

aherrmann commented Sep 30, 2019

thufschmitt commented Oct 1, 2019

aherrmann commented Oct 2, 2019

Profpatsch commented Oct 7, 2019

aherrmann commented Oct 7, 2019

aherrmann commented Oct 15, 2019

thufschmitt left a comment

Choose a reason for hiding this comment

aherrmann left a comment

Choose a reason for hiding this comment