Installplan installed state #3863
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Closed
Do you have an example |
|
No idea why we're getting |
So actually it needs one more change before we plan output changes: With that, then the change is that the Now if we want to do it like this then actually it makes sense to update the plan after building so that it reflects the new installed status post-build. Of course it'd still be updated on |
This patch just adds the state without yet using it. That'll follow in subsequent patches. So why add an Installed state? Didn't we just remove the Installed, Processing and Failed states? Those states were used when we followed the approach of updating the InstallPlan as a build progressed (whereas we now do traversals without altering the InstallPlan). The idea of adding an Installed state now is that we can more usefully represent the state of the plan when we "improve" the plan with packages from the store or when we update the plan having checked if inplace packages are up to date. Currently in these two places we replace Configured source packages with PreExisting packages. There's a couple problems with this. Firstly the PreExisting state only contains an InstalledPackageInfo which means we loose information compared to all the detail in the Configured source package. This is relevant for things like plan.json output or other features that want to know the status of a project. Secondly we have to fake things for executables since they are not properly represented by InstalledPackageInfo.
The change in how we use the PreExisting vs Installed states means that we'll now have full details for all packages, rather than installed ones having only the subset of info available from the InstalledPackageInfo. So the 'type' field now can take the values "pre-existing", "configured" or "installed". Also do a little bit of tidying up.
Change improvement and --dry-run phases to use Installed state rather than the PreExisting state. This means that PreExisting is now only used for installed packages from the global db, and never for installed packages from the store.
We only ever switch Configured to Installed. The PreExisting state only comes from the original solver plan, which only uses installed packages from the global db.
Not a big deal, but should be useful later for more precise status reporting. For now just means the rebuild reasons can be more precise.
dcoutts
force-pushed
the
installplan-installed-state
branch
from
334e94a
to
2dd0812
Compare
|
Rebased on latest master with ezyang's improved ghc-pkg dump error message so hopefully we can diagnose the appveyor build failures. |
|
Ah, ok, that's better: |
|
Ok, now I really don't get these appveyor build failures: This happens when building the dependencies of cabal. And this is after having installed and registered dozens of other packages, so how did the file permissions change? |
|
Usually that indicates an intermittent failure. I kick a rebuild in such cases. We should eventually figure out why this is happening though. |
|
LGTM, assuming tests pass. |
Install plan improvement is the process of replacing configured source packages with installed instances from the store. Originally we did this by reading the ghc-pkg db to get the InstalledPackageInfo for all the packages in the store. We had to do that because when we replaced configured source packages by installed instances we used the PreExisting constructor which requires an InstalledPackageInfo, which we get from the installed package db. But now that we no longer use PreExisting for packages from the store we also no longer need the InstalledPackageInfo. All we need is a set of UnitIds. Also, once support for depending on executables was added then we needed a way to do plan improvement for executable packages/components. We did this by the simple approach of grabbing the dir listing for the store and using that as a set of UnitIds. So this patch extends the approach we use for executables and uses it for all packages. This means we no longer load the package db for the store. Note that we still have to create the package db in the store. This also relates to the locking protocol in the store. The goal for the store is to be able to access and update it concurrently. The locking protocol will include checking membership by checking if the directory entry for the package is present. So this patch gets us to the right point for the reading side, leaving the writing side to do.
All the use sites (currently only two but soon to be three) use InstallPlan.installed to do a bulk change of states, differing only in the filter condition. So it simplifies things and shares more code if we make the main one be the bulk version. The InstallPlan.remove already works similarly.
dcoutts
force-pushed
the
installplan-installed-state
branch
from
961ec40
to
66ed37a
Compare
|
I reverted this because it seems to have broken improvement: |
ezyang
reviewed
Sep 20, 2016
| @@ -731,6 +733,11 @@ getPkgConfigDb verbosity progdb = do | |||
| liftIO $ readPkgConfigDb verbosity progdb | |||
|
|
|||
|
|
|||
| getDirectoryContentsMonitored :: FilePath -> Rebuild [FilePath] | |||
| getDirectoryContentsMonitored dir = do | |||
| monitorFiles [monitorDirectory dir] | |||
There was a problem hiding this comment.
This is wrong, you need to monitor the directory contents, not the directory itself.
There was a problem hiding this comment.
And this is just an example of how the API is error prone; better to do something Shake-like where you have no access to the manual monitorFiles call ;)
There was a problem hiding this comment.
Are you sure? A directory should change mtime when any entry within it is created/deleted.
I could be wrong of course, but I did actually think about this very issue when writing that code. Re error prone, yes more wrappers like this one in the Rebuild monad will help. Of course we have to get those ones correct :-)
There was a problem hiding this comment.
From the stat(2) man page (and I think this is true on Windows too):
Moreover, st_mtime of a directory is changed by the creation or deletion of files in that directory.
(which is also what I observe experimentally)
So this should be just enough for getDirectoryContentsMonitored, since it's the creation of dirs in the store that should trigger re-reading the entries of that dir.
And getDirectoryContentsMonitored should be just enough for reading the store, since entries in the store are (supposed to be) immutable so it's just the adding/removing of them that matters.
So where is this breaking down?
There was a problem hiding this comment.
This behaviour is the same on NTFS, though apparently not on FAT.
There was a problem hiding this comment.
OK, you are right. I guess there is a different bug...
|
Ok you are right. I think I have diagnosed the real problem: it is a bug in the file monitor API. But first, here are steps to reproduce:
So the problem, discovered by tracing the monitor calls, is that the store directory gets monitored twice: first tracking mtime, and then with just existence. What seems to happen is that the mtime monitor is overwritten, so that we now only test directory existence. I guess monitors should be a monoid and accumulate? I have a failing unit test for this (just call monitor on the same dir twice), but my bad internet connection won't let me upload it. On September 21, 2016 2:16:24 AM GMT+09:00, Duncan Coutts notifications@github.com wrote:
Sent from my Android device with K-9 Mail. Please excuse my brevity. |
Ahh! Yes. You're quite right. Great catch! Now that you've pointed it out I can clearly see this behaviour from the code. We simply probe the state of each file/dir requested and insert the result into a Map (hence overwriting if we probed that file before). I'll alter it to build a Map first, merging the monitor kinds, then probe them all. |
|
Here is a unit test for the problem, for your patch ;) |
Merged
|
@ezyang oh, sorry didn't see your unit test, but I added one fairly similar. FYI, now that I see it, I don't think that test will work. Note that it's not in the rebuild monad so that So I'll rebase this PR and resubmit. Thanks again for the speedy reversion and diagnosis. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
So why add an Installed state? Didn't we just remove the
Installed,ProcessingandFailedstates? Those states were used when we followed the approach of updating theInstallPlanas a build progressed (whereas we now do traversals without altering theInstallPlan).The idea of adding an Installed state now is that we can more usefully represent the state of the plan when we "improve" the plan with packages from the store or when we update the plan having checked if inplace packages are up to date. Currently in these two places we replace
Configuredsource packages withPreExistingpackages. There's a couple problems with this. Firstly thePreExistingstate only contains anInstalledPackageInfowhich means we loose information compared to all the detail in the Configured source package. This is relevant for things like plan.json output or other features that want to know the status of a project. Secondly we have to fake things for executables since they are not properly represented byInstalledPackageInfo. So this avoids the need for thos hacks.Also simplify the way we do the improvement now that we no longer need the
InstalledPackageInfo. Now rather than reading the ghc-pkg db we can just get the dir listing for the store and use that as a set ofUnitIds.This already helps the plan.json output and will also be needed for maintaining the .ghc.environment file sanely.