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

Add --build-depends flag, associated support to new-repl #5454

Merged
merged 9 commits into from Jul 29, 2018

Conversation

typedrat
Copy link
Collaborator

@typedrat typedrat commented Jul 24, 2018

Closes #5425. Adds --package flag, allows new-repl use outside of project folders.

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.

@typedrat typedrat self-assigned this Jul 24, 2018
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.

Looks OK in general, but I have some comments.

, pkgVersion == nullVersion = Dependency pkgName anyVersion
| otherwise = thisPackageVersion pkgId

buildInfoL :: ComponentName -> Traversal' PackageDescription BuildInfo
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it could be useful elsewhere, maybe move it somewhere closer to the definitions of PackageDescription/BuildInfo (e.g. D.PackageDescription.Lens)?

@@ -236,7 +236,7 @@ data PackageSpecifier pkg =
-- | A fully specified source package.
--
| SpecificSourcePackage pkg
deriving (Eq, Show, Generic)
deriving (Eq, Show, Functor, Generic)
Copy link
Member

Choose a reason for hiding this comment

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

I'd rather prefer if we used an explicit lambda with case analysis or a mapSpecificSourcePackage helper here. Code like fmap fmap is not super readable.

envIncludeTransitive (\p flags -> flags { envIncludeTransitive = p })
falseArg
, option ['z'] ["only-specified"]
"Only include explicitly specified packages (and 'base'). This implies '--no-transitive-deps'."
Copy link
Member

Choose a reason for hiding this comment

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

Description doesn't make it clear how --only-specified differs from --no-transitive-deps. I guess we should mention environment files somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They differ if you're in a project/package context, --only-specified ignores that context.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, I figured, but it's not obvious from the description. Maybe we should call it --ignore-project?

Copy link
Member

Choose a reason for hiding this comment

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

I'm against introducing a new flag for something we can already express with current flags (especially because it makes things awkward when such competing/conflciting flags get turned into cabal.config/cabal.project flags):

We already have --project-file=, and I've been arguing that we can just have --project-file='' denote to ignore any project context; i.e. force a project-less context.

, let
isGlobErr (BadLocGlobEmptyMatch _) = True
isGlobErr _ = False
, any isGlobErr locs ->
Copy link
Member

Choose a reason for hiding this comment

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

Code golf: can also be written as notNull [ () | BadLocGlobEmptyMatch _ <- locs ].


let
targetSelectors = [TargetPackage TargetExplicitNamed [pkgId] Nothing]
finalizer = return () --handleDoesNotExist () (removeDirectoryRecursive tempDir)
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, so finalizer is return () in both cases?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, that's just for debugging. This is a WIP.

-- help us resolve the targets, but that isn't ideal for performance,
-- especially in the no-project case.
withInstallPlan (lessVerbose verbosity) baseCtx $ \elaboratedPlan -> do
-- Interpret the targets on the command line as repl targets
Copy link
Member

Choose a reason for hiding this comment

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

The targets <- ... and the Reject multiple targets... bits look like they could be moved to a helper function to avoid copy-paste.

(reqArg "PACKAGE" packageIdReadE (fmap prettyShow :: [PackageId] -> [String]))
, option [] ["no-transitive-deps"]
"Don't automatically include transitive dependencies of requested packages."
envIncludeTransitive (\p flags -> flags { envIncludeTransitive = p })
Copy link
Member

Choose a reason for hiding this comment

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

I don't see how envIncludeTransitive is used.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I haven't added code to include the transitive dependencies yet. :)

@hvr
Copy link
Member

hvr commented Jul 26, 2018

I briefly discussed the naming choice --package with @typedrat, and I'm convinced that it's a misnomer; the better naming would be something akin to --build-depends, as a) we care only about the library components, and b) we want consistency w/ with pre-existing terms, in this case from the .cabal file; c) when we extend the syntax of build-depends to specify public multi-libs packages, we'd increase the level of misnaming even more, as some multi-lib (I have at least one in mind which will be) that will be designed in a way to deliberate clash with each other, and so you must be able to select the scope at the component level. d) reusing the build-depends syntax is more convenient as you can specify the version constraints syntax directly w/o being forced to resort to --constraints (this is e.g. what cabal-env.sh does)

@typedrat typedrat moved this from In Progress to Needs Review in Have new-build become build (GSOC2018) Jul 27, 2018
@typedrat typedrat moved this from Needs Review to In Progress in Have new-build become build (GSOC2018) Jul 27, 2018
@typedrat typedrat changed the title [WIP] Add --package flag, associated support to new-repl Add --package flag, associated support to new-repl Jul 27, 2018
@typedrat typedrat moved this from In Progress to Needs Review in Have new-build become build (GSOC2018) Jul 27, 2018
@typedrat
Copy link
Collaborator Author

For some reason, outside of packages, this is failing with an error about being unable to find base.

@typedrat typedrat moved this from Needs Review to Done in Have new-build become build (GSOC2018) Jul 29, 2018
@typedrat typedrat changed the title Add --package flag, associated support to new-repl Add --build-depends flag, associated support to new-repl Jul 29, 2018
@typedrat typedrat merged commit 36bfab3 into haskell:master Jul 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants