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

Respect TemplateHaskell addDependentFile dependency changes #105

Closed
DanBurton opened this Issue May 29, 2015 · 21 comments

Comments

Projects
None yet
7 participants
@DanBurton
Contributor

DanBurton commented May 29, 2015

For example, when building stack itself.

  • Make a local change
  • stack build
  • gitrev writes the version as "dirty"
  • commit
  • stack build

What should happen:

  • stack recognizes that the dependency changed and rebuilds accordingly

What currently happens:

  • stack does not recognize that the dependency changed

@snoyberg snoyberg added this to the Later improvements milestone May 29, 2015

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented May 29, 2015

I just did some research on this, and the way to handle this is to call ghc --show-iface on the generate .hi files, and glean relevant information from them.

@DanBurton

This comment has been minimized.

Contributor

DanBurton commented Jul 16, 2015

Here's my intended approach to working on this.

When formulating a build plan (a specific arch, ghc, lts are selected)

  1. locate the place(s) where "get source files for this local package'" is calculated
  2. locate .hi files (if present) for the .hs/.lhs files already discovered
  3. calculate additional file dependencies
    • 3.1 for TH, it's just file names, relative to that package's root (I don't believe there's such a thing as extra-source-dirs?)
    • 3.2 for module dependencies not already accounted for, look for these in any of the relevant hs-source-dirs
  4. print warnings about these additional dependencies if they are not already accounted for in the cabal file, suggest adding them to the cabal file with copy/pasteable output. (extra-source-files for 3.1, other-modules for 3.2)
  5. return the augmented list of source files

As I understand it, the build cache should smoothly take it from there, as long as we tell it about these extra files.

Some nuances and thoughts regarding:

  • locating .hi files:
    getInstallationRoot gets us to .stack-work/install/$arch/$lts/$ghc/.
    From there we need to drill down into lib/$arch-ghc-$ghc/$package-$packageVer/,
    and the .hi files for that package are in module hierarchy locations from there.
  • dependencies of dependencies
    Steps 2 thru 5 may need to be repeated until no new file dependencies are discovered.
    Limit N repeats for sanity?
  • .hi files absent prior to first build
    The build process should work correctly anyways since it is a clean build.
    I'm guessing subtle bugs may creep in here.
    The warnings, as outlined above, won't trigger unless we add a post-build check.
  • first time build cache
    The first build cache won't include these extra files, which may lead to extra rebuilding.
    This is a second reason to add a post-build check: it can update the build cache at that time.
  • build cache details
    We might need some extra way of marking a module as "needs rebuild due to dirty file dependency". I'm not quite sure what the implementation details are here.

I might stub out the "module dependencies" part (3.2) if it is tricky.

@DanBurton

This comment has been minimized.

Contributor

DanBurton commented Jul 16, 2015

One shortcoming of this approach is that it only works for a library target. I can't find the .hi file for the main-is in an executable target.

@DanBurton

This comment has been minimized.

Contributor

DanBurton commented Jul 16, 2015

Never mind. The .hi file appears to be in .stack-work/dist/$arch/Cabal-$cabalLibVer/build/$package/$package-tmp/. I'm not sure how reliably persistent this is, but it is mildly annoying that it is in a different location.

@chrisdone

This comment has been minimized.

Member

chrisdone commented Jul 16, 2015

Package files are gotten in packageDescFiles.

@DanBurton

This comment has been minimized.

Contributor

DanBurton commented Jul 16, 2015

Other notes:

  • beware of circular dependencies
  • the "repeat" procedure only needs to be calculated on the new files, not the old ones
@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jul 17, 2015

We need to also consider performance here. Having GHC dump the contents of
a hi file may be non trivial, in which case caching that information when
the files haven't changed is a good idea.

On Thu, Jul 16, 2015, 11:01 AM Dan Burton notifications@github.com wrote:

Other notes:

  • beware of circular dependencies
  • the "repeat" procedure only needs to be calculated on the new files,
    not the old ones


Reply to this email directly or view it on GitHub
#105 (comment)
.

DanBurton added a commit that referenced this issue Jul 20, 2015

@borsboom borsboom assigned borsboom and unassigned DanBurton Jul 27, 2015

borsboom added a commit that referenced this issue Aug 7, 2015

borsboom added a commit that referenced this issue Aug 7, 2015

@borsboom borsboom added the in progress label Aug 8, 2015

borsboom added a commit that referenced this issue Aug 8, 2015

@borsboom

This comment has been minimized.

Contributor

borsboom commented Aug 8, 2015

Discussion in PR #734.

borsboom added a commit that referenced this issue Aug 8, 2015

borsboom added a commit that referenced this issue Aug 9, 2015

borsboom added a commit that referenced this issue Aug 9, 2015

Merge pull request #734 from commercialhaskell/wip/manny/32-105-unlis…
…ted-dependencies

Detect unlisted modules and TH dependent files (#32,#105)

@borsboom borsboom closed this Aug 9, 2015

borsboom added a commit that referenced this issue Aug 9, 2015

chrisdone added a commit that referenced this issue Aug 9, 2015

chrisdone added a commit that referenced this issue Aug 9, 2015

borsboom added a commit that referenced this issue Aug 19, 2015

borsboom added a commit that referenced this issue Aug 19, 2015

borsboom added a commit that referenced this issue Aug 19, 2015

borsboom added a commit that referenced this issue Aug 19, 2015

borsboom added a commit that referenced this issue Aug 19, 2015

borsboom added a commit that referenced this issue Aug 19, 2015

@mitchellwrosen

This comment has been minimized.

Contributor

mitchellwrosen commented Mar 10, 2017

This still seems to be happening for multiple-package projects.

@rimmington

This comment has been minimized.

Contributor

rimmington commented Jun 17, 2017

I'm still experiencing this issue with a trivial single-executable project. I've uploaded a simple reproduction.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 18, 2017

@mitchellwrosen

This comment has been minimized.

Contributor

mitchellwrosen commented Jun 18, 2017

The example doesn't work correctly on my machine, if the correct stack behavior is to build twice and thus produce two different hashes. @snoyberg, you may have run it wrong, I think the author accidentally committed thing to the repo :)

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 18, 2017

You're right, I was getting confused by the repo. Nonetheless, the bug isn't in Stack, it's in gitrev, which seems to be mishandling whitespace. I'll open up an issue. (This is also why we generally ask for minimal reproducing test cases, to ensure that the bug is really in Stack and not some other library.)

snoyberg added a commit to snoyberg/gitrev that referenced this issue Jun 18, 2017

Fix a number of issues
These were uncovered when researching commercialhaskell/stack#105. What
I discovered:

* There was an extra layer of `.git` directories in computed paths
* The HEAD file should be added dependently regardless of whether it's a
  detached head
* Add a missing check in relRef for newline
* Switch newline handling to check both CR and LR for Windows support
@rimmington

This comment has been minimized.

Contributor

rimmington commented Jun 19, 2017

My apologies for the poor quality of the repro. The gitrev PR fixes this issue for me.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Jun 19, 2017

@nh2

This comment has been minimized.

Collaborator

nh2 commented Feb 7, 2018

Possibly relevant for people who read this issue: acfoltzer/gitrev#17

Document the fact that addDependentFile on .git/index can lead to unnecessary recompilation

@nh2

This comment has been minimized.

Collaborator

nh2 commented Feb 7, 2018

I'm thinking this whole git logic might sit much better in a buildHook or a similar user hook.

But a question @snoyberg / @DanBurton:

If such a hook was used, would that still work with stack's own recompilation avoidance? Because if stack doesn't run things like buildHook in all cases, then putting the logic there wouldn't work for stack.

@snoyberg

This comment has been minimized.

Contributor

snoyberg commented Feb 7, 2018

No idea, I've never really understood how user hooks are triggered in Cabal. But I'd stay far away from a solution that uses any kind of custom Setup.hs file. I'd rather just recompile every module every time I hit build than that (only half exaggerating).

@nh2

This comment has been minimized.

Collaborator

nh2 commented Feb 8, 2018

No idea, I've never really understood how user hooks are triggered in Cabal.

As far as I can tell, buildHook and preprocessors are run on very build, so that's fine. My question was more about Stack's extra recompilation avoidance in multi-project setups, where (IIRC) it doesn't even configure when it notices (via mtime) that nothing has changed.

I'd rather just recompile every module every time

@snoyberg This is literally what's happening for me when using only TH and no hook, but I'm having a hard time with it because it just makes iterating so slow. So right now I'm in favour of the hook.

@nh2

This comment has been minimized.

Collaborator

nh2 commented Feb 10, 2018

@mgsloan Has confirmed in chat with me that indeed stack's own recompilation avoidance will prevent hooks from running if files on disk didn't change. That means that right now, using only a buildHook won't work with stack, it will not be run if no file tracked by stack is changed.

@nh2

This comment has been minimized.

Collaborator

nh2 commented Feb 10, 2018

More discussion:

@mgsloan Right now when using gitrev and its TH instead of a hook, stack knows when it has to recompile because it tracks the addDependentFile ".git/index" done by gitrev. Are there other ways to get a file into the list of files watched by stack beyond addDependentFile?

Because I think the solution would be to put .git/index into this list, but not via addDependentFile, so that only stack knows about that file but not GHC. Then when a commit is made and index changes, stack will know that it has to rebuild, it will run the Setup.hs hooks, and the hook will correctly discover whether or not an actual commit was made.

@mgsloan said:

Yeah I suppose that would work. Perhaps extra-source-files would do the trick.

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