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

Project config #3156

Closed
wants to merge 61 commits into from
Closed

Project config #3156

wants to merge 61 commits into from

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented Feb 16, 2016

New code for review to implement the new project file system for the new nix-style approach.

Sorry, the main config module is quite long so you have to click through in the git web UI.

Fear not, #3129 is not forgotten. I'll do the review comments for those while people are looking at this stuff, they're independent so we can do them concurrently.

@dcoutts dcoutts mentioned this pull request Feb 16, 2016
@23Skidoo
Copy link
Member

Travis failure is due to -Werror:

Distribution/Client/DistDirLayout.hs:69:52: Warning:
    In the use of type constructor or class ‘InstalledPackageId’
    (imported from Distribution.Package):
    Deprecated: "Use UnitId instead"

DistDirLayout {..}
where
distDirectory = projectRootDirectory </> "dist-newstyle"
--TODO: switch to just dist at some point, or some other new name
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth it to change from dist to dist/$arch-$os-$compiler while we're at it for better isolation of build artifacts (see also #2790).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, so that's about fully parallel builds of different profiles. In a sense that's a difference of conception for projects vs sandboxes. At least at the moment a project has one configuration, with a single arch and compiler. An obvious future direction is to allow multiple profiles, but those ought to allow arbitrary differences, not just arch,os,compiler, but differences of flags or different instances of the same compiler version or whatever. This would not be too much work to implement provided we only allow one profile to be active at once. Having multiple active at once requires running the solver multiple times and combining the result into a big install plan.

Having a dist that automatically includes certain variables is a quite different approach. I'll have to think about it.

@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 16, 2016

Yep, will fix that warning and see if there's any other travis failures.

@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 17, 2016

@23Skidoo so my inclination is to defer a decision about how we manage multiple ways of doing the same build and not overwriting build artefacts, as I think it's a more general question than simply compiler versions (e.g. I want to do cabal build -O0 or cabal build --check and not overwrite the files I had for -O). I've added an item to #3104

Let me know if you're still looking at this PR or if you think its ok to merge (with squashed fixups).

@23Skidoo
Copy link
Member

so my inclination is to defer a decision about how we manage multiple ways of doing the same build and not overwriting build artefacts, as I think it's a more general question than simply compiler versions

Fine with me.

Let me know if you're still looking at this PR

Yes, I am, sorry for the delay.

@23Skidoo
Copy link
Member

Looks good in general, though there seems to be a lot of duplication with the existing code in the main config module, but I guess there is no way around that at the moment. Having a round trip test would be nice.

@23Skidoo
Copy link
Member

Also we should convert the boilerplate instances here and elsewhere to generics once 1.24 is out.

@hvr
Copy link
Member

hvr commented Feb 19, 2016

Also we should convert the boilerplate instances here and elsewhere to generics once 1.24 is out.

...is there already an issue to track this task?

@23Skidoo
Copy link
Member

@hvr

...is there already an issue to track this task?

There is one now: #3169.

@23Skidoo
Copy link
Member

May be also worth splitting some parts of ProjectConfig.hs module into its own module(s). E.g. the legacy-related code looks mostly independent from everything else.

@23Skidoo
Copy link
Member

So we could have, for example, D.C.ProjectConfig.Types and D.C.ProjectConfig.Legacy in addition to D.C.ProjectConfig.

23Skidoo and others added 8 commits February 23, 2016 00:55
(cherry picked from commit 7875b10)
This reverts commit 727f447.

This comment is unnecessary after haskell#3180. [ci skip]

(cherry picked from commit 89c24c9)
Since Cabal makes use of Maps and Sets we need them in place in our
Semigroups compat-layer.

(cherry picked from commit a03031e)
@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 27, 2016

So we could have, for example, D.C.ProjectConfig.Types and D.C.ProjectConfig.Legacy in addition to D.C.ProjectConfig.

Ok, that would shorten things a bit and separate some of the repetitive code from the more interesting code. I'm working on further config stuff, so I'll roll this all together.

@dcoutts
Copy link
Contributor Author

dcoutts commented Feb 27, 2016

This module doesn't seem to be imported from anywhere, so we're not checking that it compiles.

Actually it is imported by the ProjectConfig module.

@23Skidoo
Copy link
Member

Actually it is imported by the ProjectConfig module.

Ah, that's why Ctrl-f missed it.

hvr and others added 5 commits March 1, 2016 09:18
This implements the suggestions mentioned at
haskell#3169 (comment)

The main benefit of this change is turning 'ConfigFlags' into a uniform
product-type suitable for generic derivation of pointwise
`Semigroup`/`Monoid` instances.

NB: This changes the `Binary` serialisation of `ConfigFlags` since there's
now an additional `Maybe` inserted in `configPrograms`'s type

(cherry picked from commit 62c3aa6)
This implements the suggestion from
haskell#3196 (comment)

(cherry picked from commit 0077e2c)
This is preparatory work for implementing haskell#3169
it's kept in a different commit in order to facilitate
comparing code-generation.

(cherry picked from commit dd5fe69)
This is preparatory work for implementing haskell#3169
it's kept in a different commit in order to facilitate
comparing code-generation.

(cherry picked from commit 9b38b38)
23Skidoo and others added 26 commits March 4, 2016 19:31
(cherry picked from commit d137c93)
This commit removes references to the solver log that prevented it from being
garbage collected.  It also forces evaluation of the current level and variable
stack in 'Message.showMessages'.

(cherry picked from commit 37f28f2)
[ci skip]

(cherry picked from commit 4018b83)
(cherry picked from commit fc00e33)
(cherry picked from commit b345676)
Currently GHC 8.0 has a slightly different error message if it can't
find a `.hi` file. It's not clear yet if and how we're going to change the
message before GHC 8.0 final.

(cherry picked from commit 9c7131a)
The flag enable-executable-profiling warns to change it to just enable-profiling. So we'll fix that here too so we don't send people down an old path.
(cherry picked from commit c69dfb8)
Don't use './Setup' for building 'cabal-install'.
Exporting the default rendering style allows us to consistently format
text without needing to write a Text instance.
Fixes haskell#3157. The wrapText helper is used to format all error
messages. Previously, it was only used to format IOException errors;
other exceptions would be formatted incorrectly.
The error messages associated with ConfigStateFileError have been
reformatted and reworded for clarity.
Getters and setters really need to match up.

Detected by parse/print round trip QC tests.
withRepoContext keeps its current type (using GlobalFlags). Added
withRepoContext' that takes all the args seprately. Better name
suggestions welcome.
The existing approach has been to parse top level fields and then
separately parse each top level section, and similarly for pretty
printing. A better approach is to follow the pattern for fields and have
a section description. Then we just parse or print fields+sections in
one call. And as a bonus, secitons can have subsections (and could even
do so recursively).

This patch just adds the new functionality. No existing config parsing
is switched over.
This describes in one place the layout of the new dist dir, as used by
the nix-local-build branch. Also a similar approach to describing the
layout of the user-wide cabal directory.

The idea is that this centralises the description and makes it easier
to change and handle systematically (e.g. we have problems currently
with some user-wide files not being reolocatable).
One for testing if globs are trivial, as in they're constant strings
without the possibility of matching more than one thing. This can help
with making better error messages.

Another util for matching file globs in the rebuild monad, and
automatically monitoring the glob.
We want to reuse the FlagAssignment printer/parser for the config files.
This defines the new cabal.project files and introduces the notion of a
project root (and the logic for finding it). Also has support for
implicit projects when no cabal.project file is defined.

Supports both reading and writing project files or fragments. The
printing & parsing round trips correctly. QC tests to follow.

This is a key part of the new nix-local-build branch approach, based
around projects with clear configuration state held in a project file
(or extra files).

This has support for file and dirs as packages within a project,
including by glob. It supports both globs that much match a target, and
optional globs that are allowed to match nothing. It has partial support
for local tarball, remote http tarball and remote source repo packages.
Two kinds of round-trip test:
 * type conversion ProjectConfig -> LegcyProjectConfig and back
 * ProjectConfig -> print -> parse
The latter goes out to the config file format and back.

These tests uncovered a number of issues in our general config code.
@dcoutts
Copy link
Contributor Author

dcoutts commented Mar 13, 2016

Closing in favour of the new PR #3226.

@dcoutts dcoutts closed this Mar 13, 2016
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.

9 participants