Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Build simplifications #1168

Merged
merged 13 commits into from
Apr 20, 2019
Merged

Build simplifications #1168

merged 13 commits into from
Apr 20, 2019

Conversation

samuelpilz
Copy link
Contributor

This pull-request is the result of my review of the install.hs file.

Following improvements were made:

  • hoogle-database is only generated once. The same file was generated every time.
  • cabal-install is not installed via stack if the executable cabal can already be found.

A review from @fendor has been conducted and the suggestions have been included.

@fendor fendor added build Continuous integration and building gardening 🌱 Small tidy-ups and maintenance labels Apr 9, 2019
@fendor fendor requested review from Anrock and fendor and removed request for Anrock April 9, 2019 10:44
install.hs Outdated Show resolved Hide resolved
install.hs Outdated Show resolved Hide resolved
install.hs Outdated Show resolved Hide resolved
install.hs Outdated Show resolved Hide resolved
@nponeccop
Copy link
Contributor

@fendor Tastes differ. My eyes bleed when I see these maybe-cases and return Just. My opinion is that <$> isn't exotic at all, it's idiomatic in day-to-day Haskell code, and it's almost always improves readability. It's used 78 times in 16 files in hie. It's not the right place to discuss coding guidelines here though, so I closed the conversation so it doesn't seem like an outstanding issue preventing the PR from merging.

install.hs Outdated Show resolved Hide resolved
install.hs Outdated Show resolved Hide resolved
install.hs Outdated Show resolved Hide resolved
install.hs Outdated Show resolved Hide resolved
install.hs Show resolved Hide resolved
install.hs Show resolved Hide resolved
install.hs Show resolved Hide resolved
install.hs Outdated Show resolved Hide resolved
install.hs Show resolved Hide resolved
@alanz
Copy link
Collaborator

alanz commented Apr 17, 2019

I just checked out this branch, deleted my .stack-work and gave it ./install.hs build-all.

It first built and ran hoogle, and then started building Cabal-2.4.1.0, which was not built by the prior version, and is already on my machine, as far as I know.

It then started building cabal-install-2.0.0.0, which as far as I am concerned should not be installed, as it is an old version. This is for the hie-8.2.1 target, I think.

Stopping the build and restarting it does not re-install Cabal or cabal-install, so I guess they are used for the specific hoogle in use. Which should probably be build against the hie-8.6.4 baseline, rather than the hie-8.2.1 one.

@samuelpilz
Copy link
Contributor Author

@alanz Thank you for testing my PR.

cabal-install and hoogle are runtime-dependencies that need to be built if not already present on the system. To minimize the number of downloaded ghcs, they are built using the same ghc as the install.hs file is beeing executed. It should not build cabal-install or hoogle if the are already present in the path.

I am unable to reproduce the behavior that cabal-install-2.0.0.0 is installed. For me, it is the version as provided in the resolver from the shake.yaml, which is cabal-install-2.4.1.0.

@fendor fendor self-requested a review April 17, 2019 18:31
Copy link
Collaborator

@fendor fendor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You changed the API of install.hs, the README must be adapted before merge.

@samuelpilz
Copy link
Contributor Author

samuelpilz commented Apr 18, 2019

Following additional features are implemented:

@fendor
Copy link
Collaborator

fendor commented Apr 19, 2019

LGTM, ready to merge in my opinion, I tested it on windows.

@alanz
Copy link
Collaborator

alanz commented Apr 20, 2019

If you guys are happy with this, then merge it.

@Anrock
Copy link
Collaborator

Anrock commented Apr 20, 2019

I see it also closes #1008 and #1155

@fendor
Copy link
Collaborator

fendor commented Apr 20, 2019

In the short help message, the nightly ghc version seems to be excluded. If this is fine, then we can merge, otherwise this needs to be added to the short-help message.

install.hs Show resolved Hide resolved
@fendor fendor merged commit 243326d into haskell:master Apr 20, 2019
@samuelpilz samuelpilz deleted the build-simplifications branch April 24, 2019 06:11
@alanz alanz added this to the 2019-04 milestone May 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
build Continuous integration and building gardening 🌱 Small tidy-ups and maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants