Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[nix-local-build] Change of version number of package not handled correctly #3323

Closed
ezyang opened this issue Apr 12, 2016 · 1 comment
Closed

Comments

@ezyang
Copy link
Contributor

ezyang commented Apr 12, 2016

Steps to reproduce:

  1. Create two packages p-0.1 and q-0.1, with modules P and Q, and q depending on p (the contents of the modules are not important, blank modules will do.)
  2. Create a cabal.project with packages: p q
  3. cabal new-build q
  4. Edit p/p.cabal so that its version is 0.2
  5. cabal new-build q -v

Expected result: Invocation of GHC to build q refers to p-0.2-inplace.

Actual result: Invocation refers to p-0.1-inplace.

CC @dcoutts

@ezyang
Copy link
Contributor Author

ezyang commented Apr 12, 2016

Here is a test case which you can run directly (the version numbers differ slightly). change-version.zip

ezyang added a commit to ezyang/cabal that referenced this issue Apr 12, 2016
…l#3323, haskell#3324).

There was a comment claiming that it was done already but this function
does not exist. I think it was referring to findProjectPackages, but
this function tests only matchFileGlob (so it will notice if a.cabal is
renamed to b.cabal) and not the actual contents of the file.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
ezyang added a commit to ezyang/cabal that referenced this issue Apr 12, 2016
…l#3323, haskell#3324).

There was a comment claiming that it was done already but this function
does not exist. I think it was referring to findProjectPackages, but
this function tests only matchFileGlob (so it will notice if a.cabal is
renamed to b.cabal) and not the actual contents of the file.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
ezyang added a commit to ezyang/cabal that referenced this issue Apr 12, 2016
…l#3323, haskell#3324).

There was a comment claiming that it was done already but this function
does not exist. I think it was referring to findProjectPackages, but
this function tests only matchFileGlob (so it will notice if a.cabal is
renamed to b.cabal) and not the actual contents of the file.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
23Skidoo added a commit that referenced this issue Apr 14, 2016
Monitor contents of Cabal files so we rerun solver upon edits (#3323, #3324).
23Skidoo pushed a commit to 23Skidoo/cabal that referenced this issue Apr 14, 2016
…l#3323, haskell#3324).

There was a comment claiming that it was done already but this function
does not exist. I think it was referring to findProjectPackages, but
this function tests only matchFileGlob (so it will notice if a.cabal is
renamed to b.cabal) and not the actual contents of the file.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
(cherry picked from commit f9929a8)
dcoutts added a commit to dcoutts/cabal that referenced this issue Apr 16, 2016
Fixes haskell#3323 and haskell#3324, ensuring we monitor the project Cabal files.
Original fix by Edward Z. Yang. The approach in this patch is to fix an
underlying problem. Subsequent patches use a more consistent approach to
the monitoring as suggested by Edward.

The motivating example is:

  matches <- matchFileGlob dirname glob

where matchFileGlob is defined in the RebuildMonad module as

matchFileGlob root glob = do
    monitorFiles [monitorFileGlob glob]
    liftIO $ Glob.matchFileGlob root glob

This usage is wrong because the root used to match the glob is not the
same as the root that will be used later when checking the file monitor
for changes. You can see this is suspicious because the declaration of
the monitor does not take an root dir paramater but the immediate
matching does. That's because the root for the monitors is specified
when we do the rerunIfChanged to check the monitor.

So the only correct usage involves passing in the correct root. This is
a ripe source of bugs. So this refactoring moves the root into the
Rebuild monad directly, so the example becomes:

  matches <- matchFileGlob glob

The root is implicit, so you can't accidentally pick a different root
for the immediate match vs the later monitor check. Of course the root
still matters, but if you get that wrong you'll notice immediately
because you will not get the match results you were expecting.

So the root is now passed in with runRebuild, not with rerunIfChanged.

Also change the incorrect use of matchFileGlob. This use case now
relies on the adjusted representation of glob roots, using
FilePath.splitDrive to obtain the root (if any).
dcoutts added a commit to dcoutts/cabal that referenced this issue Apr 16, 2016
Fixes haskell#3323 and haskell#3324, ensuring we monitor the project Cabal files.
Original fix by Edward Z. Yang. The approach in this patch is to fix an
underlying problem. Subsequent patches use a more consistent approach to
the monitoring as suggested by Edward.

The motivating example is:

  matches <- matchFileGlob dirname glob

where matchFileGlob is defined in the RebuildMonad module as

matchFileGlob root glob = do
    monitorFiles [monitorFileGlob glob]
    liftIO $ Glob.matchFileGlob root glob

This usage is wrong because the root used to match the glob is not the
same as the root that will be used later when checking the file monitor
for changes. You can see this is suspicious because the declaration of
the monitor does not take an root dir paramater but the immediate
matching does. That's because the root for the monitors is specified
when we do the rerunIfChanged to check the monitor.

So the only correct usage involves passing in the correct root. This is
a ripe source of bugs. So this refactoring moves the root into the
Rebuild monad directly, so the example becomes:

  matches <- matchFileGlob glob

The root is implicit, so you can't accidentally pick a different root
for the immediate match vs the later monitor check. Of course the root
still matters, but if you get that wrong you'll notice immediately
because you will not get the match results you were expecting.

So the root is now passed in with runRebuild, not with rerunIfChanged.

Also change the incorrect use of matchFileGlob. This use case now
relies on the adjusted representation of glob roots, using
FilePath.splitDrive to obtain the root (if any).
dcoutts added a commit to dcoutts/cabal that referenced this issue Apr 17, 2016
Fixes haskell#3323 and haskell#3324, ensuring we monitor the project Cabal files.
Original fix by Edward Z. Yang. The approach in this patch is to fix an
underlying problem. Subsequent patches use a more consistent approach to
the monitoring as suggested by Edward.

The motivating example is:

  matches <- matchFileGlob dirname glob

where matchFileGlob is defined in the RebuildMonad module as

matchFileGlob root glob = do
    monitorFiles [monitorFileGlob glob]
    liftIO $ Glob.matchFileGlob root glob

This usage is wrong because the root used to match the glob is not the
same as the root that will be used later when checking the file monitor
for changes. You can see this is suspicious because the declaration of
the monitor does not take an root dir paramater but the immediate
matching does. That's because the root for the monitors is specified
when we do the rerunIfChanged to check the monitor.

So the only correct usage involves passing in the correct root. This is
a ripe source of bugs. So this refactoring moves the root into the
Rebuild monad directly, so the example becomes:

  matches <- matchFileGlob glob

The root is implicit, so you can't accidentally pick a different root
for the immediate match vs the later monitor check. Of course the root
still matters, but if you get that wrong you'll notice immediately
because you will not get the match results you were expecting.

So the root is now passed in with runRebuild, not with rerunIfChanged.

Also change the incorrect use of matchFileGlob. This use case now
relies on the adjusted representation of glob roots, using
FilePath.splitDrive to obtain the root (if any).
23Skidoo pushed a commit to 23Skidoo/cabal that referenced this issue Apr 17, 2016
Fixes haskell#3323 and haskell#3324, ensuring we monitor the project Cabal files.
Original fix by Edward Z. Yang. The approach in this patch is to fix an
underlying problem. Subsequent patches use a more consistent approach to
the monitoring as suggested by Edward.

The motivating example is:

  matches <- matchFileGlob dirname glob

where matchFileGlob is defined in the RebuildMonad module as

matchFileGlob root glob = do
    monitorFiles [monitorFileGlob glob]
    liftIO $ Glob.matchFileGlob root glob

This usage is wrong because the root used to match the glob is not the
same as the root that will be used later when checking the file monitor
for changes. You can see this is suspicious because the declaration of
the monitor does not take an root dir paramater but the immediate
matching does. That's because the root for the monitors is specified
when we do the rerunIfChanged to check the monitor.

So the only correct usage involves passing in the correct root. This is
a ripe source of bugs. So this refactoring moves the root into the
Rebuild monad directly, so the example becomes:

  matches <- matchFileGlob glob

The root is implicit, so you can't accidentally pick a different root
for the immediate match vs the later monitor check. Of course the root
still matters, but if you get that wrong you'll notice immediately
because you will not get the match results you were expecting.

So the root is now passed in with runRebuild, not with rerunIfChanged.

Also change the incorrect use of matchFileGlob. This use case now
relies on the adjusted representation of glob roots, using
FilePath.splitDrive to obtain the root (if any).

(cherry picked from commit bba5a81)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant