Skip to content

cabal-install integration-tests replacement omnibus patch #4124

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 56 commits into from
Nov 28, 2016

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Nov 20, 2016

UPDATE. This branch has been commandeered into a bigger integration-tests replacement patch


These flags allow users to configure the location of
cabal.project and dist-newstyle, respectively. Primary
motivation is for the test suite, to allow non-clobbering
inplace testing.

Bikeshed points:

  • GlobalFlags or InstallFlags? I picked GlobalFlags as it
    seemed more appropriate from a semantic perspective, but
    the downside is that you must now specify the flag
    before new-build, i.e., cabal --dist-dir=... new-build.
    However, if we ever support a Nix-style command that
    doesn't take the full complement of InstallFlags, GlobalFlags
    is the only place it could go.

  • Flag names? I think --project-file is good but perhaps
    someone has a better idea for --dist-dir.

  • Is this in duplication with globalConfigFile? I have
    no idea what that parameter does, so I don't think so.

It's not too easy to make the value of 'dist-dir' saveable
to the cabal.project file, as we'd prefer to load directly
from the cache if cabal.project is not updated.

CC @hvr

@mention-bot
Copy link

@ezyang, thanks for your PR! By analyzing the history of the files in this pull request, we identified @dcoutts, @23Skidoo and @brendanhay to be potential reviewers.

@hvr
Copy link
Member

hvr commented Nov 20, 2016

As pointed out on IRC, there's already an option in cabal install named

    --builddir=DIR                   The directory where Cabal puts generated
                                     build files (default dist)

@ezyang
Copy link
Contributor Author

ezyang commented Nov 21, 2016

It's not entirely clear to me that reusing --builddir here is the right thing to do, since --builddir needs to have different, distinct values for every subcall to Cabal. Could be error-prone.

@23Skidoo
Copy link
Member

23Skidoo commented Nov 21, 2016

From the user's point of view, what would be the diffrerence between --dist-dir and --builddir? We must explain it well to our users if we are going to have both.

@ezyang
Copy link
Contributor Author

ezyang commented Nov 21, 2016

I'm OK with these not being user facing.

A --builddir has a specific layout: you can find the executable named foo at $builddir/build/foo/foo. A --dist-dir guarantees a JSON plan file at $distdir/cache/plan.json but otherwise the format is unspecified.

@hvr
Copy link
Member

hvr commented Nov 21, 2016

Otoh, weren't we going to rename the default dist-newstyle to dist anyway? And what about the state when new-build is becoming the default and the "old-build" is getting removed?

@23Skidoo
Copy link
Member

I'm OK with these not being user facing.

Then I guess we should hide them in --help output?

@ezyang
Copy link
Contributor Author

ezyang commented Nov 25, 2016

Based on #4127, I've been convinced that we should use --builddir to control the location of dist-newstyle. Note that --builddir is a flag that is separately represented in every flags data structure (e.g., ConfigFlags, InstallFlags, CopyFlags; see ****DistPref) so some care must be taken. We cannot convert it into a global flag as that will break backwards compatibility with scripts that expect to be able pass --builddir after the subcommand (global flags must be specified before the subcommand.) Possibly for consistencies sake this means that --project-file should move out to (though I'm not sure to where.) I do not know if we can refactor dist-pref so that it doesn't show up in every flag settings individually.

@hvr
Copy link
Member

hvr commented Nov 25, 2016

@ezyang is there any chance to enhance the option parser to have it accept (some or all) global flags even after the subcommand? Then we could also make verbosity a global flag... (unless there's a reason not to)

@ezyang
Copy link
Contributor Author

ezyang commented Nov 25, 2016

We'd have to make more breaking API changes in this case, as GlobalFlags is not passed into many of the subcommand functions (e.g., configure only takes ConfigFlags.

@ezyang ezyang changed the title Support for --project-file and --dist-dir flags. cabal-install integration-tests replacement omnibus patch Nov 26, 2016
@ezyang
Copy link
Contributor Author

ezyang commented Nov 27, 2016

Retiring this PR for now.

@ezyang ezyang closed this Nov 27, 2016
@ezyang ezyang reopened this Nov 27, 2016
Signed-off-by: Edward Z. Yang <[email protected]>
Previously, these flags had no affect on new-build.
Now, they let you specify where the dist-newstyle directory
should go.

Note that if a relative path is provided, it is resolved
relative to the *project root*.  If this is undesirable,
pass an absolute path instead.

Fixes haskell#4127.

Signed-off-by: Edward Z. Yang <[email protected]>
This is a global flag, so it can only be specified prior to the
subcommand.  It controls the name of the cabal.project file which
new-build and related commands looks for.

Signed-off-by: Edward Z. Yang <[email protected]>
This information is useful for determining where the build products
are placed, so we can, e.g., run the built executables.

Signed-off-by: Edward Z. Yang <[email protected]>
Signed-off-by: Edward Z. Yang <[email protected]>
TODO: This hacks HOME so that it works in CI which doesn't
have old-time available in system GHC, remove that hack eventually.

Signed-off-by: Edward Z. Yang <[email protected]>
This is a pretty important new feature in the test suite, which
is to construct a remote repository on the fly as part of the
test suite.

The general principle is that you create a directory full of folders
for all of the packages you want available in the repo, and then
the 'withRepo' function will initialize this into a secure repo
you can do tests with.

Fixes haskell#4016.

Signed-off-by: Edward Z. Yang <[email protected]>
Note: hackage-repo-tool doesn't build with Windows, so that
support is commented out.

Signed-off-by: Edward Z. Yang <[email protected]>
Previously, in some cases we would carry around an explicit
FilePath for an executable that we wanted to invoke subsequently.

In this new scheme, any executable we want to execute gets registered
to the ProgramDb we are carrying around.  Now we can uniformly
use runProgramM in all cases.  Great!

Signed-off-by: Edward Z. Yang <[email protected]>
Previously, clients of runM had to explicitly record and check
the exit code of a run subcommand.  This has now been folded into
runM, so this is done always (which is what you wanted anyway.)

Signed-off-by: Edward Z. Yang <[email protected]>
TODO: This seems to cause Windows failure

Signed-off-by: Edward Z. Yang <[email protected]>
@ezyang
Copy link
Contributor Author

ezyang commented Nov 27, 2016

GREEEEEEEEEEEEEEN.

Otherwise, ghc-pkg will complain that it's reinitializing the
package database.

Possibly there is some refactor to make withPackageDb more robust
if it is called multiple times.

Signed-off-by: Edward Z. Yang <[email protected]>
This fixes some "permission denied" failures on Windows,
but it would be a lot better to fix properly.  See the comment
in Test.Cabal.Monad for more details.

Signed-off-by: Edward Z. Yang <[email protected]>
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.

4 participants