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

Use a hard-coded tool dependency map (fixes #4125) #4132

Merged
merged 4 commits into from
Jul 9, 2018

Conversation

snoyberg
Copy link
Contributor

@snoyberg snoyberg commented Jul 4, 2018

This is a fundamental change to how Stack discovers build tools, its inclusion should be discussed before we merge. We should also wait to see if it actually fixes #4125.

Note: Documentation fixes for https://docs.haskellstack.org/en/stable/ should target the "stable" branch, not master.

Please include the following checklist in your PR:

  • Any changes that could be relevant to users have been recorded in the ChangeLog.md
  • The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

@snoyberg snoyberg force-pushed the 4125-cabal-style-build-tools branch 2 times, most recently from 0bf5e1d to 512f76f Compare July 4, 2018 17:43
@snoyberg
Copy link
Contributor Author

snoyberg commented Jul 5, 2018

Some design decision time. Stack originally (and through the 1.7 release) calculated build tools by looking at all of the packages in a snapshot, finding the executables they provide, and performing a reverse lookup for any build-tools values. This works, but:

Starting with Cabal 2.0 (or 2.2), there's a new feature in the cabal file format called build-tool-depends, which allows you to specify which package a tool comes from, bypassing the need for the reverse lookup. cabal-install handles the legacy build-tools field with a hard-coded list of packages like alex and happy. This PR does the same for Stack, though with a slightly difference list.

This may cause some existing builds to break, but that's highly doubtful, and can be worked around easily enough by switching to build-tool-depends and/or explicitly building the needed dependencies. I lean in favor of merging this because matching cabal behavior is a Good Thing, pushing towards the new build-tool-depends and more explicit deps is a Good Thing, and reducing complexity in our code base is a Good Thing 😄 .

I'd say let's give it another few days for someone to object, and to confirm it fixes #4125, and then we can merge.

@snoyberg snoyberg force-pushed the 4125-cabal-style-build-tools branch from 512f76f to ba9179e Compare July 5, 2018 04:57
@Ericson2314
Copy link

N.B. I made haskell/cabal#5412 based on the issues I had making build-tool-depends (and build-tools) the sole way to get access to binaries with Nixpkgs alone. I'm curious what Stack's experience with packages that, e.g. do build-depends: hspec-discovery and expect to get an executable in addition to a library.

@snoyberg
Copy link
Contributor Author

snoyberg commented Jul 5, 2018

I'm not quite sure of the question, but I'll give an answer related to other bugs I've seen. I think it's a mistake for Cabal to hide the hspec-discovery executable when build-depends: hspec-discovery is specified. It breaks compatibility with how cabal worked in the past, and AFAICT doesn't add any real determinism to the process, since we've already specified that hspec-discovery must be installed before the package in question. Stack does not attempt to do any of this hiding, and I would strongly advocate against adding in such a restriction.

@snoyberg
Copy link
Contributor Author

snoyberg commented Jul 5, 2018

As an update: this does in fact seem to solve #4125.

@simonmichael
Copy link
Contributor

This sounds good to me. I have an addition for the list of tools:

If a tool is not in the hard-coded list, does/should the install fail, or fall back to the old behaviour ?

@snoyberg
Copy link
Contributor Author

snoyberg commented Jul 5, 2018

I'll add that, no problem. If it's not in the hard-coded list, and the executable isn't already available with that name, Stack will print a warning, and the build itself will most likely fail. Falling back to the old behavior would require all of the complicated code I mentioned which is suffering from the bugs right now and preventing performance optimizations.

@Ericson2314
Copy link

Ericson2314 commented Jul 5, 2018

@snoyberg [BTW, happy to talk about this elsewhere as I'm definitely veering off-topic.] Being able to only build the libraries components of build-depends is, I agree a small benefit. The real benefit is cross compilation: the tools must be built for the build platform, but libraries are built for the host platform. If everything is built together then either the libraries are for the wrong platform, or the tools won't run. If every build-depends package has its binaries built for the build platform, then we've doubled the build time for cross as a great number of packages are conservatively being built twice.

@snoyberg
Copy link
Contributor Author

snoyberg commented Jul 6, 2018

OK, that motivation makes a lot more sense, I hadn't heard it before. I still think it's best to leave Stack as-is for maximum backwards compatibility, I know in particular the hspec-discovery thing has caused breakage with new-build.

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

Successfully merging this pull request may close these issues.

stack 1.7.1 still has problems finding build tools ("The program 'happy' is required")
4 participants