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

Dependency executables aren't in PATH #3417

Closed
phadej opened this issue May 10, 2016 · 10 comments
Closed

Dependency executables aren't in PATH #3417

phadej opened this issue May 10, 2016 · 10 comments

Comments

@phadej
Copy link
Collaborator

phadej commented May 10, 2016

Summary @ezyang. There's an easy fix for this which should only take an afternoon to implement: for each direct dependency, if it contains any executables, add the PATH of the executables to the environment when we are building. There are two primary risks: (1) how do you actually add the PATH to the environment, and (2) if there is a PATH limitation that we butt into.


While trying to use cabal new-build on servant:

Preprocessing test suite 'spec' for servant-0.7...
ghc: could not execute: hspec-discover

The executable is clearly not in $PATH :(

@dcoutts
Copy link
Contributor

dcoutts commented May 11, 2016

Yes, proper handling of executables and the path, both for build tools and also for the things you're building is a significant missing feature.

The plan is roughly: have a ./dist/bin dir where all the built exes from this project go. For binaries of dependencies there's a couple things we can do. One is just extend the $PATH for only when we're building things, but the other is to have an env that you can use from your shell and be able to run these tools too. So that way we can support a cabal shell cabal env or similar that lets you enter the shell environment and get the $PATH with all the visible things.

If anyone wants to help with this it'd be much appreciated. There's plenty to do.

@ezyang
Copy link
Contributor

ezyang commented Jul 21, 2016

A number of comments:

  1. There is a long standing bug cabal-install doesn't consider build-tools as dependencies #220 whereby build-tools are not considered dependencies. This means that if a dependency is only specified via build-tools, a new-build plan will never actually build the executable in question. This has been worked around in the ecosystem by making sure preprocessors like hspec-discover are indirectly build-depends of the packages in question. So you don't have to address this, but it's something to keep in mind.
  2. Do direct dependencies, or transitive dependencies get their executables added to the path? In a vacuum it would certainly make sense for only direct dependencies to be added to the path. But in fact this is NOT backwards compatible behavior, as evidenced by servant's Cabal file https://hackage.haskell.org/package/servant-0.8/servant.cabal which brings hspec-discover into scope transitively by hspec. The reason this used to work is that anything you installed would get chucked in .cabal/bin, so you'd always see it when you got around to your package. But it's easy enough for servant to fix their Cabal file, so I am not sure this is sufficient evidence that we should go the transitive route. Big problems computing what executables to bring into scope.
  3. The obvious thing is to extend the PATH for each dependency we want to use. However, we have to be careful: there are limits to how large the PATH variable can be, esp. on Windows. https://superuser.com/questions/635082/too-many-folders-in-the-path-variable suggests it's anywhere from 2048 to four times this quantity. If we don't go the transitive dependencies route, this might be enough; but someone please stress test this case; store names are not short and you may not need very many executables to go over the limit.
  4. If we go the copying route, it would make sense for new-build to create the little executable directory at the beginning of the build. The most appropriate place to do this is cabal-install/Distribution/Client/ProjectBuilding.hs in buildAndInstallUnpackedPackage. The set of executables to be put into the path can be determined from pkgDependencies on the ElaboratedConfiguredPackage inside the ReadyPackage—the ComponentId should be sufficient to extract the path where the executable lives, although I am unsure what the correct function to compute this is (when we build the executable, the install dir is specified by pkgInstallDirs). I have no idea how to implement transitive behavior here.

So, it's a bit fiddly, needs to be tested properly, but shouldn't be very much code. CC @acfoltzer

@hvr
Copy link
Member

hvr commented Jul 22, 2016

I consider the current practice of expecting executables to be built for dependencies mentioned in build-depends to be a bug. I'm strongly for fixing this, since new-build is going to be a big departure anyway, so IMO we should use this as an opportunity to finally fix things, rather than trying to hard to cater to stay bug-compatible. (c.f. #3661)

So I strongly support to have executables only being (possibly) built and brought into PATH via build-tools, and like with build-depends, only directly specified dependencies shall be made visible.
Specifically I want to support having having packages with distinct version requirements of happy or alex within the same install-plan, rather than forcing the install-plan to globally use the same version. This is something that became possible with nix-local-build.

Allowing transitive leakage would break the general design principle followed in .cabal semantics and result in non-modular effects.

@ezyang
Copy link
Contributor

ezyang commented Jul 22, 2016

@hvr I have considered this in #2965. I think it is the right way to go about doing it, but one trouble is that there is no necessary correspondence between a build-tool and a package name. They happily do coincide in many cases but here are some where they don't (all taken from Hackage):

  • gtk2hsc2hs, gtk2hsHookGenerator, gtk2hsTypeGen ---> gtk2hs-buildtools
  • ghc, llvm-config, nix-instantiate, nix-build, nix-env, nix-store (no package)

Duncan has suggested that we hard-code a mapping into Cabal from build-tools to package names for legacy purposes. I have suggested that we introduce a new field tool-depends which specifies packages which provide build-tools, but I suppose we could also just change the semantics of build-tools to now apply to these executables.

And yes, it would be a simple matter to solve these separately ala setup dependencies.

@hvr
Copy link
Member

hvr commented Jul 25, 2016

@ezyang I like the idea to hard-code (or rather make the legacy mapping a config setting with a default-value in ~/.caba/config, so users can tweak it), to help save ~99% of legacy .cabal files out there (which is almost exclusively about alex/happy/cpphs/gtk2hs/hsc2hs/c2hs iirc from the last time I scanned 00-index.tar for build-tools -- I can make proper stats if needed)

Maybe we should try to come up with one or two concrete specifications, write them down, and decide which one qualifies as the best way forward. I believe we should tackle this rather sooner than later, and I'd like to help with implementing this one once we have decided on a plan.

Also, some packages are already adapting to build-depends not declaring executable dependencies anymore, c.f. gtk2hs/gtk2hs#171 (comment)

@ezyang
Copy link
Contributor

ezyang commented Jul 25, 2016

OK I wrote in a proposal to the top of #220

@ezyang
Copy link
Contributor

ezyang commented Aug 4, 2016

Note: modifying PATH is a bit tricky. Basically, you should do the same thing as how we handle current working directory changes: if it's single-threaded, just globally (and unsafely) change the global environment. If it's multithreaded, we'll invoke a helper process, so you can set the environment at that point.

@ezyang
Copy link
Contributor

ezyang commented Aug 24, 2016

This is fixed in HEAD.

@ezyang ezyang closed this as completed Aug 24, 2016
@dcoutts
Copy link
Contributor

dcoutts commented Sep 2, 2016

Note: modifying PATH is a bit tricky.

I should note that the ProgramDb and that infrastructure has quite good support for altering the $PATH (or other env vars) of programs that will be invoked. We should use this (or extend as needed) and try to avoid mutating process state like our own env or cwd.

@ezyang
Copy link
Contributor

ezyang commented Sep 2, 2016

To fix #1541, we have to set PATH. Additionally, internal build-tools that are built separately via per-component build won't have entries in the ProgramDb, which can only be setup by Custom setup scripts. So in that case, we must use PATH, and then there's not much reason not to use PATH for everything else.

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

No branches or pull requests

5 participants