Calculate IPID from source before building #2752

Closed
wants to merge 6,040 commits into
from

Projects

None yet
@vagrawal
vagrawal commented Aug 6, 2015

This patch allows cabal to calculate IPID before building the package. The benefits to this method is that we can stop a duplicate compilation by looking at IPID and that IPID generated by this method is unique and changes with source (and also #2745, so now it changes only installation IPID and not inplace IPID when doing cabal build). Now this patch generates IPID strictly from source(by calculating hash of sdist), but there can still be mutations in compiled package without changing the source. It is not immediately clear as which thing is best to include in the hash but there are some candidates:

  • Command line flags + InstallPlan
  • Command line flags + InstallPlan + ProgramConfiguration
  • LocalBuildInfo (moving IPID calculation to after configuration step)
  • LocalBuildInfo + ConfigEx Flags + Install Flags

Also I had done some work to change the library path to IPID(not very hard), but then found out a recent change by @ezyang having a more broad plan including backpack, so I have reverted but I think it is a better way to go(and it will bring one step closer to #2745). Please comment if I can replace it. I can also do locking before building part as suggested by @ttuegel. Please let me know if it is a good way to go as @dcoutts said that it is not critical and can be left for now.

edsko and others added some commits Mar 6, 2015
@edsko edsko Actually _use_ setup deps in configure and co
The only problematic thing is that when we call `cabal clean` or `cabal
haddock` (and possibly others), _without_ first having called `configure`, we
attempt to build the setup script without calling the solver at all. This means
that if you do, say,

    cabal configure
    cabal clean
    cabal clean

for a package with a custom setup script that really needs setup dependencies
(for instance, because there are two versions of Cabal in the global package DB
and the setup script needs the _older_ one), then first call to `clean` will
succeed, but the second call will fail because we will try to build the setup
script without the solver and that will fail.
750523e
@edsko edsko Unit tests for setup dependencies 61d961a
@edsko edsko Abstract out qualification of goals
This happened independently in a number of places, which was bad; and was about
to get worse with the base 3/4 thing.
082dcdd
@edsko edsko Better implementation of qualifyDeps
Never consider flag choices as independent from their package.
03244d6
@edsko edsko Treat base special in goal qualification 3685452
@edsko edsko Only qualify base if a base shim is present 67c503a
@edsko edsko Unit tests for dealing with base shims 112e5e2
@edsko edsko Make the modular solver the default always
(previously the default was the topdown solver for GHC < 7). Also adds a
deprecation warning when the topdown solver is selected.
d0ecedc
@cdepillabout cdepillabout Changed Main.hs to output "Hello, Haskell!" instead of "Hello, World!". 17adeeb
@freizl freizl Correct the link - package version policy. d938778
@23Skidoo 23Skidoo Merge pull request #2528 from freizl/master
Correct the link - package version policy.
a485f0a
@cheecheeo cheecheeo cabal check will fail on -fprof-auto passed as a ghc-option
This is consistent with the the current -auto-all behavior.

This fixes #2479.
82f77c7
@23Skidoo 23Skidoo Merge pull request #2532 from cheecheeo/2479_fprof_auto
cabal check will fail on -fprof-auto passed as a ghc-option
ca1b6f5
Matthias Pronk slightly better error message in case of backjump limit reached bf7b271
@zadarnowski zadarnowski Fixed bash completion for sandbox subcommands 5e3c7ca
@23Skidoo 23Skidoo Merge pull request #2537 from zadarnowski/master
Fixed bash completion for sandbox subcommands
133198f
@23Skidoo 23Skidoo Merge pull request #2536 from mjepronk/master
Slightly better error message in case of backjump limit reached
33a80be
@freizl freizl Move SrcDirs following Main file generator. c9078da
@jtdaugherty jtdaugherty bootstrap.sh: fixes linker matching to avoid cases where tested linke…
…r names appear unexpectedly in compiler output (fixes #2542)

This fixes cases where strings like "ld" appear in compiler output on lines
that have nothing to do with the linker command invocation.
36c2754
@23Skidoo 23Skidoo Merge pull request #2543 from GaloisInc/master
bootstrap.sh: fixes linker matching to avoid cases where tested linker names appear unexpectedly in compiler output (fixes #2542)
15786f8
@geoff-codes geoff-codes Rewrite linker check to avoid grepping "ld"s.
I was the one who introduced that bit, and it seems like well may have caused more trouble than it helped... so, sorry :(

Anyway, clang has since then grown the ability to `-print-prog-name`, so I've done away with all that grepping through -### stuff.
@creswick @23skidoo, could you see if this works for you?
af39e93
@geoff-codes geoff-codes Add a missing warning guard. 0023da1
@23Skidoo 23Skidoo Merge pull request #2550 from geoff-codes/patch-1
Rewrite linker check to avoid grepping "ld"s.
aee7b4f
@NathanHowell NathanHowell Add Functor, Foldable and Traversable instances to Condition ea46a29
@23Skidoo 23Skidoo Merge pull request #2551 from NathanHowell/traversable-condition
Add Functor, Foldable and Traversable instances to Condition
87986e3
@23Skidoo 23Skidoo Paths_ module: don't require base >= 4.
Fixes #994.
eb450ef
@23Skidoo 23Skidoo Merge pull request #2552 from 23Skidoo/issue-994
Paths_ module: don't require base >= 4.
3e319f8
@23Skidoo 23Skidoo Revert "Revert "Print a more friendly message when http_proxy is down.""
This reverts commit c9a7735.
2724aaf
@23Skidoo 23Skidoo Wording. fa9aed9
@23Skidoo 23Skidoo Merge pull request #2560 from 23Skidoo/issue-1962
Print a more friendly message when http_proxy is down.
9d8edff
@dcoutts dcoutts Force cabal upload to always use digest auth and never basic auth 1d315eb
@23Skidoo 23Skidoo Implement a dummy 'uninstall' command.
As suggested by @sclv on reddit. See
haskell#648 (comment)
13562e9
@byorgey byorgey Merge pull request #2483 from cdepillabout/cabal-install-create-main-hs
Make `cabal init` create Main.hs if it doesn't exist.
a7a540f
@23Skidoo 23Skidoo Doc fix: 'cabal freeze PACKAGES' is not supported.
Fixes #2565.
51b0244
@23Skidoo 23Skidoo Merge pull request #2567 from haskell/23Skidoo-patch-1
Doc fix: 'cabal freeze PACKAGES' is not supported.
7205817
@23Skidoo 23Skidoo Merge pull request #2564 from 23Skidoo/uninstall-dummy
Implement a dummy 'uninstall' command.
0a732be
@23Skidoo 23Skidoo Merge pull request #2541 from freizl/master
Move SrcDirs following Main file generator.
97fed33
@ezyang ezyang Document more functions in Paths_pkgname, fixes #715
Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
000e814
@23Skidoo 23Skidoo Merge pull request #2570 from ezyang/cabal-715
Document more functions in Paths_pkgname, fixes #715
f9ef1ff
@23Skidoo 23Skidoo Merge pull request #2563 from dcoutts/master
Force cabal upload to always use digest auth and never basic auth
6352e32
@23Skidoo 23Skidoo -Wall police. 3ea65d4
@23Skidoo 23Skidoo -Wall police. 128c7af
@bergmark bergmark Use PVP-compliant example when warning about missing base upper bound.
Refs hackage-server#253.
5435394
@23Skidoo 23Skidoo Merge pull request #2574 from bergmark/pvp-base-warning
Use PVP-compliant example when warning about missing base upper bound
c3341bc
@23Skidoo @23Skidoo 23Skidoo Implement offline mode for the 'install' command.
When in offline mode, 'cabal install' only installs packages from the local
tarball cache. Offline mode can be enabled with the '--offline' flag.

Fixes #2566.
e7a9e5e
@23Skidoo 23Skidoo Update changelog.
[ci skip]
0f0f116
@23Skidoo 23Skidoo Tweak message. 20b9af9
@23Skidoo 23Skidoo Small simplifaction. 4a78e46
@dcoutts dcoutts Allow -Werror and similar when guarded by a manual flag
Improve the package description check for -Werror and similar flags so
that they are allowed so long as they're not enabled by default. We
check the conditions that they're guarded by to make sure they're always
false (given the default values of the manual flags). Also extend the
error messages to explain how to use these flags with conditionals.
d952223
@dcoutts dcoutts Improve the -fhpc check warning message 2315464
@dcoutts dcoutts Make the -threaded flag check a build warning
The -threaded flag really makes no difference for libraries, it's a
link-time option only.
b409e5e
@dcoutts dcoutts -rtsopts and -with-rtsopts also have not effect in libraries ae8b253
@dcoutts dcoutts Fix warnings 2543f23
@dcoutts dcoutts Improve the -auto-all -fprof-auto warning message
And extend it to cover other similar variants. People often put it in to
help them profile *this* package, but it causes problems for profiling
other packages. The -fprof-auto-exported is arguably ok though.
447f112
@dcoutts dcoutts Extend ghc-options checks to ghc-prof-options
Currently there are no checks that need to be specific to one of them.
Both sets of options get used in distributed packages.
548d444
@23Skidoo 23Skidoo Merge pull request #2594 from dcoutts/master
Improve a number of ghc-options checks
6400ba5
@23Skidoo 23Skidoo Fix warnings. b0cbede
@dcoutts dcoutts Merge pull request #2578 from 23Skidoo/offline-mode
Implement offline mode for the 'install' command.
a256ad9
@dcoutts dcoutts Handle multiple preferred-versions in the index tarball better
The existing code supports reading multiple preferred-versions files in
the 00-index.tar and merging them. However it doesn't do it quite right
when the same file is updated, it merged them instead of the later one
overriding the first.

This should make no difference right now because the 00-index.tar
typically only contains a single preferred-versions file, with no
updates.
1490420
@qnikst qnikst Provide cabal_macros in c2hs invocation.
This commit includes cabal_macros in c2hs call, so it will
be possible to use MIN_VERSION macros in chs files.
aa25978
@23Skidoo 23Skidoo Merge pull request #2600 from qnikst/c2hs-cabal-macros
Provide cabal_macros in c2hs invocation.
4ae7f56
@23Skidoo 23Skidoo Update changelog
[ci skip]
6e6e678
@dcoutts dcoutts Merge pull request #2515 from edsko/pr/setup-deps
Introduce "setup dependencies"
b3633ca
@dcoutts dcoutts Merge pull request #2530 from edsko/pr/base-shim
Add support for base shims to the modular solver
c272908
@dcoutts dcoutts Merge pull request #2531 from edsko/pr/switch-default
Make the modular solver the default always, and deprecate the top-down solver
ca5f1ed
@lukexi lukexi Don't recompile C files unless needed ccebdc3
@23Skidoo 23Skidoo Merge pull request #2601 from lukexi/master
Don't recompile C files unless needed
04f205d
@23Skidoo 23Skidoo Update changelog
[ci skip]
9914f02
@23Skidoo 23Skidoo Update changelog
[ci skip]
760c82c
@triplepointfive triplepointfive Fix sandbox delete-source error message typo
\a is considered as a bell character, which is omitted as a result
2b48003
@23Skidoo 23Skidoo Merge pull request #2603 from triplepointfive/delete-source-typo
Fix sandbox delete-source error message typo
d59cd84
@AngusL AngusL Document 'extensions' field replacements
Resolves #2401.
d6c8f86
@23Skidoo 23Skidoo Merge pull request #2609 from AngusL/master
Document 'extensions' field replacements
05d45e3
@mtolly mtolly Automatically configure when running haddock
Fixes #2275
9321e88
@phadej phadej Implement #2366. More Condition instances c7cfe22
@phadej phadej Fix #2598. Omit empty CondTree branches a94ac64
@dcoutts dcoutts Merge pull request #2624 from phadej/pp-cond-tree-branches
Fix #2598. Omit empty CondTree branches
cb8be1d
@nomeata nomeata Only set rpath on dynamic executables
not static ones. They make no sense there, increase startup time and
may violate some distributions policy on rpaths. This closes #2625.
a82e8fb
@fisx fisx Support new section syntax for remote-repo. 1638195
@fisx fisx Gardening. f7c1ccf
@phadej phadej Resolve #2593: Lift global database restrictions 9699f1a
@fisx fisx Render remote repos into sections, not fields. 7981265
@phadej phadej Fix #367. Find module files in all source directories. 612bc18
@fisx fisx Empty lines between several repos. fc0cf9e
@phadej phadej Make cabal check warn about BOM
Implements first bullet point of #2573
1e74906
@phadej phadej Ignore bom when reading cabal files from index tarball fca1357
@23Skidoo 23Skidoo Merge pull request #2626 from nomeata/master
Only set rpath on dynamic executables
0393bfa
@edsko edsko Explain what linking is e75051f
@edsko edsko Separate module for modular solver DSL c2cabcf
@edsko edsko Introduce and use innM. 693437d
@edsko edsko Document addLinking 088bb3f
@edsko edsko Call 'preferLinked' as the _first_ heuristic 93e85a0
@edsko edsko Use P-setup rather P.setup for setup deps of P
(in the solver log)

Addresses haskell#2500 (comment)
8b67793
@edsko edsko Move varPI to where Var is defined. a2cf42e
@edsko edsko Add comment for LinkGroup 61f001a
@edsko edsko Remove redundant TODO 0c53cf7
@dcoutts dcoutts Merge pull request #2621 from phadej/cabal-check-bom
Make cabal check warn about BOM
491d601
@dcoutts dcoutts Merge pull request #2618 from phadej/condition-monad
Resolve #2366. More Condition instances
367d97c
@edsko edsko Merge lgCanon and lgInstance
We previously recorded the canonical package and the instance of a link group
separately, but it turns out we can always record these together: when we know
one we know the other. This makes various choices a lot clearer.

Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
Addresses haskell#2500 (comment)
3585c29
@edsko edsko Add comment to dependencyInconsistencies e1e2b49
dcoutts and others added some commits Jul 21, 2015
@dcoutts @23Skidoo dcoutts Move PackageFixedDeps class to Types module
It was in the PackageIndex module but it was unused there and it
being there was just confusing.
9962d34
@dcoutts @23Skidoo dcoutts Move the ConfiguredPackage validation out of the InstallPlan module
The InstallPlan can be generalised by abstracting over the specific
package types. The only thing that really relies on a lot of the
details of the concrete ConfiguredPackage type is the bit that
validates them individually (as opposed to validating packages
within the plan in relation to other packages, graph structure etc).

So as a prelude to generalising the InstallPlan, move the checks
on the ConfiguredPackage into the Dependency module and use them
when checking the output of the solver.
888479a
@dcoutts @23Skidoo dcoutts Introduce a ResolverPackage type for the result of the solvers
Rather than directly reusing the InstallPlan.PlanPackage type which has
more cases and which we'd like to generalise somewhat.
7b18793
@dcoutts @23Skidoo dcoutts Remove a couple uses of the InstalledPackage wrapper type
Removed from the InstallPlan and solver interface.
The InstalledPackage is really just there for the benefit of the old
TopDown solver which needs to know source deps.
d4e2c79
@dcoutts @23Skidoo dcoutts Localise the InstalledPackage wrapper type to the old TopDown solver
No longer needed anywhere else.
55dde30
@dcoutts @23Skidoo dcoutts Move the ExtDependency type to the only module where it's used
Fewer shared types makes the code easier to grok and navigate.
3523bf3
@dcoutts @23Skidoo dcoutts Parameterise InstallPlan by its package types
So rather than the concrete InstalledPackageInfo and ConfiguredPackage,
the InstallPlan and PlanPackage are parameterised by the type of the
installed package, the source package and the types used for successful
and unsucessful build results.
0bacb64
@dcoutts @23Skidoo dcoutts Remove the now-unused Platform and CompilerInfo from the InstallPlan
It wasn't used within the InstallPlan, but it had accessors and those
were used in a few places. Just pass them into those few places that
need it.
143b696
@23Skidoo 23Skidoo Whitespace. efbf171
@23Skidoo 23Skidoo Make type signatures less verbose. 060bbf7
@23Skidoo 23Skidoo Whitespace, 80-col violations. e1b8358
@23Skidoo 23Skidoo Changelog update. 702c3b1
@randen randen Propose code for supporting haddock response files
This change is for adding the ehancement to support haddock
response files.  This is what was used to build HP 7.10.2
for Windows, but it does need a corresponding change in
haddock to utilize it. Hopefully, this and its corresponding
haddock change can make it into the next ghc release and
eliminate much gnashing of teeth, error prone, and time
consuming manual steps in the HP build process.

Modified:

* Cabal/Distribution/Simple/Haddock.hs
  * renderArgs function: I put a couple of things into locals
    since I needed another use for UTF8 support check, plus I
    added another check based on version; the temp file logic
    was just as the prologue case above but I did need to
    repeat the invocation of the 'k' function in order to
    keep the cases separate and allow proper handling of the
    temp file automatic (or not, per --keep-temp-files)
    deletion.  Important: the haddock version being check
    against for response file support, greater than 2.16.1,
    is a placeholder and may or may not be the actual value
    since that will depend on the as-yet-unreleased haddock
    (which looks like it may be >2.16.1 but at this moment is
    not released).
8af90e1
@23Skidoo 23Skidoo Merge pull request #2746 from randen/haddock2
Support haddock response files
4c60c7b
@23Skidoo 23Skidoo Merge pull request #2738 from 23Skidoo/refactoring-installplan
Refactoring installplan
5f2228c
@23Skidoo 23Skidoo Changelog update. 38cd1fd
@grayjay grayjay Improve error message for unsatisfiable package constraints (issue #2643
)

This commit adds the sources of constraints to debugging and error messages,
e.g., "main config file" or "command line flag".
5ef2dc4
@grayjay grayjay Improve constraint error messages and refactor after code review 06970d7
@23Skidoo 23Skidoo Merge pull request #2727 from grayjay/track-package-constraints
Improve error message for unsatisfiable package constraints (issue #2643)
900ecca
@23Skidoo 23Skidoo Changelog update. fae7b04
@23Skidoo 23Skidoo Rename some functions.
Rename a bunch of functions of type 'Foo -> String' from 'debugFoo' to
'showFoo'.
c82ad56
@christiaanb christiaanb Add frameworks when linking a dynamic library
This fixes the Cabal side of [GHC trac #10568](https://ghc.haskell.org/trac/ghc/ticket/10568)
b6b680f
@23Skidoo 23Skidoo Merge pull request #2747 from christiaanb/patch-2
Add frameworks when linking a dynamic library
6ed2c17
@anton-dessiatov anton-dessiatov Fix windows build after changes introduced by 8ea6f33 98e4cc1
@23Skidoo 23Skidoo Merge pull request #2750 from anton-dessiatov/master
Fix windows build
7fab900
@23Skidoo 23Skidoo Revert "Merge pull request #2516 from anton-dessiatov/master"
This reverts commit f5020d4, reversing
changes made to 8d1cc52.
e885c62
@ezyang
Contributor
ezyang commented Aug 6, 2015

Keep trying to fix Travis :) Rebase your patches together when you finish!

@ezyang
Contributor
ezyang commented Aug 6, 2015

@fugyk If you would move us towards #2745 (getting rid of LibraryName in the process) that would be stellar!

Thomas Tuegel and others added some commits Mar 29, 2015
Thomas Tuegel reorganize exitcode-stdio-1.0 tests 4fd8162
Thomas Tuegel add deadlock test for detailed-0.9 test suites e1bee5e
Thomas Tuegel remove PackageTests/BuildTestSuiteDetailedV09 fdb1897
Thomas Tuegel fix deadlock in detailed-0.9 test suite runner
Fixes #2489. The detailed-0.9 test suite runner would deadlock because
the IO manager was not draining the output pipe of the child
process. Thanks to @r0ml for the patch.
b642ec5
Thomas Tuegel ignore dist-* from test suite tests ad55287
Thomas Tuegel Merge pull request #2508 from ttuegel/test-deadlock
Fix deadlock in detailed-0.9 test suites
c57d272
@Yuras Yuras Disable HPC in REPL for GHC 7c1f312
@23Skidoo 23Skidoo Merge pull request #2758 from Yuras/hpc-in-ghci
Disable HPC in REPL for GHC
055c4c5
@grayjay grayjay Update documentation for test-only packages 4bbbed7
Thomas Tuegel Merge pull request #2765 from grayjay/test-documentation
Update documentation for test-only packages
28dfb90
@christiaanb christiaanb Only include dynlink opts when building a dynamic executable
Fixes #2766
7f2746b
@ezyang ezyang Eliminate InstalledPackageInfo module name parameter.
This parameter was a legacy when GHC depended directly on
Cabal; this is no longer the case so there is no need for
this parameter (which is unused inside Cabal.)

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
5d1f9f7
@23Skidoo 23Skidoo Merge pull request #2767 from christiaanb/patch-3
Only include dynlink opts when building a dynamic executable
e020387
@ezyang ezyang Eliminate InstalledPackageInfo module name parameter.
This parameter was a legacy when GHC depended directly on
Cabal; this is no longer the case so there is no need for
this parameter (which is unused inside Cabal.)

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
3a6ba5b
@23Skidoo 23Skidoo Merge pull request #2770 from ezyang/cabal-ipi-no-parameter
Eliminate InstalledPackageInfo module name parameter.
c451821
@23Skidoo 23Skidoo Merge pull request #2617 from phadej/no-global-db
Resolve #2593: Lift global database restrictions
295f266
@grayjay grayjay Warn if 'license-file' is absolute or outside of source tree (fixes #… 3d2e799
@23Skidoo 23Skidoo Merge pull request #2773 from grayjay/check-license-files
Warn if 'license-file' is absolute or outside of source tree (fixes #2742)
ca82f98
@edsko edsko For ghc >= 7.10 use displayException in topHandler
(Ideally this would check the version of base instead, but that's not really
possible with bootstrapping Cabal; checking the version of ghc instead is what
happens in lots of other places so sticking with that for now.)
a052df8
@edsko edsko Rethrow async exceptions 3cef8b6
@dcoutts dcoutts Merge pull request #2781 from edsko/pr/use-displayException
For ghc >= 7.10 use displayException in topHandler
f8709c1
@BardurArantsson BardurArantsson Remove dead code
Only the NoComments case of IncludeComments was ever used and we can
simplify the generation of the default "package environment file"
accordingly.
8c48517
@23Skidoo 23Skidoo Merge pull request #2785 from BardurArantsson/dead-code-1
Remove dead code
df27eed
@ezyang
Contributor
ezyang commented Aug 20, 2015

You should squash the two patches.

@ezyang
Contributor
ezyang commented Aug 20, 2015

It looks good, my only quibbles are on the actual calculation of the hash.

@ezyang
Contributor
ezyang commented Aug 24, 2015

Hey @fugyk, are you still planning on adding the sanity checks, where we recompute the hash before registering/copying to ensure that the build products match the actual IPID (so someone doesn't edit and then install with the wrong IPID).

@ezyang ezyang added a commit to ezyang/cabal that referenced this pull request Aug 24, 2015
@ezyang ezyang Support --ipid flag for explicitly passing IPID in.
This flag (through a terrible hack) also supports install
directory variables (e.g. $pkgver).  Depends on PR #2752.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
f8241e2
@ezyang ezyang referenced this pull request Aug 24, 2015
Closed

Cabal IPID flags #2791

@vagrawal

@ezyang I had almost done that by recalculating hash at register step as mentioned in the message but then decided it won't be enough as it suffers from ABA-like problem. I think doing it by timestamps will be better. Also timestamp and contents must be accessed from same syscall to make IPID calculation atomic.

But I am afraid I can't do it very soon. Sorry for that. I am working hard to finish this GSoC for many days. I think I can finish it by today and after that I need to take a little break. I can do it after 2-3 days if you wish.

@ezyang
Contributor
ezyang commented Aug 24, 2015

It's OK; I think the patch as it stands can go in and we can file the other functionality as a feature request. It might also be possible to put in the simple version, which is susceptible to ABA, and then file a bug report to fix this problem (I think in practice people are unlikely to have this problem.)

@ttuegel
Member
ttuegel commented Aug 24, 2015

I had almost done that by recalculating hash at register step as mentioned in the message but then decided it won't be enough as it suffers from ABA-like problem.

Of course it does, that's why I indicated we would need to copy the source to a (secure-ish) temporary directory and install from there. That's a separate, pre-existing issue which I need to fix (for other reasons).

Also timestamp and contents must be accessed from same syscall to make IPID calculation atomic.

I don't know if that's possible on any platform, but I'm pretty confident there's no cross-platform way to do it. Anyway, that's actually not sufficient; GHC would also need to access the contents from the same system call. (The system clock, and therefore the timestamp, are not monotonic.)

It's OK; I think the patch as it stands can go in and we can file the other functionality as a feature request. It might also be possible to put in the simple version, which is susceptible to ABA, and then file a bug report to fix this problem (I think in practice people are unlikely to have this problem.)

While the problem may be unlikely, it is also hard to detect and hard to fix. I don't think this should go into master without the install-from-copy feature, but this is a known limitation which we have discussed before.

@ezyang
Contributor
ezyang commented Aug 24, 2015

While the problem may be unlikely, it is also hard to detect and hard to fix. I don't think this should go into master without the install-from-copy feature, but this is a known limitation which we have discussed before.

There are two cases, one of which is very obvious:

  1. A user is not using cabal-install. In this case, it's very obvious if the IPID is bogus: later, they try to install the same IPID and Cabal complains that this IPID is already present in the installed package database. (Granted, this may be a bit fiddly to FIX; the user has to excise the package and anyone who depends on it. But it's at least obvious there's a problem)
  2. A user is using cabal-install. In this case, cabal-install may silently decide not to reinstall a package because it's already present. But this doesn't really seem all that harmful: this would mean that the SAME VERSION of the package (with a different actual sdist) was already installed. If the user has already shot themselves in the foot this much, I don't think it's our responsibility to deal with it.
@vagrawal

we would need to copy the source to a (secure-ish) temporary directory and install from there

@ttuegel What about just building by changing the source. It will need a workflow change of configuring every time if we copy source in configure step and build from there.

@ttuegel
Member
ttuegel commented Aug 24, 2015

What about just building by changing the source. It will need a workflow change of configuring every time if we copy source in configure step and build from there.

@fugyk There will be no workflow change because we will copy the source during install. Plain in-tree builds don't need to have a continuously-updated IPID; it's only a problem when you go to install them. As @ezyang and I discussed previously, having a continuously-updated IPID for in-tree builds is actually harmful.

@ezyang
Contributor
ezyang commented Aug 24, 2015

Well, I think @fugyk is concerned about this ABA problem: configure, modify the source, build the package, modify the source BACK, and then attempt an install. Under most IPID sanity checks, this will go through, even though the build products are not for the IPID we have.

@ttuegel
Member
ttuegel commented Aug 24, 2015

Well, I think @fugyk is concerned about this ABA problem: configure, modify the source, build the package, modify the source BACK, and then attempt an install. Under most IPID sanity checks, this will go through, even though the build products are not for the IPID we have.

Right, I understand. The install phase needs to copy the source, re-configure, re-build, and finally install. That sequence ensures that the build products match the IPID, and I think that's the only practical way to make that assurance.

@vagrawal

The install phase needs to copy the source, re-configure, re-build, and finally install

That's cabal(-install) install. We can't leave setup configure, build, copy, register workflow(EDIT- from as much you guys told me. My earlier patch attempted to do that). What about that?

@ezyang
Contributor
ezyang commented Aug 24, 2015

I mean, I'm OK with disabling register/copy from manual Setup calls unless you say "no really I know what I'm doing". But it still needs to be possible, so that tools like cabal-install can do it.

@vagrawal

I'm OK with disabling register/copy from manual Setup calls

Or I am in favour of your middle ground solution that if we don't pass IPID in configure step, cabal register will register with inplace IPID.

@ttuegel
Member
ttuegel commented Aug 24, 2015

That's cabal(-install) install. We can't leave setup configure, build, copy, register workflow(EDIT- from as much you guys told me. My earlier patch attempted to do that). What about that?

The only people who use ./Setup copy and ./Setup register are source-build distributions and cabal-install. For backward compatibility, we keep those commands, but they can emit a warning that you better know what you're doing. As for cabal-install, we either give a mechanism to silence the warning, or (better) have it use the new Cabal install mechanism, since it subsumes the required functionality.

Or I am in favour of your middle ground solution that if we don't pass IPID in configure step, cabal register will register with inplace IPID.

Can we register in-place IPIDs in the global database?

@vagrawal

The only people who use ./Setup copy and ./Setup register are source-build distributions and cabal-install

I guess mainly it's used for bootstraping. AFAIK GHC uses it in some places.

Can we register in-place IPIDs in the global database?

Normally people don't do setup register I guess. For them we can do inplace register. (Q: What about tools like stack?)

@ezyang ezyang added a commit to ezyang/cabal that referenced this pull request Aug 25, 2015
@ezyang ezyang Support --ipid flag for explicitly passing IPID in.
This flag (through a terrible hack) also supports install
directory variables (e.g. $pkgver).  Depends on PR #2752.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
e34bfc4
@ezyang
Contributor
ezyang commented Aug 25, 2015

OK, a few more problems which I've noticed when testing this revision with GHC:

  • We should track the "optimization level" to make it possible for someone to install a library multiple times with --disable-optimisation and --enable-optimisation (we have a test case for this in GHC, and optimisation level DEFINITELY affects ABI).
  • We're not erroring if you 'Setup register' a package into a database that already has this package; we definitely should error by default.

I can contribute fixes for these.

@ezyang ezyang added a commit to ezyang/cabal that referenced this pull request Aug 25, 2015
@ezyang ezyang Support --ipid flag for explicitly passing IPID in.
This flag (through a terrible hack) also supports install
directory variables (e.g. $pkgver).  Depends on PR #2752.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
77f55e6
@ezyang
Contributor
ezyang commented Aug 25, 2015

Another thing todo: need to make it possible for cabal-install to get the IPID. (For build paths and stuff.) I think I'm going to go ahead and implement this.

UPDATE: This doesn't seem to be critical path, cabal-install seems to work fine without the IPID.

@ezyang
Contributor
ezyang commented Aug 27, 2015

We're not erroring if you 'Setup register' a package into a database that already has this package; we definitely should error by default.

So, the easiest way to fix this is to change our invocation of 'ghc-pkg' from 'reregister' to 'register'. But this means we have to be careful to unregister a package from the inplace database before attempting to register it again.

While implementing this, I noticed that the UniqueIPID test was a bit bad: it was registering in the GLOBAL user database (it's passing the --inplace flag but this does not do what the author thought it does.) I also have a patch to fix this.

@ezyang
Contributor
ezyang commented Aug 27, 2015

Currently, this patch applies new IPID calculation to ALL versions of GHC (it's not feature gated.) This is desirable because it means that Cabal doesn't have to maintain two feature paths (sdist computed IPIDs and ABI hash IPIDs) and we can simplify code accordingly. But there are BC considerations...

GHC 7.8 and earlier, prior to this patch. There's no support for multiple instances in the package database; so we want ghc-pkg to complain if we install the same package name/version pair multiple times. The current behavior is that './Setup registeron a package ALWAYS works (by invokingghc-pkg update), and it will delete any installed packages that have the same package id (package name and version). This means that './Setup register' may silently break packages in your package database, and./Setup copy` may silently clobber files for an already existing package in your database. If you use cabal-install, Cabal will ALWAYS refuse to reinstall. If you force a reinstall, and are lucky and the ABIs are the same, nothing will be broken; if you do force a reinstall, some number of packages will become unavailable. One thing to note is that it is fairly difficult to end up with a segfaulting set of packages: the easiest way to cause a segfault is to './Setup copy' (clobbering interface/object files) without subsequently './Setup register'ing.

GHC 7.8 and earlier, after this patch. Let me first describe the state from this literal patch, then describe some possible alternative worlds we can live in. With this patch, ./Setup register and ./Setup copy can now cause a segfault whenever a pair of matching IPIDs doesn't imply a compatible API: this is because we invoke ghc-pkg update which will just overwrite an existing with the same IPID without invalidating any preexisting packages which depended on it (this is akin to the "being lucky and the ABI not changing" case.) GHC also (internally) assumes that IPIDs imply ABI compatibility (and will use this property to de-duplicate packages which show up in multiple package databases) and has no way of detecting this situation. If you force a Cabal reinstall, similar admonitions apply.

Now for alternatives. Earlier this thread I proposed that we refuse to register a package unless you --force if the IPID already existed. However, there is no direct ghc-pkg command which actually has this effect: ghc-pkg register will refuse to install if there is another package with the same package ID (package name + package version, reporting "package foo-0.1 is already installed"), ghc-pkg update will happily overwrite an existing IPID, and ghc-pkg register --multi-instance doesn't exist prior to 7.10. So what we would have to do is call ghc-pkg once before the registration to check for our IPID, and then register it. (Non-atomic, so there is a race condition.) And of course, we can't update ghc-pkg because it ships with GHC.

Another possibility is to just not bother; @ttuegle has suggested that ./Setup register and ./Setup copy should unconditionally emit warnings to make sure the user knows what they are doing (although, IMO, a warning but go ahead is kind of pointless because you may have already messed up your package database at that point.)

GHC 7.10, prior to this patch. Primary difference: Cabal computes a "package key", which is then passed to GHC for compilation to be used for symbol names and type equality. The default behavior of ghc-pkg is the same as pre-GHC 7.8, but there is a flag --enable-multi-instance which applies different behavior: ghc-pkg will no longer fail if there is a package already in the database with a matching package ID, as long as there is not something in the database with the same IPID. However, there is a BUG in the implementation, which causes it to raise spurious case-sensitivity errors: https://phabricator.haskell.org/D1177

GHC 7.10, after this patch. Well, technically this patch doesn't do it but the companion patch #2748 enables multi instances. With these two patches, it means that if we register an existing IPID, we will overwrite it, but otherwise if it doesn't already exist, it will just be added to the database (without deleting any existing packages).

As a refinement, if we want to prevent overwriting an existing IPID, we can just change our invocation of ghc-pkg update to ghc-pkg register. The non-atomic GHC 7.8 implementation will also work in this case, however.

My recommendation. It seems most uniform if we continue to use ghc-pkg update to register our packages, but to add an extra check to see if the exact IPID already exists in the database. We fail out in that case, but it can be overridden by passing a new --force flag to cabal register. Furthermore, we pass --enable-multi-instance to ghc-pkg whenever we can.

UPDATE: One complication: GHC 7.6's ghc-pkg does not support the --ipid flag, so you have to implement this indirectly by querying for the package name + version, and then checking if the returned IPID matches.

@ezyang
Contributor
ezyang commented Aug 27, 2015

OK implemented here #2792

@ezyang
Contributor
ezyang commented Aug 28, 2015

OK, another major bug: cabal-install needs to get the IPID so that it can compute the install directory correctly.

@ezyang
Contributor
ezyang commented Aug 29, 2015

Thought about this some more, as before there are delicate backwards compatibility considerations, stemming from the fact that a './Setup' executable (in the case of Custom build type) can be compiled with an older version of Cabal.

The ideal situation is that the installation directory is based off of the installed package ID. OK, but how do we get the IPID from the configure step where it is currently computed? There are two options:

  1. cabal-install computes it directly (using it's version of Cabal), or
  2. cabal-install gets it from calling Setup configure and having configure somehow report the IPID.

In fact, it seems that we'll have to do a combination of both. Since there's no support for getting an IPID from Setup configure with old versions of Cabal, in that case we are forced to use method (1) to get an IPID. (In fact, it may not have any relation to the IPID that register might end up registering the package as; if it really is an old version of Cabal, it will be installed with the ABI. But it is harmless if the IPID is our calculated one.)

EDIT: But method (2) doesn't work, because the library directory is configured during configure (which is when the IPID is calculated). So if cabal-install is always computing the IPID directly, it should also pass it to Cabal when possible, using the --ipid flag.

@23Skidoo
Member

Thought about this some more, as before there are delicate backwards compatibility considerations, stemming from the fact that a './Setup' executable (in the case of Custom build type) can be compiled with an older version of Cabal.

Well, you can require it to be compiled against a recent version. We do this for --allow-newer. The downside is that the resulting error can be surprising for users.

@ezyang
Contributor
ezyang commented Aug 29, 2015

Well, this code is going to apply to all packages being built with Cabal, so an admonition otherwise would basically prevent people from using Setup with old Cabal ever. Not good.

@ezyang
Contributor
ezyang commented Aug 29, 2015

I've pushed a fix for this problem. Seems to work OK.

@ezyang
Contributor
ezyang commented Aug 29, 2015

Haha! Testing caught a file handle exhaustion problem (only GHC7.8 and earlier). I know what the problem is, cooking up a fix.

@ezyang
Contributor
ezyang commented Aug 29, 2015

OK, so I fixed the FD exhaustion problem by forcing the fingerprint, but now there is a second problem where readFile is not guaranteed to work if the locale is not setup correctly. Now, ideally we'd like to just hash the underlying bytestring, so I guess we should switch the implementation to use fingerprintData

@ezyang
Contributor
ezyang commented Aug 29, 2015

OK, also fixed that by using fingerprintData: 3535425 and 209147f (I'll rebase soon).

@BardurArantsson
Collaborator

QQ: If I want to test (including @ezyang's fixes), which commit should I be testing?

@ezyang
Contributor
ezyang commented Sep 10, 2015
@BardurArantsson
Collaborator

I've tried that branch on a few of my own (smallish) projects, and all seems to work OK. (Not that I'm doing anything particularly advanced, mind you.)

Am I right in thinking that the "global" package database gets ignored if you've previously built dependencies for your project and you then create a sandbox to build a project in? (Hope that sentence makes sense, I'm on painkillers ATM. Damn teeth!) That seems to be the case for one of my multi-package projects. (It's not a big deal, just a little observation that I thought I'd get cleared up.)

Overall, I'm very excited to see this work (hopefully) go into master soon! Hopefully we can reengineer/reimagine sandboxes to be much much simpler once it's in.

@BardurArantsson
Collaborator

... and of course, just as I say that, I hit a little snag:

[1 of 1] Compiling Main             ( src-test/Main.hs, dist/dist-sandbox-2bc1c660/build/tests/tests-tmp/Main.o )
Linking dist/dist-sandbox-2bc1c660/build/tests/tests ...
Creating package registration file:
/tmp/pkgConf-cqrs-memory-0.1011311762291653377373.0
Installing library in
/home/bardur/wd/cqrs/.cabal-sandbox/lib/x86_64-linux-ghc-7.10.2/cqrs-memory-0.10.0-1i9ETqIuXRojhGsgmdpxO1
Registering cqrs-memory-0.10.0...
cabal: identical key 1i9ETqIuXRojhGsgmdpxO1 already exists in DB; ghc-pkg
unregister it first.
ExitFailure 1cabal: Error: some packages failed to install:
cqrs-memory-0.10.0 failed during the final install step. The exception was:
ExitFailure 1

(FWIW, the scenario here is that I initially ran "cabal install ./cqrs-memory" and then ran "cabal install ./cqrs-memory --enable-tests". Apparently these "hash" to the same IPID.)

One can argue whether the unregister/reinstall shouldn't just happen automatically, but the error message could certainly be improved. I tried "the obvious thing", namely

ghc-pkg unregister 1i9ETqIuXRojhGsgmdpxO1

but that doesn't work because ghc-pkg doesn't understand the hash->pkg link. So I tried

ghc-pkg unregister cqrs-memory

but that doesn't work either. Interestingly, ghc-pkg list doesn't list cqrs-memory either.

What command am I supposed to be running to get rid of the already-registered "duplicate"?

(Incidentally, the error message itself should probably show the precise command that you can run, if any.)

@ezyang
Contributor
ezyang commented Sep 12, 2015

Oh, I bet it's because the sandbox logic isn't unregistering packages from the sandbox database. (That's also why ghc-pkg isn't showing it up: the package is in the sandbox. Use -f to select the sandbox db, but we should just fix this case.) The error message can be a lot better here; it should print an IPID instead of a package key, I guess?

@BardurArantsson
Collaborator

Oh, right: Here's the command that works:

ghc-pkg -f .cabal-sandbox/x86_64-linux-ghc-7.10.2-packages.conf.d/ unregister cqrs-memory

The message should probably just print that command?

@vagrawal

Hopefully we can reengineer/reimagine sandboxes to be much much simpler once it's in.

I had already done much of the work (at vagrawal@adf17a7 and https://phabricator.haskell.org/D1119). In short, it adds various nix-like facilities like using central package DB for all packages, garbage collection, allowing multiple instances (already in this patch), non-mutability without GC and also many other improvements like uninstalling and upgrading and creating lightweight sandbox manually with ghc-pkg. It still has some small issues like austin's comment in phabricator, but overall it seems to work nice. I think it will require like a day or two more to make the required changes. I was waiting for this patch to get merged as it could potentially change things in my patch and I will do the required changes after #2817 will be merged.

I had written a sort of concise specification for that patch for my mentor once. You can check it out at https://gist.github.com/fugyk/bea5e97e9f24c51c0174. The last commit was done in hurry so the current patch might not work as expected.

... and of course, just as I say that, I hit a little snag:

There are quite some known snags in this patch. I will try to list some of them here:

  • It is hard to reason weather same source hash(even with same command line flags) and same ghc version implies same ABI. There are so many other variables like programs(e.g. LLVM), OS version etc which are not included in hash.
  • Even full command line flags are not included in hash, including flags like --enable-optimization which can change ABI. We can include them by cherry-picking by hand but it does not seem a good idea.
  • It is not atomic, meaning it could calculate the hash which is not used for building. It could lead to some strange situations (described in earlier discussions).
  • As @BardurArantsson pointed out, error message is not good, but telling to manually unregister the package does not seem good either.
  • Maintaining Compatibility. @ezyang has done a great work to make this compatible with older ghc but that makes overall harder for other people to modify the code.

I have to say, I am not in favour of merging package key with IPID for the above reasons, but I guess it is needed for some simplicity in the infrastructure.

@BardurArantsson
Collaborator

There are so many other variables like programs(e.g. LLVM), OS version etc which are not included in hash.

This particular thing is probably not a huge issue, I think. For most people who are not on a rolling release distribution (no idea about Windows here), their LLVM (or whatever) probably isn't going to change in any significant way.

@23Skidoo
Member

@BardurArantsson

ghc-pkg -f .cabal-sandbox/x86_64-linux-ghc-7.10.2-packages.conf.d/ unregister cqrs-memory

cabal sandbox hc-pkg unregister cqrs-memory should also work.

@23Skidoo
Member

Even full command line flags are not included in hash, including flags like --enable-optimization which can change ABI. We can include them by cherry-picking by hand but it does not seem a good idea.

It's not very elegant, but there's no way around it, I think.

@BardurArantsson
Collaborator

@23Skidoo : Thanks, that's a bit more elegant :).

About the command line flags... what about just doing a really dumb thing and e.g. sorting + uniq'ing them and then hashing? I think it's probably better here to just assume that all command line flags have an effect on the output -- exceptions could then be explicitly removed from that list if deemed necessary. That way you'd at least always get rebuilds when appropriate at the cost of sometimes getting rebuilds when they actually technically weren't necessary. I think that's a better solution than one where you risk not getting rebuilds when you should. Does that make sense?

EDIT: Actually, command line flag order might even matter -- I'm not sure.

@ezyang
Contributor
ezyang commented Sep 23, 2015

See also #2830

@ezyang ezyang added the library label Sep 24, 2015
@vagrawal vagrawal closed this Sep 27, 2015
@vagrawal

This PR has been closed due to github's bug. I was trying to update my email address in git for submission of code samples in GSoC website, but I have overwritten all of the history due to carelessness. But after reverting back, this PR is not updating and is in permanently closed status. I have contacted github support and got the reply that it can't be opened back and I have to create a new PR. I am not opening a new PR as both of the closed PR is superseded by @ezyang's PR.

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