-
Notifications
You must be signed in to change notification settings - Fork 696
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 new-show-build-info command #5954
Conversation
@@ -0,0 +1,34 @@ | |||
module Distribution.Simple.Utils.Json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this file should be removed, is that still true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely! We already have a JSON library (which is more or less a derivative of http://hackage.haskell.org/package/microaeson-0.1.0.0/docs/Data-Aeson-Micro.html) inside exe:cabal
.
By moving show-build-info into exe:cabal
we'd kill two birds with one stone...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I don't care whether we address this as part of this PR, or at a later stage; so feel free to consider this issue of the JSON library duplication punted to a follow-up PR
It may be worth leaving a TODO source-comment at the top of this module
Request review from @hvr, to validate that the output is relevant and correct. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, it builds and isn't a mess of merges. Well done, @fendor. I don't even want to know the git vodoo rituals you had to perform to get this :)
cabal has a PrettyPrinter class, right? could those be wrapped in
double-quotes?
…On Sun, Mar 24, 2019, 18:34 Daniel Gröber ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Awesome, it builds and isn't a mess of merges. Well done, @fendor
<https://github.com/fendor>. I don't even want to know the git vodoo
rituals you had to perform to get this :)
------------------------------
In Cabal/Distribution/Simple/Setup.hs
<#5954 (comment)>:
> @@ -2212,6 +2216,81 @@ optionNumJobs get set =
| otherwise -> Right (Just n)
_ -> Left "The jobs value should be a number or '$ncpus'"
+
+-- ------------------------------------------------------------
+-- * ghc-mod support flags
This isn't really ghc-mod specific.
------------------------------
In cabal-install/Distribution/Client/Setup.hs
<#5954 (comment)>:
> @@ -56,6 +56,9 @@ module Distribution.Client.Setup
, copyCommand
, registerCommand
+ --ghc-mod support commands
See above.
------------------------------
In cabal-install/Distribution/Client/Setup.hs
<#5954 (comment)>:
> @@ -2949,3 +2960,26 @@ relevantConfigValuesText :: [String] -> String
relevantConfigValuesText vs =
"Relevant global configuration keys:\n"
++ concat [" " ++ v ++ "\n" |v <- vs]
+
+
+-- ------------------------------------------------------------
+-- * Commands to support ghc-mod
Same.
------------------------------
In cabal-install/main/Main.hs
<#5954 (comment)>:
> @@ -302,7 +305,11 @@ mainWorker args = do
, hiddenCmd win32SelfUpgradeCommand win32SelfUpgradeAction
, hiddenCmd actAsSetupCommand actAsSetupAction
, hiddenCmd manpageCommand (manpageAction commandSpecs)
-
+ -- ghc-mod supporting commands
+ , hiddenCmd CmdShowBuildInfo.showBuildInfoCommand
I think having it hidden for now is ok. AFAIK that just affects whether
the command shows up in the command list in --help output. So if we don't
want users to discover this yet we can keep it hidden until we're done
fleshing it out.
------------------------------
In cabal-install/main/Main.hs
<#5954 (comment)>:
> + verbosity
+ distPref
+ useSandbox
+ noAddSource
+ (buildNumJobs buildFlags)
+ mempty
+ []
+ globalFlags
+ config
+ nixShell verbosity distPref globalFlags config $ do
+ maybeWithSandboxDirOnSearchPath useSandbox $ buildForCommand commandUI
+ verbosity
+ config'
+ distPref
+ buildFlags
+ extraArgs
Please keep the code style intact, this just looks horrid IMO.
Something like:
nixShell verbosity distPref globalFlags config $ do
maybeWithSandboxDirOnSearchPath useSandbox $
buildForCommand commandUI verbosity config' distPref buildFlags extraArgs
or if that is >80chars:
nixShell verbosity distPref globalFlags config $ do
maybeWithSandboxDirOnSearchPath useSandbox $ buildForCommand
commandUI verbosity config' distPref buildFlags extraArgs
------------------------------
In cabal-install/main/Main.hs
<#5954 (comment)>:
> + let noAddSource =
+ fromFlagOrDefault DontSkipAddSourceDepsCheck (buildOnly buildExFlags)
+ (useSandbox, config) <- loadConfigOrSandboxConfig verbosity globalFlags
+ distPref <- findSavedDistPref config (buildDistPref buildFlags)
+ -- Calls 'configureAction' to do the real work, so nothing special has to be
+ -- done to support sandboxes.
+ config' <- reconfigure configureAction
+ verbosity
+ distPref
+ useSandbox
+ noAddSource
+ (buildNumJobs buildFlags)
+ mempty
+ []
+ globalFlags
+ config
Same as below.
------------------------------
In cabal-install/main/Main.hs
<#5954 (comment)>:
>
+buildAction :: (BuildFlags, BuildExFlags) -> [String] -> Action
+buildAction flags@(buildFlags, _) = buildActionForCommand
+ (Cabal.buildCommand defaultProgramDb)
+ verbosity
+ flags
+ where verbosity = fromFlagOrDefault normal (buildVerbosity buildFlags)
+
+showBuildInfoAction :: (BuildFlags, BuildExFlags) -> [String] -> Action
+showBuildInfoAction flags@(buildFlags, _) = buildActionForCommand
+ (Cabal.showBuildInfoCommand defaultProgramDb)
+ verbosity
+ flags
+ -- Default silent verbosity so as not to pollute json output
+ where verbosity = fromFlagOrDefault silent (buildVerbosity buildFlags)
Wouldn't logging output go to stderr? This doesn't seem necessary to me.
Though I do vaguely remember new-build's Up to date message going to
stdout maybe.
------------------------------
In cabal-install/main/Main.hs
<#5954 (comment)>:
> +build = buildForCommand (Cabal.buildCommand defaultProgramDb)
+
+buildForCommand :: CommandUI BuildFlags
+ -> Verbosity
+ -> SavedConfig
+ -> FilePath
+ -> BuildFlags
+ -> [String]
+ -> IO ()
+buildForCommand command verbosity config distPref buildFlags extraArgs =
+ setupWrapper verbosity
+ setupOptions
+ Nothing
+ command
+ mkBuildFlags
+ (const extraArgs)
Same here, keep the code style as is please.
------------------------------
In cabal-install/main/Main.hs
<#5954 (comment)>:
> @@ -1246,3 +1292,23 @@ manpageAction commands flagVerbosity extraArgs _ = do
then dropExtension pname
else pname
putStrLn $ manpage cabalCmd commands
+
+--Further commands to support ghc-mod usage
ghc-mod reference again. If anything it should mention cabal-helper, but
I'm not sure that's appropriate.
------------------------------
In cabal-install/main/Main.hs
<#5954 (comment)>:
> + fromFlagOrDefault DontSkipAddSourceDepsCheck (buildOnly buildExFlags)
+ (useSandbox, config) <- loadConfigOrSandboxConfig verbosity globalFlags
+ distPref <- findSavedDistPref config (buildDistPref buildFlags)
+ -- Calls 'configureAction' to do the real work, so nothing special has to be
+ -- done to support sandboxes.
+ config' <- reconfigure configureAction
+ verbosity
+ distPref
+ useSandbox
+ noAddSource
+ (buildNumJobs buildFlags)
+ mempty
+ []
+ globalFlags
+ config
+ nixShell verbosity distPref globalFlags config $ do
If I'm looking at this right that do is redundant, Hlint would complain :)
------------------------------
In cabal-install/Distribution/Client/CmdWriteAutogenFiles.hs
<#5954 (comment)>:
> @@ -0,0 +1,220 @@
+-- | cabal-install CLI command: build
I think write-autogen-files should be a new-build flag instead. The
overhead of having a separate command is just not called for when all we
need to do is make new-build quit early.
------------------------------
In cabal-install/Distribution/Client/CmdShowBuildInfo.hs
<#5954 (comment)>:
> + ++ " " ++ pname ++ " new-build\n"
+ ++ " Build the package in the current directory or all packages in the project\n"
+ ++ " " ++ pname ++ " new-build pkgname\n"
+ ++ " Build the package named pkgname in the project\n"
+ ++ " " ++ pname ++ " new-build ./pkgfoo\n"
+ ++ " Build the package in the ./pkgfoo directory\n"
+ ++ " " ++ pname ++ " new-build cname\n"
+ ++ " Build the component named cname module Distribution.Client.InstallPlanin the project\n"
+ ++ " " ++ pname ++ " new-build cname --module Distribution.Client.InstallPlanenable-profiling\n"
+ ++ " Build the component in profilingmodule Distribution.Client.InstallPlan mode (including dependencies as needed)\n\n"
+
+ ++ cmdCommonHelpTextNewBuildBeta
+ }
+
+
+-- | The @build@ command does a lot. It brings the install plan up to date,
reference to the @build@ command, should say @show-build-info@ instead.
You should go over the code and fix that everywhere. commandNotes above
also has this problem.
------------------------------
In cabal-install/Distribution/Client/CmdShowBuildInfo.hs
<#5954 (comment)>:
> @@ -0,0 +1,254 @@
+-- | cabal-install CLI command: build
+--
+module Distribution.Client.CmdShowBuildInfo (
+ -- * The @build@ CLI and action
s/build/show-build-info/
------------------------------
In cabal-install/Distribution/Client/CmdShowBuildInfo.hs
<#5954 (comment)>:
> @@ -0,0 +1,254 @@
+-- | cabal-install CLI command: build
s/build/show-build-info/
------------------------------
In cabal-install/Distribution/Client/CmdShowBuildInfo.hs
<#5954 (comment)>:
> + ++ "Dependencies are built or rebuilt as necessary. Additional "
+ ++ "configuration flags can be specified on the command line and these "
+ ++ "extend the project configuration from the 'cabal.project', "
+ ++ "'cabal.project.local' and other files.",
+ commandNotes = Just $ \pname ->
+ "Examples:\n"
+ ++ " " ++ pname ++ " new-build\n"
+ ++ " Build the package in the current directory or all packages in the project\n"
+ ++ " " ++ pname ++ " new-build pkgname\n"
+ ++ " Build the package named pkgname in the project\n"
+ ++ " " ++ pname ++ " new-build ./pkgfoo\n"
+ ++ " Build the package in the ./pkgfoo directory\n"
+ ++ " " ++ pname ++ " new-build cname\n"
+ ++ " Build the component named cname module Distribution.Client.InstallPlanin the project\n"
+ ++ " " ++ pname ++ " new-build cname --module Distribution.Client.InstallPlanenable-profiling\n"
+ ++ " Build the component in profilingmodule Distribution.Client.InstallPlan mode (including dependencies as needed)\n\n"
Lots of s/new-build/show-build-info/ in here
------------------------------
In Cabal/Distribution/Simple/Utils/Json.hs
<#5954 (comment)>:
> + | JsonNumber !Int
+ | JsonObject [(String, Json)]
+ | JsonString !String
+
+renderJson :: Json -> ShowS
+renderJson (JsonArray objs) =
+ surround "[" "]" $ intercalate "," $ map renderJson objs
+renderJson (JsonBool True) = showString "true"
+renderJson (JsonBool False) = showString "false"
+renderJson JsonNull = showString "null"
+renderJson (JsonNumber n) = shows n
+renderJson (JsonObject attrs) =
+ surround "{" "}" $ intercalate "," $ map render attrs
+ where
+ render (k,v) = (surround "\"" "\"" $ showString k) . showString ":" . renderJson v
+renderJson (JsonString s) = surround "\"" "\"" $ showString s
I think this doesn't quote properly. When trying this out on exe:cabal I
get:
{"cabal_version":"3.0.0.0","compiler":{"flavour":"GHC","compiler_id":"ghc-8.4.4","path":"/usr/bin/ghc"},"components":[{"type":"executable","name":"CExeName (UnqualComponentName "cabal")", [...]
The "CExeName (UnqualComponentName "cabal")" bit is obviously not proper
json.
Hoenstly you should just be using a proper pretty printer for the
component name and not just show anyways.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#5954 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACNoMUQuffxWrXIkp_QPuv1bQqh7HMBzks5vaCeXgaJpZM4b_wRM>
.
|
255df31
to
056c810
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me longer to review than I promised. Thanks for doing this; this is a first good step!
So to move forward from here, the biggest concern for me is to
- Separate the autogen-files concern into a separate PR
- Simplify and avoid duplication by moving code into the frontend
- Align JSON schema with
plan.json
prior-art
UPDATE: #5954 (comment)
@@ -0,0 +1,34 @@ | |||
module Distribution.Simple.Utils.Json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, definitely! We already have a JSON library (which is more or less a derivative of http://hackage.haskell.org/package/microaeson-0.1.0.0/docs/Data-Aeson-Micro.html) inside exe:cabal
.
By moving show-build-info into exe:cabal
we'd kill two birds with one stone...
cabal-install/main/Main.hs
Outdated
verbosity | ||
flags | ||
-- Default silent verbosity so as not to pollute json output | ||
where verbosity = fromFlagOrDefault silent (buildVerbosity buildFlags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The stdout/stderr issue is a general issue (iirc we have a separate ticket for that) in cabal we don't need to solve in this PR :-)
Btw, I don't see a flag to redirect the output to a file. If there isn't we should have one (maybe --buildinfo-json-output=FILE
)
4252120
to
f711572
Compare
ef3090d
to
b0fd93a
Compare
Cabal/Distribution/Simple.hs
Outdated
let lbi' = lbi { withPrograms = progs } | ||
pkg_descr0 = localPkgDescr lbi' | ||
pkg_descr = updatePackageDescription pbi pkg_descr0 | ||
-- TODO: Somehow don't ignore build hook? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about this TODO? What is the consequence of ignoring build hooks, i.e. what's the worst that can happen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are even build hooks being ignored?
How could they influence the result?
@@ -0,0 +1,34 @@ | |||
module Distribution.Simple.Utils.Json |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I don't care whether we address this as part of this PR, or at a later stage; so feel free to consider this issue of the JSON library duplication punted to a follow-up PR
It may be worth leaving a TODO source-comment at the top of this module
-- It selects the 'AvailableTarget's that the 'TargetSelector' refers to, | ||
-- or otherwise classifies the problem. | ||
-- | ||
-- For the @write-autogen-files@ command select all components except non-buildable and disabled |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be moved to a separate PR as well if it concerns the write-autogen-files
command?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opiniion the comment might be outdated, since this function is neither exported nor used by write-autogen-files
import Distribution.Simple.Configure (tryGetPersistBuildConfig) | ||
import Data.List (find) | ||
|
||
showBuildInfoCommand :: CommandUI (ConfigFlags, ConfigExFlags, InstallFlags, HaddockFlags, TestFlags) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What needs to happen either here or in a subsequent PR is to implement the ability to refer to targets by their unique "unit id". So far this command appears to only support the less accurate symbolic target specifiers which aren't able to address all elements of a build-plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit Id as in TargetInfo.ComponentLocalBuildInfo.componentUnitId
? Or whose unit id?
Are we missing components?
liftOptions snd setSnd (buildExOptions showOrParseArgs) | ||
} | ||
where | ||
setFst a (_,b) = (a,b) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phadej don't we have _1
/_2
lenses for this somewhere? :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Such custom setters are used in all Cmd*
files. So, I either doubt it, or I can do a subsequent pr to replace that for every Cmd.
Now the output of Next is the flag for filtering unit-ids. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we really need some tests for this. Have a look at cabal-testsuite. If you need any help with that give me a shout.
a4271dc
to
d54726c
Compare
I'd need some help with how I should start with that. |
d93ec29
to
a804818
Compare
a804818
to
dec0166
Compare
@23Skidoo The tests are not running and the invocation still needs some improvements. E.g., currently invocations like |
576eeeb
to
928e92f
Compare
This feature is almost done, the tests fail for cabal-install due that |
@@ -375,6 +375,7 @@ library | |||
Distribution.Simple.Program.Types | |||
Distribution.Simple.Register | |||
Distribution.Simple.Setup | |||
Distribution.Simple.ShowBuildInfo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the functionality needs to be in Distribution.Simple
, can't it be implemented solely in cabal-install
. There should be a rationale why it cannot be, if there are legitimate reasons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the functionality needs to be in
Distribution.Simple
, can't it be implemented solely incabal-install
. There should be a rationale why it cannot be, if there are legitimate reasons.
There is, according to @hvr and @DanielG, and this pr is obsolete since the Cabal part has already been merged.
For discussions on cabal-install part, see: #6241
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good.
Sad we need support in Cabal
though, as we have to worry about build-type: Custom
stuff.
Closing based on #5954 (comment) |
Please include the following checklist in your PR:
[ci skip]
is used to avoid triggering the build bots.Please also shortly describe how you tested your change. Bonus points for added tests!
Based on the pull request #2771 and the work of @cfraz89.
Rebases on cfraz89's work to work on master.
Introduces the command
new-show-build-info
.Example output for my local
cabal-install
package:Example output