Issues, issues, issues...

Francesco Ariis fa-ml at
Thu Jun 25 16:32:14 UTC 2015

On Thu, Jun 25, 2015 at 05:55:47PM +0200, Bardur Arantsson wrote:
> I noticed another thing while perusing the source code: There seem to be
> quite a few TODO comments scattered about.
> Is there some sort of convention whereby it is permitted to add TODOs as
> long as the person doing so pinkie-promises to fix/remove them later? Or
> is it somehow related to severity? Or is it just that people couldn't be
> bothered to make issues and just have them in out-of-band instead? Is it
> all just legacy from before issue tracking was introduced? Do people
> spontaneously clean up TODOs that others have left behind?

I recently released a little something [1] to scan for TODOs in a
codebase (after being overwhelmed by them), so after your comment
I felt compelled to see how cabal and its repo would fare!

I attach the output: there are a lot of todos but the number more or less
on the same level with projects of similar complexity. I guess it is

-------------- next part --------------
    40  Not sure about JHC
    41  This file should probably be removed.
    80  We want to tell fdToHandle what the file path is, as any exceptions
        etc will only be able to report the FD currently
    89  bits copied from System.FilePath [fixme]
    96  Should use System.FilePath library [fixme]
   104  Copied from GHC.Handle [fixme]
    92  In some future release, remove 'parseCompilerFlavorCompat' and use
        ordinary 'parse'. Also add ("nhc", NHC) to the above 'compilerMap'.
    66  * remove BSD4
   356  dedupe?
   547  By having a 'testEnabled' field in the PackageDescription, we are
        mixing build status information (i.e., arguments to 'configure')
        with static package description information. This is undesirable,
        but a better solution is waiting on the next overhaul to the
        GenericPackageDescription -> PackageDescription resolution process.
   688  See TODO for 'testEnabled'.
   921  many of the places where this is used, we actually want to look at
        unbuildable bits too, probably need separate functions [fixme]
  1141  make PackageDescription an instance of Text.
   162  make this variant go away we should always know the
   202  check for name clashes case insensitively: windows file systems
        cannot cope.
   466  recommend the bug reports URL, author and homepage fields
   467  recommend not using the stability field
   468  recommend specifying a source repo
   597  check location looks like a URL for some repo types.
   876  check sets of paths that would be interpreted differently between
        Unix and windows, ie case-sensitive or insensitive. Things that
        might clash, or conversely be distinguished.
   880  use the tar path checks on all the above paths
  1145  If the user writes build-depends: foo with (), this is
        indistinguishable from build-depends: foo, so there won't be an
        error even though there should be [xxx]
  1285  What we really want to do is test if there exists any configuration
        in which the base version is unbounded above. However that's a bit
        tricky because there are many possible configurations. As a cheap
        easy and safe approximation we will pick a single "typical"
        configuration and check if that has an open upper bound. To get a
        typical configuration we finalise using no package index and the
        current platform.
   115  treat Nothing as unknown, rather than empty list once we support
        partial resolution of system parameters [fixme]
   123  Add instances and check
   207  The current algorithm is rather naive. A better approach would be
   495  we need to find a way to avoid pulling in deps for non-buildable
        components. However cannot simply filter at this stage, since if
        the package were not available we would have failed already.
   550  One particularly tricky case is defaulting. In the original package
        description, e.g., the source directory might either be the default
        or a certain, explicitly set path. Since defaults are filled in
        only after the package has been resolved and when no explicit value
        has been set, the default path will be missing from the package
        description returned by this function.
   681  this should take a ByteString, not a String. We have to be able to
        decode UTF8 and handle the BOM. [fixme]
  1228  make this use section syntax add equivalent for
    80  this is a temporary hack. Ideally, fields containing default values
        would be filtered out when the @FieldDescr a@ list is generated.
   228  this ends up printing trailing spaces when combined with nest.
   125  what is this double parse thing all about? Can't we just do the all
        isSpace test the first time?
   251  this is a bit smelly hack. It's because we want to parse bool
        fields liberally but not accept new parses. We cannot do that with
        ReadP because it does not support warnings. We need a new parser
   402  should we write the modified package descr back to the
   115  This is abusing the notion of a 'PathTemplate'. The result isn't
        necessarily a path.
   521  build separate libs in separate dirs so that we can build multiple
        libs, e.g. for 'LibTest' library-style test suites
   102  This should be determined via autoconf (AC_EXEEXT) | Extension for
        executable files (typically @\"\"@ on Unix and @\"exe\"@ on Windows
        or OS\/2)
   110  This should be determined via autoconf (AC_OBJEXT) | Extension for
        object files. For GHC the extension is @\"o\"@.
   585  eliminate this function and turn it into a variant on
        commandAddAction instead like commandAddActionNoArgs that doesn't
        supply the [String]
   372  should use a per-compiler method to map the source package ID into
        an installed package id we can use for the internal package set.
        The open-codes use of InstalledPackageId . display here is a hack.
   400  mention '--exact-configuration' in the error message when this
   544  Do we need the internal deps? NB: does *not* include holeDeps!
   944  we don't check that all dependencies are used!
  1023  internal dependencies (e.g. the test package depending on the main
        library) is not currently supported
  1429  Refine this check for signatures
  1474  produce a log file from the compiler errors, if any.
  1539  do we also need dependent packages' ld options?
   461  do we need to put hs-boot files into place for mutually recursive
   572  problem here is we need the .c files built first, so we can load
        them with ghci, but .c files can depend on .h files generated by
        ghc by ffi exports.
   729  do we need to put hs-boot files into place for mutually recursive
        modules? FIX: what about exeName.hi-boot?
   869  problem here is we need the .c files built first, so we can load
        them with ghci, but .c files can depend on .h files generated by
        ghc by ffi exports.
   220  should be using --supported-languages rather than hard coding
   400  perhaps override?
   344  do we need to put hs-boot files into place for mutually recursive
   454  problem here is we need the .c files built first, so we can load
        them with ghci, but .c files can depend on .h files generated by
        ghc by ffi exports.
   572  do we need to put hs-boot files into place for mutually recursive
        modules? FIX: what about exeName.hi-boot?
   705  problem here is we need the .c files built first, so we can load
        them with ghci, but .c files can depend on .h files generated by
        ghc by ffi exports.
   713  these should be moved elsewhere.
    74  decide if we need the user to be able to control the libdir for
        shared libs independently of the one for static libs. If so it
        should also have a flag in the command line UI For the moment use
        dynlibdir = libdir
   211  does lhc support -XHaskell98 flag? from what version? [fixme]
   343  do we need to put hs-boot files into place for mutually recursive
   470  discover this at configure time or runtime on Unix The value is 32k
        on Windows and POSIX specifies a minimum of 4k but all sensible
        Unixes use more than 4k. we could use getSysVar ArgumentLimit but
        that's in the Unix lib
   508  do we need to put hs-boot files into place for mutually recursive
        modules? FIX: what about exeName.hi-boot?
   113  inplaceDirTemplates :: InstallDirs FilePath
   159  what about non-buildable components?
   119  Clarify what "preference order" means. Check that this invariant is
        preserved. See #1463 for discussion. [fixme]
   125  deal with pre-processors that have implementaion dependent output
        eg alex and happy have --ghc flags. However we can't really inlcude
        ghc-specific code into supposedly portable source tarballs.
   227  try to list all the modules that could not be found not just the
        first one. It's annoying and slow due to the need to reconfigure
        after editing the .cabal file each time.
   274  eliminate sdist variant, just supply different handlers
   295  This is a somewhat nasty hack. GHC requires that hs-boot files be
        in the same place as the hs files, so if we put the hs file in
        dist/ then we need to copy the hs-boot file there too. This should
        probably be done another way. Possibly we should also be looking
        for .lhs-boot files, but I think that preprocessors only produce
        .hs files. [fixme]
   498  install .chi files for packages, so we can --include those dirs
        here, for the dependencies
   513  perhaps use this with hsc2hs too
   514  remove cc-options from cpphs for cabal-version: >= 1.10
   551  move this into the compiler abstraction
   552  this forces GHC's crazy 4.8.2 -> 408 convention on all the other
        compilers. Check if that's really what they want. [fixme]
   180  this could be a lot faster. We're doing normaliseLineEndings twice
        and converting back and forth with lines/unlines.
   241  use a proper named function for the conversion from source package
        id to installed package id
   251  discover this at configure time or runtime on unix The value is 32k
        on Windows and posix specifies a minimum of 4k but all sensible
        unixes use more than 4k. we could use getSysVar ArgumentLimit but
        that's in the unix lib [fixme]
   129  there's really no guarantee this will work. registering into a
        totally different db stack can fail if dependencies cannot be
        satisfied. [fixme]
   166  eliminate pwd!
   169  the method of setting the InstalledPackageId is compiler specific
        this aspect should be delegated to a per-compiler helper.
   112  Not sure where this should live [fixme]
   274  the configPrograms is only here to pass info through to configure
        because the type of configure is constrained by the UserHooks. when
        we change UserHooks next we should pass the initial
        ProgramConfiguration directly and not via ConfigFlags ^All programs
        that cabal may run [fixme]
   359  reverse this
  1519  this one should not be here, it's just that the silly UserHooks
        stop us from passing extra info in other ways
  1554  re-enable once we have support for module/file targets ++ " " ++
        pname ++ " build Foo.Bar " ++ " A module\n" ++ " " ++ pname ++ "
        build Foo/Bar.hs" ++ " A file\n\n" ++ "If a target is ambiguous it
        can be qualified with the component " ++ "name, e.g.\n" ++ " " ++
        pname ++ " build foo:Foo.Bar\n" ++ " " ++ pname ++ " build
  1687  re-enable once we have support for module/file targets ++ " " ++
        pname ++ " repl Foo.Bar " ++ " A module\n" ++ " " ++ pname ++ "
        repl Foo/Bar.hs" ++ " A file\n\n" ++ "If a target is ambiguous it
        can be qualified with the component " ++ "name, e.g.\n" ++ " " ++
        pname ++ " repl foo:Foo.Bar\n" ++ " " ++ pname ++ " repl
  1744  do we need this instance?
  1756  think about if/how options are passed to test exes
  2128  kill off thic bc hack when defaultUserHooks is removed.
   155  This is abusing the notion of a 'PathTemplate'. The result isn't
        necessarily a path.
   159  This is abusing the notion of a 'PathTemplate'. The result isn't
        necessarily a path.
   114  determine in some other way
   139  Actually make use of the information provided in the file.
   497  handle exceptions like text decoding.
   509  this probably fails if the process refuses to consume or if it
        closes stdin (eg if it exits)
   197  probably should disallow starting with a number
    93  maybe move this to Distribution.Package.Version? (package-specific
        versioning scheme).
   312  Convert to a "-v" flag instead.
    23  Turn this into a utility function
   311  see equivalentVersionRange for details [fixme]
   557  this is wrong. consider version ranges "<=1" and "<1.0" this
        algorithm cannot distinguish them because there is no version that
        is included by one that is excluded by the other. Alternatively we
        must reconsider the semantics of '<' and '<=' in version ranges /
        version intervals. Perhaps the canonical representation should use
        just < v and interpret "<= v" as "< v.0". [fixme]
   194  this does not allow for optional or repeated fields [fixme]
    63  make this concurrency safe, either lock the report file or make
        sure the writes for each report are atomic (under 4k and flush at
    88  make this concurrency safe, either lock the report file or make
        sure the writes for each report are atomic
   103  In principle, we can support $pkgkey, but only if the configure
        step succeeds. So add a Maybe field to the build report, and either
        use that or make up a fake identifier if it's not available.
    71  do something if the request fails [fixme]
    36  this may give more warnings than it should give; consider two
        branches of a condition, one saying ghc-options: -Wall and the
        other ghc-options: -Werror joined into ghc-options: -Wall -Werror
        checkPackages will yield a warning on the last line, but it would
        not on each individual branch. Hovever, this is the same way
        hackage does it, so we will yield the exact same errors as it will.
   104  What if the result is not representable as POSIX seconds? Probably
        fine to return garbage.
   253  NubListify
   255  NubListify
   267  NubListify
   278  NubListify
   280  NubListify
   285  NubListify
   291  NubListify
   293  NubListify
   296  NubListify
   313  NubListify
   315  NubListify
   351  NubListify
   353  NubListify
   450  misleading, there's no way to override this default either make it
        possible or rename to simply getCabalDir.
   610  this is only here because viewAsFieldDescr gives us a parser that
        only recognises 'ghc' etc, the case-sensitive flag names, not what
        the normal case-insensitive parser gives us. [fixme]
   616  The following is a temporary fix. The "optimization" and
        "debug-info" fields are OptArg, and viewAsFieldDescr fails on that.
        Instead of a hand-written hackaged parser and printer, we should
        handle this case properly in the library.
   682  this is a hack, hiding the user name and password. But otherwise it
        masks the upload ones. Either need to share the options or make
        then distinct. In any case they should probably be per-server.
   697  next step, make the deprecated fields elicit a warning.
   256  should warn or error on constraints that are not on direct deps or
        flag constraints not on the package in question.
   264  the top down resolver chokes on the base constraints below when
        there are no targets and thus no dep on base. Need to refactor
        constraints separate from needing packages.
   284  this should work using exclude constraints instead
   295  this should work using exclude constraints instead
   305  this should work using exclude constraints instead
   498  warn about unsupported options
   513  is this needed here? see dontUpgradeNonUpgradeablePackages
    74  This function is (sort of) ok. However, there's an open bug w.r.t.
        unqualification. There might be several different instances of one
        package version chosen by the solver, which will lead to clashes.
   123  data structure conversion is rather ugly here
   128  Should we include the flag default in the tree?
   148  We could inline this above.
    78  This isn't the ideal location to declare the type, but we need them
        for constrained instances.
   137  Different pairs might have different conflict sets. We're obviously
        interested to return a conflict that has a "better" conflict set in
        the sense the it contains variables that allow us to backjump
        further. We might apply some heuristics here, such as to change the
        order in which we check the constraints.
   134  This isn't quite optimal, because we do not merely report the shape
        of the tree, but rather make assumptions about where that shape
        originated from. It'd be better if the pruning itself would leave
        information that we could pick up at this point.
    77  Installed packages should also store their encapsulations!
   187  Nothing should be treated as unknown, rather than empty list. This
        code should eventually be changed to either support partial
        resolution of compiler flags or to complain about incompletely
        configured compilers. [fixme]
   360  The enumeration of OptionalStanza names is very brittle; if a
        constructor is added to the datatype we won't notice it here
    33  More information is needed about the repo.
   212  It would be better to actually check the reverse dependencies of
        installed packages. If they're not depended on, then reinstalling
        should be fine. Even if they are, perhaps this should just result
        in trying to reinstall those other packages as well. However, doing
        this all neatly in one pass would require to change the builder, or
        at least to change the goal set after building.
    97  when we add support for remote tarballs then this message will need
        to be changed because for remote tarballs we fetch them at the
        earlier phase.
    37  alternatively, we might consider looking for the two magic bytes at
        the beginning of the gzip header.
   111  add command-line constraint and preference args for unpack
    58  print info message when we're using a proxy based on verbosity
   157  check the content-length header matches the body length. [fixme]
   158  stream the download into the file rather than buffering the whole
        thing in memory.
    97  make getInstalledPackages use sensible verbosity in the first place
   382  really should use guessed source roots. [xxx]
   178  we should probably make a better attempt at parsing comments above.
        Unfortunately we can't use a full-fledged Haskell parser since
        cabal's dependencies must be kept at a minimum. [xxx]
   220  use a better error message, remove duplication.
   228  Make InstallContext a proper data type with documented fields. |
        Common context for makeInstallPlan and processInstallPlan.
   233  Make InstallArgs a proper data type with documented fields or just
        get rid of it completely. | Initial arguments given to 'install' or
   372  this just applies all flags to all targets which is silly. We
        should check if the flags are appropriate [fixme]
   412  this is a general feature and should be moved to D.C.Dependency
        Also, the InstallPlan.remove should return info more precise to the
        problem, rather than the very general PlanProblem type.
   554  This is a bit of a hack, pretending that each package is installed
        It's doubly a hack because the installed package ID didn't get
        updated... [fixme]
   668  this should be a proper function in a proper place [fixme]
   770  does not handle flags [fixme]
   855  might be nice if the install plan gave us the new
  1293  'cabal get happy && cd sandbox && cabal install ../happy' still
        fails even with this workaround. We probably can live with that.
   652  It would be nicer to use ComponentDeps here so we can be more
        precise in our checks. That's a bit tricky though, as this
        currently relies on the 'buildDepends' field of
        'PackageDescription'. (OTOH, that field is deprecated and should be
        removed anyway.) As long as we _do_ use a flat list here, we have
        to allow for duplicates when we fold specifiedDeps; once we have
        proper ComponentDeps here we should get rid of the `nubOn` in
   661  use something lower level than finalizePackageDescription
   112  do we want to do this here? : createDirectoryIfMissing True
   380  exclude non-buildable exes
   440  installed package info is missing synopsis
    24  replace this with something better [fixme]
    39  move somewhere else [fixme]
   220  should we allow multiple package DBs (e.g. with 'inherit')?
   246  Instead of modifying the global process state, it'd be better to
        set the environment individually for each subprocess invocation.
        This will have to wait until the Shell monad is implemented;
        without it the required changes are too intrusive.
   379  path canonicalisation is done in addBuildTreeRefs, but we do it
        twice because of the timestamps file. [fixme]
   599  use a better error message, remove duplication.
   742  Is this compatible with the 'inherit' feature? [fixme]
   748  configPackageDB' and configCompilerAux' don't really belong in this
        module [fixme]
   762  make configCompilerAux use a sensible verbosity [fixme]
   104  Move this to D.C.Utils?
   175  return only the refs that vere actually removed. [fixme]
   180  removing snapshot deps is done with `delete-source
        .cabal-sandbox/snapshots/$SNAPSHOT_NAME`. Perhaps we also want to
        support removing snapshots by providing the original path. [fixme]
    84  would be nice to remove duplication between
        D.C.Sandbox.PackageEnvironment and D.C.Config.
   148  Currently, we follow cabal-dev and set 'user-install: False' in the
        config file. In the future we may want to distinguish between
        global, sandbox and user install types.
   334  Use substPathTemplate with compilerTemplateEnv ++
        platformTemplateEnv ++ abiTemplateEnv.
   342  Also check for an initialised package DB?
   411  Should we make these fields part of ~/.cabal/config ? [fixme]
    71  We should keep this info in the index file, together with build
        tree refs. [fixme]
   214  This function is not thread-safe because of 'inDir'. [fixme]
   261  What if the clock jumps backwards at any point? For now we only
        print a warning. [fixme]
    44  stop exporting these:
  1456  remove when "cabal install" avoids
  1918  this should be an 'add-source'-only flag. [fixme]
  2058  this is too GHC-focused for my liking..
  2193  Disabled for now because it does not work as advertised (yet).
  2223  do we want to allow per-package flags?
   129  use runProgramInvocation, but has to be able to set CWD
   798  check integer widths, eg for large file sizes
   416  should we warn if there are no world targets?
   716  use Text instance for FlagName and FlagAssignment [fixme]
   114  I wonder if it would make sense to promote this datatype to Cabal
        and use it consistently instead of InstalledPackageIds?
    38  how do we find this path for an arbitrary hackage server? is it
        always at some fixed location relative to the server root? [fixme]
    59  better error message when no repos are given [fixme]
   660  It'd be nice if 'cabal install' picked up the '-w' flag passed to
        'configure' when run inside a sandbox. Right now, running
   683  Redesign ProgramDB API to prevent such problems as #2241 in the
   703  Passing 'SandboxPackageInfo' to install unconditionally here means
        that 'cabal install some-package' inside a sandbox will sometimes
        reinstall modified add-source deps, even if they are not among the
        dependencies of 'some-package'. This can also prevent packages that
        depend on older versions of add-source'd packages from building
        (see #1362). [fixme]
    45  Test this against a package installed in the sandbox but not
        depended upon. [xxx]
     3  This module was originally based on the PackageTests.PackageTester
        module in Cabal, however it has a few differences. I suspect that
        as this module ages the two modules will diverge further. As such,
        I have not attempted to merge them into a single module nor to
        extract a common module from them. Refactor this module and/or
        Cabal's PackageTests.PackageTester to remove commonality.
        2014-05-15 Ben Armston
   229  Convert to a "-v" flag instead.
    55  Perhaps these should be made comments of the corresponding data
        type definitions. For now these are just my own conclusions and may
        be wrong.

More information about the cabal-devel mailing list