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

new-exec #4722

Merged
merged 37 commits into from Sep 26, 2017

Conversation

Projects
4 participants
@fgaz
Collaborator

fgaz commented Aug 24, 2017

Merged master with dmwit/master and finished new-exec.

  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Basic tests.

I had some trouble with git. There are two mege commits where I should have rebased instead, and now it's kinda difficult to fix (see those new-bench commits? they are from the merge.), but I suppose I'll have to do it before merging in master?

Also there's the question of how to rewrite the ghc program name. As a temporary solution I just check if it's ``elem["ghc", "ghcjs", "ghci"] and use it as is.
If there is no objection I'll change it to use the selected (`-w`) ghc version.

Fixes #3638 (finally!)

dmwit and others added some commits Feb 3, 2017

disable GHC environment files in another way
Although we may not want to create GHC environment files in a standard
location after every build, it is still useful to be able to build them
in a custom place in special circumstances. So let's disable this
feature at the call-sites where it doesn't work properly rather than
inside the implementation of the feature.
bare-bones new-exec support
This implements a bare-bones skeleton for cabal new-exec.

The old cabal exec gave programs access to a sandbox's package database.
By analogy, cabal new-exec should give programs access to the store's
package database; however, this database will be cluttered with many
non-project-related packages that may confuse issues. Therefore new-exec
selects just the packages that are in the current project's dependency
tree and makes them available to compiler tools. Currently only very new
GHCs are supported, via the GHC_ENVIRONMENT mechanism for selecting a
subset of some package databases.

Eventually we should probably also modify the PATH so that dependencies'
executables are available.
PATH modifications for new-exec
cabal new-exec should be allowed to run any executables available in the
dependencies for the current project. This patch introduces that
ability: we walk the installation plan, picking out a bin/ directory for
each package, then including it in the PATH if there are any executables
in that directory.
use the ProgramDb in new-exec
Previously, whichever version of compiler tools that were on the PATH
would be used. Going through the ProgramDb should let us use the
appropriate versions of these tools, instead.
use the install plan to modify PATH in new-exec
Previously, cabal new-exec would modify the PATH by including each
package's bin directory if it contained executables. This has some
drawbacks:

* Lots of filesystem probing
* Can have false positives due to dynamic libraries
* Mildly unpredictable

The new plan is to add each package's bin directory if the package
includes an executable stanza. This may include some directories that
have no executables in them (e.g. if the executable is not yet built or
is not in the install plan), but is more reproducible and requires less
filesystem access.
allow new-exec'ing of executables built inplace
Previously, new-exec would use the `build` directory when looking for
executables built inplace. But these executables are actually in
subdirectories of `build`, so the search would fail (potentially falling
back to an executable in the user's PATH instead). Now we place one
directory in the PATH for each executable in each inplace-build package
in the install plan.
Add args fallback when we can't use .ghc.env files
Still in progress. The support check doesn't work yet.
Add global package env to new-exec ghc invocations
The global db (note: not the user db) is needed (it contains `base`).
Reorder things a bit
Edge cases first, restrict do blocks...

@fgaz fgaz referenced this pull request Aug 24, 2017

Closed

implement a new-exec command #4304

@23Skidoo

This comment has been minimized.

Show comment
Hide comment
@23Skidoo

23Skidoo Aug 25, 2017

Member

I'm on vacation in Norway right now, but I'll review this when I get back next week.

Member

23Skidoo commented Aug 25, 2017

I'm on vacation in Norway right now, but I'll review this when I get back next week.

@cocreature

This comment has been minimized.

Show comment
Hide comment
@cocreature

cocreature Aug 28, 2017

Collaborator

FWIW, as a user I would like cabal new-exec to not rewrite commands and only set environment variables. Rewriting commands is bound to cause confusion and will always be really fragile, e.g., cabal new-exec -- ghc might work but cabal new-exec -- time ghc won’t. I can see a case for a way to execute ghc regardless of how the executable is called but I would prefer for that to be a separate cabal new-ghc command.

Collaborator

cocreature commented Aug 28, 2017

FWIW, as a user I would like cabal new-exec to not rewrite commands and only set environment variables. Rewriting commands is bound to cause confusion and will always be really fragile, e.g., cabal new-exec -- ghc might work but cabal new-exec -- time ghc won’t. I can see a case for a way to execute ghc regardless of how the executable is called but I would prefer for that to be a separate cabal new-ghc command.

@fgaz

This comment has been minimized.

Show comment
Hide comment
@fgaz

fgaz Aug 28, 2017

Collaborator

@cocreature the rewriting only happens as a fallback when using older versions of ghc that don't support .ghc.environment files, and eventually (well, after the 5 or so years of support...) will disappear. Other compilers will probably need rewriting though (I'm changing that code to be more general).

I agree that the naming is unfortunate, and I actually would prefer a new-ghc too, but new-exec is meant to be kinda compatible with old exec, and cabal -w "some-ghc" new-exec -- "some-ghc" will be completely equivalent to the hypotetical cabal -w "some-ghc" new-ghc.

Of course if there is enough support we can always split the command, it's only a few changes.

Collaborator

fgaz commented Aug 28, 2017

@cocreature the rewriting only happens as a fallback when using older versions of ghc that don't support .ghc.environment files, and eventually (well, after the 5 or so years of support...) will disappear. Other compilers will probably need rewriting though (I'm changing that code to be more general).

I agree that the naming is unfortunate, and I actually would prefer a new-ghc too, but new-exec is meant to be kinda compatible with old exec, and cabal -w "some-ghc" new-exec -- "some-ghc" will be completely equivalent to the hypotetical cabal -w "some-ghc" new-ghc.

Of course if there is enough support we can always split the command, it's only a few changes.

@cocreature

This comment has been minimized.

Show comment
Hide comment
@cocreature

cocreature Aug 28, 2017

Collaborator

@fgaz There will be an option to disable .ghc.environment.* files #4542 (which I am definitely going to use) so I don’t think you can rely on this ever going away.

Collaborator

cocreature commented Aug 28, 2017

@fgaz There will be an option to disable .ghc.environment.* files #4542 (which I am definitely going to use) so I don’t think you can rely on this ever going away.

@fgaz

This comment has been minimized.

Show comment
Hide comment
@fgaz

fgaz Aug 28, 2017

Collaborator

@cocreature that flag only applies to the .ghc.env file that every new- command generates in the project root. new-exec uses a separate and temporary .ghc.env file which is always generated regardless of that flag.

Collaborator

fgaz commented Aug 28, 2017

@cocreature that flag only applies to the .ghc.env file that every new- command generates in the project root. new-exec uses a separate and temporary .ghc.env file which is always generated regardless of that flag.

Match the configured compiler in new-exec
Alter the compiler flags only if the exe path matches the configured
compiler's one. Ex. `cabal -w ghc-8.0 new-exec ghc-8.0 Main.hs`

The flags depend on the compiler flavor. Only GHC and GHCJS are
supported right now.
case compilerId compiler
of CompilerId GHC _ -> argsEquivalentOfGhcEnvironmentFileGhc
CompilerId GHCJS _ -> argsEquivalentOfGhcEnvironmentFileGhc
CompilerId _ _ -> error "Only GHC and GHCJS are supported"

This comment has been minimized.

@fgaz

fgaz Sep 1, 2017

Collaborator

Is error appropriate? ImplInfo does it like this too.

@fgaz

fgaz Sep 1, 2017

Collaborator

Is error appropriate? ImplInfo does it like this too.

This comment has been minimized.

@23Skidoo

23Skidoo Sep 19, 2017

Member

It's OK.

@23Skidoo

23Skidoo Sep 19, 2017

Member

It's OK.

matchCompilerPath elaboratedShared program =
programPath program
`elem`
(programPath <$> configuredCompilers)

This comment has been minimized.

@fgaz

fgaz Sep 1, 2017

Collaborator

Should i canonicalize those paths?

@fgaz

fgaz Sep 1, 2017

Collaborator

Should i canonicalize those paths?

This comment has been minimized.

@23Skidoo

23Skidoo Sep 19, 2017

Member

Not 100% sure, probably not. Aren't the configured progs paths already absolute?

@23Skidoo

23Skidoo Sep 19, 2017

Member

Not 100% sure, probably not. Aren't the configured progs paths already absolute?

This comment has been minimized.

@fgaz

fgaz Sep 23, 2017

Collaborator

No idea. They seem absolute but maybe it's only the general case. I'll probably leave it like this and add a comment about it.

@fgaz

fgaz Sep 23, 2017

Collaborator

No idea. They seem absolute but maybe it's only the general case. I'll probably leave it like this and add a comment about it.

This comment has been minimized.

@23Skidoo

23Skidoo Sep 23, 2017

Member

OK. I think they should be absolute after the configure progs step.

@23Skidoo

23Skidoo Sep 23, 2017

Member

OK. I think they should be absolute after the configure progs step.

@fgaz fgaz requested a review from 23Skidoo Sep 1, 2017

@23Skidoo

This comment has been minimized.

Show comment
Hide comment
@23Skidoo

23Skidoo Sep 19, 2017

Member

OSX failure seems to be spurious.

Member

23Skidoo commented Sep 19, 2017

OSX failure seems to be spurious.

@23Skidoo 23Skidoo added this to the 2.2 milestone Sep 19, 2017

@23Skidoo

23Skidoo approved these changes Sep 19, 2017 edited

So this looks fine, nicely self-contained and ready to go in. Would be nice with a changelog note and a section in the user's guide, but that (and minor fixes I suggested) can be part of a separate PR.

Show outdated Hide outdated cabal-install/Distribution/Client/ProjectOrchestration.hs
Show outdated Hide outdated cabal-install/Distribution/Client/CmdExec.hs
case compilerId compiler
of CompilerId GHC _ -> argsEquivalentOfGhcEnvironmentFileGhc
CompilerId GHCJS _ -> argsEquivalentOfGhcEnvironmentFileGhc
CompilerId _ _ -> error "Only GHC and GHCJS are supported"

This comment has been minimized.

@23Skidoo

23Skidoo Sep 19, 2017

Member

It's OK.

@23Skidoo

23Skidoo Sep 19, 2017

Member

It's OK.

Show outdated Hide outdated cabal-install/Distribution/Client/ProjectPlanOutput.hs
matchCompilerPath elaboratedShared program =
programPath program
`elem`
(programPath <$> configuredCompilers)

This comment has been minimized.

@23Skidoo

23Skidoo Sep 19, 2017

Member

Not 100% sure, probably not. Aren't the configured progs paths already absolute?

@23Skidoo

23Skidoo Sep 19, 2017

Member

Not 100% sure, probably not. Aren't the configured progs paths already absolute?

@fgaz

This comment has been minimized.

Show comment
Hide comment
@fgaz

fgaz Sep 26, 2017

Collaborator

All done.
EDIT: oh wait, the docs

...except that git rerere failed me (or I failed it) and now we have three more commits that could have been fixupd. Can I merge anyway once ci passes?

Collaborator

fgaz commented Sep 26, 2017

All done.
EDIT: oh wait, the docs

...except that git rerere failed me (or I failed it) and now we have three more commits that could have been fixupd. Can I merge anyway once ci passes?

@23Skidoo

This comment has been minimized.

Show comment
Hide comment
@23Skidoo

23Skidoo Sep 26, 2017

Member

There's a merge conflict.

Member

23Skidoo commented Sep 26, 2017

There's a merge conflict.

@23Skidoo 23Skidoo merged commit 87a9dfa into haskell:master Sep 26, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@23Skidoo

This comment has been minimized.

Show comment
Hide comment
@23Skidoo

23Skidoo Sep 26, 2017

Member

Merged, thanks!

Member

23Skidoo commented Sep 26, 2017

Merged, thanks!

@fgaz fgaz moved this from current to done in Last Mile for `cabal new-build` (HSOC2017) Oct 17, 2017

@fgaz fgaz deleted the fgaz:new-exec/1 branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment