Skip to content

Improve build-type defaulting #4958

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 5 commits into from
Dec 18, 2017
Merged

Improve build-type defaulting #4958

merged 5 commits into from
Dec 18, 2017

Conversation

hvr
Copy link
Member

@hvr hvr commented Dec 17, 2017

A personal DRY-pet-peeve of mine when writing .cabal files from scratch is forgetting about declaring the build-type, which historically will then be inferred to be Custom which is almost always not what is desired. Instead, a better default is Simple as that represents the most basic and recommended build-type. Morever, starting with cabal-version:1.24, we have a redundant signal in the .cabal file to statically imply a custom build-type, namely the custom-setup stanza.

To this end, this PR finally removes this papercut by improving the defaulting to use the following simple rules:

  • When cabal-version is set to 2.1 or higher, the default is Simple unless a custom-setup stanza exists, in which case the inferred default is Custom.
  • For cabal-versions below 2.1, the default is Custom unconditionally (legacy defaulting).

This allows us to bring down the minimal (modern) trivial cabal package definition down to a single file with 4 lines:

cabal-version: 2.1
name: mu
version: 0
library

NB: We don't need any Setup.hs file, as cabal new-build doesn't need any, and cabal sdist will magically generate one on the fly and include it in the source distribution.


Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Test-suite has been extended/updated

hvr added 3 commits December 17, 2017 00:32
This refactoring unifies the defaulting logic into a single place
paving the way for changing the defaulting logic.
This implements the following defaulting rules:

 * For `cabal-version:2.0` and below, default to the `Custom`
   build-type unconditionally (legacy defaulting)

 * Otherwise, if a `custom-setup` stanza is defined, default to
   the `Custom` build-type; else default to `Simple` build-type.

This gets us better defaults for the two most popular use-cases, and
which can be statically inferred by only looking at the `.cabal` file.

This allows us to bring down the minimal (modern) trivial cabal
package definition down to a single file with 4 lines:

    cabal-version: 2.1
    name: mu
    version: 0
    library

NB: We don't need any `Setup.hs` file, as `cabal sdist` will magically
generate one on the fly.
@hvr hvr added this to the 2.2 milestone Dec 17, 2017
@hvr hvr requested review from 23Skidoo and phadej December 17, 2017 08:39
@hvr hvr self-assigned this Dec 17, 2017
hvr added 2 commits December 17, 2017 15:39
This tweaks the existing `CustomPlain` test to test the legacy
defaulting logic for the unconditionally `Custom` default.

Morever, a new `SimpleDefault` test has been added which tests
that `cabal-version:2.1` does indeed infer `build-type: Simple`
when there is no `custom-setup` stanza defined.
@hvriedel
Copy link

In a future refactoring, UnknownBuildType should IMO be removed

@23Skidoo
Copy link
Member

Yes, makes sense.

@23Skidoo
Copy link
Member

Test failure is spurious, some apt-get failure.

Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM, I think this can go in.

@hvr hvr merged commit bf83d33 into haskell:master Dec 18, 2017
@hvr hvr deleted the pr/btype-defaulting branch December 18, 2017 16:51
Copy link
Collaborator

@phadej phadej left a comment

Choose a reason for hiding this comment

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

Side-note: one could create buildType lens too (it would be lawful under lax PD equivalence). Then pretty-printer would omit build-type field if it has default value. (Not sure we want that feature).

@@ -448,20 +448,20 @@ checkFields pkg =
"Package names with the prefix 'z-' are reserved by Cabal and "
++ "cannot be used."

, check (isNothing (buildType pkg)) $
, check (isNothing (buildTypeRaw pkg) && specVersion pkg < mkVersion [2,1]) $
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is great change, which one should mention in the changelog, It's updated in the manual (great!), but let's advertise this change in Changelog too.

Optionally, we can add a section in changelog gathering all cabal format related changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right!

Fwiw, I've got more related changes to for cabal check & build-type in the pipeline; the idea about structuring the changelog is a good one too, although I've been wanting to have an explicit cabal spec changelog in the user's guide as well... :-)

hvr added a commit to hvr/cabal that referenced this pull request Jan 6, 2018
This is a follow-up to haskell#4958 which opened up the opportunity to do
this make-illegal-states-unrepresentable refactoring as well.
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants