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

Replace new-build's call to sdist with new command which lists sources of things that will be built #3401

Closed
ezyang opened this issue May 5, 2016 · 6 comments · Fixed by #3985

Comments

@ezyang
Copy link
Contributor

ezyang commented May 5, 2016

Summary by @ezyang. For Nix-local build recompilation avoidance, we need a variant of sdist --list-sources mode which prints the list of files which will be used by ./Setup build under the current ./Setup configure parameters. It would be implemented in the following way:

  1. Read out the LocalBuildInfo (unlike sdist, we require the package to be configured.)
  2. Get the resolved PackageDescription from LocalBuildInfo
  3. Use the same existing --list-sources code to get all of the sources.

I don't know if this should be a flag on sdist: the sdist commands can be run without configuring, but we require configuration for this command. Perhaps we can introduce a new subcommand like ./Setup query sources which lets us interrogate Setup.hs for more information.

New Setup commands live in [Cabal/Distribution/Simple.hs]. The list-sources code is in listPackageSources in [Cabal/Distribution/Simple/SrcDist.hs]. The place where Nix-local-builds would hook in is the call to allPackageSourceFiles in [cabal-install/Distribution/Client/ProjectBuilding.hs]

Testing strategy:

  1. Create an ordinary Cabal project with a non-buildable section referencing a Haskell source file
  2. Build it with cabal new-build
  3. Modify the source file.
  4. Rerun cabal new-build and verify that no work was necessary

For recompilation avoidance, new-build invokes cabal sdist --list-sources to get a list of files that are ostensibly participate in the compilation (any file that isn't packaged up in an sdist shouldn't affect compilation, since it won't be put in a tarball that gets distributed to others!)

Here's the problem: we shouldn't assume that cabal sdist --list-sources will always succeed. For example, suppose that a non-buildable section of Cabal file refers to a module that does not actually exist. Setup build will work just fine (since it won't actually try to access any of these modules) but cabal sdist --list-sources will fail (very unclearly; there's no visual indication that we were list-source'ing) because it will attempt to sdist all modules. Arguably, sdist --list-sources should also get a list of targets like build, and it will list sources only for those targets; however, because we have to support Custom on old Cabal we have to accept that this may happen.

Now, suppose that we can't list-sources to get an accurate picture about what files need to be tracked. What should we do in this case? We shouldn't pass an empty list of files to the monitor; then we'll never rebuild when a user (legitimately) edits a file. So we need some kind of fallback. Here are a few obvious possibilities:

  1. Recursively track everything in the directory. This is what new-build used to do. This strategy can explode in some pretty spectacular ways so I'd like to avoid it.
  2. We could "force" ourselves to use an up-to-date implementation of sdist (i.e., bypass Custom Setup), so that we at least get "something". In this case we need to improve sdist to take targets and to also be "robust to errors" (i.e., if the Cabal claims something should be there which isn't, roll with it, and do as good a job as you can.)

What do people think?

@23Skidoo
Copy link
Member

23Skidoo commented May 5, 2016

Improving the robustness of --list-sources seems to be the obvious way forward. Sandboxes would also benefit from this.

@ezyang
Copy link
Contributor Author

ezyang commented Jul 14, 2016

So, I actually figured out what the problem is: we need a variant of sdist which operates on the PackageDescription as resolved during the configure step (i.e., the PD stored in LocalBuildInfo), as opposed to the flattened description that sdist unconditionally uses right now. (sdist tries to work even if you haven't configured but that is clearly not a consideration here.) I updated the description.

@ezyang ezyang changed the title Bugs related to new-build calling sdist --list-sources Replace new-build's call to sdist with new command which lists sources of things that will be built Jul 14, 2016
@ezyang ezyang self-assigned this Aug 2, 2016
@ezyang ezyang mentioned this issue Aug 2, 2016
38 tasks
@dcoutts
Copy link
Contributor

dcoutts commented Sep 8, 2016

As I said in #3786 we should just produce the globs ourselves from the .cabal file description, and not bother with sdist at all.

@ezyang
Copy link
Contributor Author

ezyang commented Sep 8, 2016

@dcoutts So what are you going to do about packages which use Custom setup to register extra preprocessors? That information is not in the Cabal file, so you are screwed! There needs to be a Cabal API.

@23Skidoo
Copy link
Member

23Skidoo commented Sep 8, 2016

IIRC in general sdist hooks can arbitrarily modify the list of included files. It'd be nice to disallow this and force usage of autogen-modules and extra-source-files.

@ezyang
Copy link
Contributor Author

ezyang commented Oct 16, 2016

I'm working on this.

ezyang added a commit to ezyang/cabal that referenced this issue Oct 16, 2016
…ing.

New module Distribution.Client.SourceFiles implements
'needElaboratedConfiguredPackage', which if run in the 'Rebuild'
monad is sufficient to ensure all source files that participate
in a build are monitored.

Fixes haskell#3401.  It also fixes the "we didn't detect a new file
appearing" problem.

Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Oct 16, 2016
…ing.

New module Distribution.Client.SourceFiles implements
'needElaboratedConfiguredPackage', which if run in the 'Rebuild'
monad is sufficient to ensure all source files that participate
in a build are monitored.

Fixes haskell#3401.  It also fixes the "we didn't detect a new file
appearing" problem.

Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Oct 16, 2016
…ing.

New module Distribution.Client.SourceFiles implements
'needElaboratedConfiguredPackage', which if run in the 'Rebuild'
monad is sufficient to ensure all source files that participate
in a build are monitored.

Fixes haskell#3401.  It also fixes the "we didn't detect a new file
appearing" problem.

Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Oct 16, 2016
…ing.

New module Distribution.Client.SourceFiles implements
'needElaboratedConfiguredPackage', which if run in the 'Rebuild'
monad is sufficient to ensure all source files that participate
in a build are monitored.

Fixes haskell#3401.  It also fixes the "we didn't detect a new file
appearing" problem.

Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Oct 17, 2016
…ing.

New module Distribution.Client.SourceFiles implements
'needElaboratedConfiguredPackage', which if run in the 'Rebuild'
monad is sufficient to ensure all source files that participate
in a build are monitored.

Fixes haskell#3401.  It also fixes the "we didn't detect a new file
appearing" problem.

Signed-off-by: Edward Z. Yang <[email protected]>
ezyang added a commit to ezyang/cabal that referenced this issue Oct 18, 2016
…ing.

New module Distribution.Client.SourceFiles implements
'needElaboratedConfiguredPackage', which if run in the 'Rebuild'
monad is sufficient to ensure all source files that participate
in a build are monitored.

Fixes haskell#3401.  It also fixes the "we didn't detect a new file
appearing" problem.

Signed-off-by: Edward Z. Yang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants