Skip to content

Project config #3226

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

Merged
merged 9 commits into from
Mar 17, 2016
Merged

Project config #3226

merged 9 commits into from
Mar 17, 2016

Conversation

dcoutts
Copy link
Contributor

@dcoutts dcoutts commented Mar 13, 2016

Supersedes PR #3156. Significant changes since then:

  • Split huge ProjectConfig module into three as suggested by @23Skidoo

  • Proper QC tests: can round trip from ProjectConfig type out to a config file and back. Doing this detected lots of plumbing problems which are all now fixed.

  • Several field parsers have to be implemented locally rather than using generic infrastructure. This is directly related to the above point, we need to do that to round trip correctly.

  • Specifying packages in projects is greatly extended. Previously only one field for globs which could only match .cabal files and the globs were all allowed to match nothing. Matching nothing is useful in one major use case, allowing optional local or "vendor" unpacked deps, so one can just unpack and start using those local deps without any extra explict config however matching nothing is not ok for many other normal use cases. So we now split that into two fields packages and optional-packages. Only the latter can be globs that are allowed to match nothing.

  • We can now specify packages in the project as:

    • .cabal files
    • directories
    • local tarballs
    • any of the above via file globs
    • remote http tarballs
    • remote source repositories (git, darcs etc, same syntax as in .cabal files)

    Only the first two have full support. For the rest the syntax is supported but there's not yet support for downloading and unpacking them.

dcoutts added 8 commits March 13, 2016 19:51
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

Reviewing note: github shows the patches in the wrong order. It's showing them in date authoring order rather than repository addition order.

@dcoutts dcoutts mentioned this pull request Mar 13, 2016
@23Skidoo
Copy link
Member

Nice!

@dcoutts
Copy link
Contributor Author

dcoutts commented Mar 13, 2016

Phew! Tests pass.

@dcoutts
Copy link
Contributor Author

dcoutts commented Mar 13, 2016

@23Skidoo FYI, I can follow this up pretty soon with project planning and building. Indeed I'll probably make a parallel PR for those building on this one.

One issue I'm aware of in this PR is that the file globbing stuff needs a bit of cleaning up. It should be consolidated into the one module rather than spread around. Also, it has some syntactic limitations currently like it cannot match absolute paths nor trailing '/' dir slashes.

@23Skidoo
Copy link
Member

remote source repositories (git, darcs etc, same syntax as in .cabal files)

I think the existing syntax is not completely adequate for this, because we'll want to allow specifying the exact revision (actually, we'll want to force people to do it if the tag is not specified). Can be fixed later of course, since this part is not yet finished.

@23Skidoo
Copy link
Member

LGTM modulo minor comments.

@dcoutts
Copy link
Contributor Author

dcoutts commented Mar 15, 2016

allow specifying the exact revision

I was thinking about this. It looks like the tag field can double up as the revision for git. The original syntax for the "this" kind was of course always intended to specify an exact state of the repo, used to reproduce "this" release. We called the field tag rather than revision because at the time most other source control systems encouraged tagging to identify reusable repo states, especially for releases. But it's morally much the same thing and in git a tag is just a revision with a name. I think if you do specify a revision hash in the tag field then our current git code will check it out successfully.

That said, perhaps it's worth just aliasing "revision" to the existing "tag" field.

@23Skidoo
Copy link
Member

I think if you do specify a revision hash in the tag field then our current git code will check it out successfully.

Yes, it will run git checkout rev-number, which should work.

That said, perhaps it's worth just aliasing "revision" to the existing "tag" field.

+1.

@hvr
Copy link
Member

hvr commented Mar 15, 2016

Fyi, the tern "revision" is non-idiomatic in the context of Git and would therefore be very confusing IMHO. Idiomatically, you rather refer to "commit-ish" identifiers or sometimes "tree-ish" identifiers depending on what kind of reference you need (unless you need a more specific reference such as a "tag"). See also

https://www.kernel.org/pub/software/scm/git/docs/#_identifier_terminology

@23Skidoo
Copy link
Member

That said, perhaps it's worth just aliasing "revision" to the existing "tag" field.

However, this won't work in the case of Subversion, in which tags are separate from revisions. To check out a tag, you do svn checkout http://svn.example.com/repo/tags/mytag repo and to check out a revision you run something like svn checkout http://svn.example.com/repo/trunk repo && cd repo && svn up -rREV.

@23Skidoo
Copy link
Member

Fyi, the tern "revision" is non-idiomatic in the context of Git and would therefore be very confusing IMHO.

Stack uses "commit" instead of "revision".

@hvr
Copy link
Member

hvr commented Mar 15, 2016

Well, "commit" would be more appropriate (but still inaccurate!), even better would be "committish" if we went for VCS-specialised terms (which IMHO we should, as Git's language for object reference is quite rich -- or some may say over-engineered; terms appropriate for Git don't work well for Svn or Darcs, and vice-versa)

@dcoutts
Copy link
Contributor Author

dcoutts commented Mar 15, 2016

If they're genuinely different concepts for some vcss and just aliases in others, I see no problem adding an extra field to the .cabal file syntax too. The interpretation would, as it does now, depend on the vcs type. We'd then inherit that in the cabal.project files too.

@23Skidoo
Copy link
Member

I don't think we'll need "commit-ish" if we'll have both "commit"/"revision" and "tag". We can even validate the "commit" field to make sure that it looks like a SHA-1 hash (in the case of Git).

@dcoutts
Copy link
Contributor Author

dcoutts commented Mar 16, 2016

@23Skidoo So I suggest the Platform thing be re-evaluated later. I don't think it's needed, but if it is we can always change it.

As for the commit vs tag, changing this in the Cabal lib at this stage before a release is not realistic, so probably what we should do is add the commit thing locally to cabal-install in the 1.24 branch, and do it in Cabal lib in master and have cabal-install share the one from Cabal lib.

So good to merge now?

@dcoutts
Copy link
Contributor Author

dcoutts commented Mar 16, 2016

AppVeyor build failure looks spurious.

@23Skidoo
Copy link
Member

Yes, let's merge.

dcoutts added a commit that referenced this pull request Mar 17, 2016
@dcoutts dcoutts merged commit 40ad04c into haskell:1.24 Mar 17, 2016
@23Skidoo
Copy link
Member

Also cherry-picked into master.

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

Successfully merging this pull request may close these issues.

3 participants