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

Use sdist to find files to monitor, fixes #3019 for Simple Cabal files #3265

Closed
wants to merge 3 commits into from

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Apr 1, 2016

First two commits are just simple updates. Third commit is a refactoring I needed to reuse sdist in cabal-install. Last commit is the fix.

I'm reasonably confident this should work, but I haven't tested quite enough (there aren't any integration tests for new-build boohoo).

@ezyang
Copy link
Contributor Author

ezyang commented Apr 1, 2016

CC @dcoutts

findFirstFile id
findFile = findFile0 id

findDirFile :: FilePath -- ^ working directory (not included in result)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add Haddock comments to all new top-level definitions in this file.

@23Skidoo
Copy link
Member

23Skidoo commented Apr 1, 2016

The .cabal file can sometimes contain references to non-existing files (via extra-source-files, for example), so may be worth adding a test for that.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 1, 2016

Well, tests for new-build in general would be good...

IIUC, sdist will just not report files that don't exist. This can tickle a known bug which I mentioned at #3019 (comment) I don't know what happens in this situation.

@23Skidoo
Copy link
Member

23Skidoo commented Apr 1, 2016

Also, the sandbox code uses the setup script to list sources (added in c7aa581/#1305). The reason for that is apparently #403.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 1, 2016

Oh that's lovely! I thought I was going to have to implement that but I guess not! (Note that #403 only applies for Custom setup scripts, so we can avoid an exec when the build type is simple.)

@23Skidoo
Copy link
Member

23Skidoo commented Apr 1, 2016

so we can avoid an exec when the build type is simple.

setupWrapper does that automatically for you.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 1, 2016

Oh, I see that internalSetupMethod uses inDir to change the working directory, which means it won't work in parallel. (Oh! That explains why we force an external process for parallel builds.)

@ezyang
Copy link
Contributor Author

ezyang commented Apr 1, 2016

OK, so an alternate way to implement this is to use SetupWrapper, and ditch the CWD refactoring. The current patchset only has a modest benefit, which is that it can be done same process even when we are multithreaded (not true for setup wrapper.) @23Skidoo, what do you think?

@23Skidoo
Copy link
Member

23Skidoo commented Apr 1, 2016

I like the current solution a bit better, since there are less moving parts, so unless there's a compelling reason to use setupWrapper (is #403 a concern?), I'd prefer to keep it.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 1, 2016

We must use setupWrapper for Custom Cabal scripts, so I'm going to have to implement that code path anyhow.

@23Skidoo
Copy link
Member

23Skidoo commented Apr 1, 2016

Then it's probably easier to just use setupWrapper uniformly.

@ezyang ezyang force-pushed the nix-local-build branch 2 times, most recently from 1a13145 to 0068715 Compare April 2, 2016 05:27
@ezyang
Copy link
Contributor Author

ezyang commented Apr 2, 2016

This branch now also contains some of @grayjay's fixes, which needed to be merged, were very helpful and should be backported.

@cartazio
Copy link
Contributor

cartazio commented Apr 3, 2016

question: at least as of a few days ago with when i built the nix-local-builds branch, the output for cabal help new-build was the standard(ish) docs for cabal help install. Is that supposed to be, or is the cabal help output semi stale?

@ezyang
Copy link
Contributor Author

ezyang commented Apr 3, 2016

The docs are not up to date at the moment. (although, I think the flag descriptions are accurate)

@ezyang
Copy link
Contributor Author

ezyang commented Apr 5, 2016

Is there an easy way to kick the AppVeyor build here?

@23Skidoo
Copy link
Member

23Skidoo commented Apr 5, 2016

Restarted the build.

ezyang and others added 3 commits April 4, 2016 22:57
As a refactoring, this moves allPackageSourceFiles from
Distribution.Client.Sandbox.Timestamp to
Distribution.Client.SrcDist, makes it thread safe, and has
it return files relative to the source directory.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>
When the solver links a package, it also merges the 'LinkGroup's of the
package's direct dependencies.  This commit causes the solver to skip setup
dependencies, which should remain independent of the package's other
dependencies.
@ezyang ezyang force-pushed the nix-local-build branch from bab0ed1 to f471123 Compare April 5, 2016 05:57
@ezyang
Copy link
Contributor Author

ezyang commented Apr 5, 2016

Merged in.

@ezyang ezyang closed this Apr 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants