Skip to content

Don't trust sdist output, it might report files that don't exist #3786

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

Closed
ezyang opened this issue Sep 6, 2016 · 3 comments
Closed

Don't trust sdist output, it might report files that don't exist #3786

ezyang opened this issue Sep 6, 2016 · 3 comments

Comments

@ezyang
Copy link
Contributor

ezyang commented Sep 6, 2016

Right now, we trust every file reported by sdist as one we need to monitor. This means that if sdist reports a file which does NOT exist, we will repeatedly rebuild (uselessly!) So we need to not do that.

I suppose there are two things to observe:

  • Since cabal build won't tell us what files it actually looked at / what their statuses were, we have to test it ourself in cabal-install (and we will test it again when the file monitor recaches!) OUCH.
  • We should probably update our file monitor before we actually do the build, to avoid race conditions where the build has nothing to do with what we observe when we probe for the monitor. (There are still races, see [nix-local-build] Recompilation avoidance ABA problem #3179).

Maybe there should be a mode for file monitor that says, "compute the stats/hashes as they are right now", so that we aren't uselessly restatting files.

This bug might be obsoleted by #3401; in this rewrite, we could just make sure that any files we return actually exist.

ToDo: Bother @dcoutts about this enough until he agrees this is what we should do.

@ezyang ezyang added this to the 2.0 (planned for next feature release) milestone Sep 6, 2016
@ezyang ezyang self-assigned this Sep 6, 2016
@dcoutts
Copy link
Contributor

dcoutts commented Sep 8, 2016

Imho, we should not call sdist at all but derive the files from the .cabal file using our own globs (at least for non-custom).

Note I've also seen a sdist bug where it doesn't handle this construct correctly (due to the naive flattening):

if flag(foo-hacking)
  main-is: Foo.hs
else
  main-is: FooHacking.hs

This gets flattened into "Foo.hsFooHacking.hs".

Also, we know we can't do it correctly via sdist --list-sources since it does not handle search paths correctly (it just says where things were found, not where we looked for them). So I'd advocate that we construct the globs ourselves from the .cabal file info and then look at the corner cases where there's extra info from Setup.hs or whatever.

@23Skidoo
Copy link
Member

23Skidoo commented Sep 8, 2016

This gets flattened into "Foo.hsFooHacking.hs".

That's #3313.

@ezyang
Copy link
Contributor Author

ezyang commented Sep 8, 2016

OK, I'll obsolete this bug with #3401.

@ezyang ezyang closed this as completed Sep 8, 2016
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

3 participants