haskell / cabal

Official upstream development repository for Cabal and cabal-install
https://haskell.org/cabal
Other
1.62k stars 691 forks source link

cabal new-build does not track files added via TH’s addDependentFile #4746

Open cocreature opened 7 years ago

cocreature commented 7 years ago

Steps to reproduce

Expected behavior

cabal new-build rebuilds because fileinput has changed.

Actual behavior

cabal new-build exits without rebuilding after saying Up to date.

This issue affects both cabal-2.0 as well as the master branch.

phadej commented 7 years ago

One hack solution is to watch extra-source-files too. But that won't solve dynamic (e.g. what inline-c uses?) stuff.

23Skidoo commented 7 years ago

BTW, is there a way to force ghc --make always rerun a TH function? E.g. if I'm doing some kind of I/O at compile-time, like reading from an environment variable.

phadej commented 7 years ago

@23Skidoo that would force GHC to always build that module, that doesn't sound like a very good idea.

23Skidoo commented 7 years ago

Yep, but sometimes you want that in a build system. For example, Shake rules can be marked alwaysRerun.

Re: extra-source-files, that field is package-global and commonly used for things like README and changelog. So we'd need to allow per-component extra-source-files to avoid unnecessary recompilation.

23Skidoo commented 7 years ago

An alternative way to fix this is to get the list of files to be monitored from ghc --make via -ddump-module-graph (a new feature that is going to be in 8.4).

phadej commented 7 years ago

Does -ddump-module-graph dump addDependentFile & co stuff?

23Skidoo commented 7 years ago

@phadej If you click that link, you'll see that I asked that exact question there.

cocreature commented 7 years ago

Given that this feature will only be in GHC 8.4 I don’t think that’s a reasonable solution at this point.

23Skidoo commented 7 years ago

@cocreature With old versions we can simulate that feature with ghc -M (which will be slower because we'll have to do dependency resolution twice - once for ghc -M and once for ghc --make, but for legacy GHC versions we can afford to not care). BTW, ghc -M does support addDependentFile dependencies:

# DO NOT DELETE: Beginning of Haskell dependencies
Data/FileEmbed.so : Data/FileEmbed.hs
Main.so : Main.hs
Main.so : Data/FileEmbed.shi
# DO NOT DELETE: End of Haskell dependencies
hvr commented 7 years ago

@23Skidoo btw, we can avoid paying for the slow-legacy fallback when it's not needed, because we know when TH is needed (we already require TemplateHaskell to be declared in *-extensions in order to enable other logic specific to TH). So I think that's a workable fallback.

hvr commented 6 years ago

Unfortunately we not only missed the boat for GHC 8.4, but most likely also for GHC 8.6 to get supporting features into GHC... as https://phabricator.haskell.org/D3898 got stuck on a decision which design path to go down... :-/

PS: Screen-scraping GHC's dump-hi output would be an obvious but terrible hack for the obvious reasons and since it's not intended to be consumed by tooling its format is neither documented nor guaranteed to be correct and can change without notice.

ygale commented 5 years ago

Upvoting this issue. It is critical for larger web-dev shops that have people who are totally focused on front-end web UI development. They require instant reload; they don't have time to poke around at files manually every time to induce rebuilds.

We are a yesod shop, so we are using stack. For comparison, stack detects this by parsing dump-hi files. The syntax change in dump-hi files in GHC 8.4 caused a similar issue in stack for a time, now fixed.

tom-audm commented 5 years ago

Another upvote for this issue!

I think type:bug is a more accurate description than type: enhancement. It's a regression from cabal old-build's behavior, and can cause truly wtf moments when using e.g. gitrev (see my issue #5866 ). I'd say it actually breaks the gitrev package for practical usage.

kaldonir commented 4 years ago

We ran into the same issue today. Is there any progress here?

phadej commented 4 years ago

@kaldonir, no. As far as I know no-one is actively working on adding necessary features to GHC (e.g. https://github.com/ghc-proposals/ghc-proposals/pull/245 or something similar so the compiler could tell Cabal what extra dependencies exist).

Alternatively or additionally cabal could track extra-source-files, which is imprecise yet better than nothing. Again, I'm not aware anybody is working on that either.

kmicklas commented 4 years ago

@phadej Are you opposed to making cabal track extra-source-files as an intermediate step? This wouldn't help with gitrev but would solve this for I suspect most use cases, until we have a proper fix in GHC. I'm willing to implement this if there's support for it.

phadej commented 4 years ago

I'm not. It should be easy, I think it should go to the same place as https://github.com/haskell/cabal/commit/c2f4625f2feeda94a9174d927dc39a91c0059a7e

PR welcome!

parsonsmatt commented 3 years ago

So parsing the .dump-hi files was discouraged a while ago. I'm curious if there's any objection to just doing that, since it's what stack has been doing since 2015 with one bug report in that time.

If this is an acceptable solution then I can allocate some time and resources to implementing it.

Thanks!

Ericson2314 commented 3 years ago

GHC should be able to give cabal this info in a machine consumable cormat.

But frankly I don't really like the design of addDependentFile because it precludes proper sandboxing — it's very dynamic sort of dependency.

parsonsmatt commented 3 years ago

GHC should be able to give cabal this info in a machine consumable cormat.

Agreed, but it hasn't started doing that in the 4 years that this ticket has been open. I'm interested in solving it now.

gbaz commented 3 years ago

@parsonsmatt for solving now, would simply tracking extra-source-files as suggested above suffice? That should be straightforward to implement.

Edit: hrm looks like we actually do that, but just fail to do so with wildcard matches. So I guess it suffices to patch that?

parsonsmatt commented 3 years ago

We've got ~176 files to track (and counting), and manually tracking them in the cabal file would be a major nuisance. But patching the wildcard behavior would suffice, though I do still think this should be a seamless process.

Ericson2314 commented 3 years ago

GHC's driver is being overhauled at the moment, which will help with this and other dependency issues.

Mikolaj commented 3 years ago

Yes, I remember tracking extra source files started working again, recently. Is there a ticket about adding wildcards to extra-source-files? The closest I could find was https://github.com/haskell/cabal/issues/7290.

Ericson2314 commented 3 years ago

Yeah tracking extra-source-files is definitely the right short-term solution.

gbaz commented 3 years ago

I have a pr now for glob expanding extra-src-files and am moving the remainder of the ticket to blocked on ghc, because I think we really don't want to hack around ghc proprietary formats to try to extract this information in any more sophisticated way.

elldritch commented 2 years ago

Is there an easy way to instrument addDependentFile calls so we can figure out what files to add to extra-source-files as a workaround?

gbaz commented 2 years ago

The type is FilePath -> Q () -- you can wrap it in any other function you want including IO actions lifted to Q to output the data however you desire.

parsonsmatt commented 2 years ago

If you're able to operate in an abstract monad, like Quasi m => m _, then you can provide a newtype around Q and define an instrumented qAddDependentFile. But this won't work for anything hardocded to Q, which is quite a lot of the API.

phadej commented 2 years ago

@parsonsmatt it will. runQ :: Quasi m => Q a -> m a is your friend.

elldritch commented 2 years ago

Yeah, this works for my own code calling addDependentFile. What I'm really trying to do is figure out how to intercept and instrument addDependentFile calls from external TH libraries that I depend on (e.g. file-embed). It seems that this is not possible.

jberryman commented 2 years ago

Can confirm in cabal 3.6.2.0 when I add the argument to addDependentFile to extra-source-files, addDependentFile seems to work properly (and an unchanged module isn't recompiled)

michaelpj commented 1 year ago

This seems to have broken again in 3.8.1, are we sure that https://github.com/haskell/cabal/pull/7608 is correct?

SamirTalwar commented 1 year ago

We saw the same problem in https://github.com/hasura/graphql-engine, but unfortunately I couldn't reproduce it with a little project, so I figured we were doing something wrong. Interesting to know we're not the only ones.

For now, we have downgraded to Cabal 3.6 in CI.

gbaz commented 1 year ago

Well 3.6.2.0 was just a backport of 7608, so if that was correct, I don't see how it would have broken in main unless there was an unrelated regression? Can you expand with a reproducer, or at least a more cogent statement of what the expected vs actual behavior is?

michaelpj commented 1 year ago

I haven't tried to minimize yet. It's large, but here's our actual reproducer: https://github.com/input-output-hk/plutus

cabal build plutus-core:plutus-core
# edit plutus-core/cost-model/data/builtinCostModel.json>
cabal build plutus-core:plutus-core
# observe no recompliation

We've confirmed that this works on cabal 3.6.2, and not on 3.8.1. I've tried a few semi-random things to try and get cabal to notice the change:

  1. Move the file to the package root to avoid directory separators in the pattern.
  2. Remove all the other extra-source-files
  3. Explicitly add a call to addDependentFile <path> to the relevant module.
  4. Changing to use a wildcard match on the file extension
  5. Bumping the cabal-version of the cabal file

Well 3.6.2.0 was just a backport of 7608, so if that was correct, I don't see how it would have broken in main unless there was an unrelated regression?

Okay, well we have it working on 3.6.2 so my finger-pointing at 7608 must be wrong :) Then I'm quite baffled!!

michaelpj commented 1 year ago

One more failed experiment:

  1. Add the package directory to the extra-source-file path in case there was a CWD mixup and it was looking from the project root somehow.
michaelpj commented 1 year ago

(Perhaps as a tangent, but seeing the discussion here: maybe I don't want cabal to die if an extra-source-file line matches nothing, but I sure would like a warning. It almost certainly means I've made a mistake as the user!)

Mikolaj commented 1 year ago

@michaelpj: may I ask you to open a new ticket, copy the reproduction info and mark it a regression in 3.8? That would be high priority to investigate before 3.10. It'd at least make sure the https://github.com/haskell/cabal/pull/7608 commit has not been lost on 3.8 branch and on current master.

Re warning when glob is empty, that also merits a new ticket, perhaps even a newcomer one.

michaelpj commented 1 year ago

Will do.

michaelpj commented 1 year ago

https://github.com/haskell/cabal/issues/8632 https://github.com/haskell/cabal/issues/8634

I don't have labeling permissions, so I can't label them, sorry.

ulysses4ever commented 1 year ago

@Mikolaj we should give the permissions to Michael, I think

Mikolaj commented 1 year ago

No way, @michaelpj, I've just checked and you weren't even a lowly Read-level member of the project according to the Settings page. I've invited you to the respectable Triage level. Could you try?

michaelpj commented 1 year ago

done

Mikolaj commented 1 year ago

Perfect. Thank you.

NorfairKing commented 1 year ago

+1 Still an issue it seems.

hsyl20 commented 1 year ago

Stack's code for parsing .hi files is in a separate library: https://github.com/commercialhaskell/hi-file-parser. I've recently updated it to support 9.4.5 and 9.6.1. Cabal could use it to detect dynamic dependencies per module. The dependency on RIO could easily be removed.

NorfairKing commented 1 year ago

Linking this to https://github.com/haskell/cabal/issues/8605 because I can't start using cabal until this is fixed.

gbaz commented 1 year ago

Thinking about hi based tracking I think there's a potential issue: if files are not in the .cabal manifest they still won't get packaged up by an sdist. So that means that an sdist of a package that works locally with some "dynamic" tracking will fail if packaged up. I think that's not a good situation. As it stands, I'm not sure why tracking extra-src-files for rebuild along with glob-expansion of them doesn't suffice.

KristianBalaj commented 7 months ago

I think this is a related issue I've just created - https://github.com/haskell/cabal/issues/9745