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

Add new-sdist command #5389

Merged
merged 19 commits into from Jun 25, 2018
Merged

Add new-sdist command #5389

merged 19 commits into from Jun 25, 2018

Conversation

typedrat
Copy link
Collaborator

@typedrat typedrat commented Jun 19, 2018

new-sdist is done, and so are the tests (thanks @quasicomputational). There are some unfortunate implications I don't see a way around in terms of how new-style builds work, meaning that the sdist hooks are dead. Fortunately, I don't see many cases this will actually effect. I only found three packages in a representative sample that would be "broken" (at sdist time, has no effects on end users of these packages) and honestly it'd not be much sweat off my back to fix them myself.

Status:

  • .tar.gz sdists
  • .zip sdists
  • Listing source files
  • Works with custom setup (in a sense)

Closes #4293, but does not touch #4047 or #4675, previously thought to be required. It solves the issue by simply not running preprocessors and not running Setup configure under the logic that whoever is downloading this sdist will be able to do it themselves in all likelihood. Doing otherwise would make it impossible to insure that the same source files will generate an identical sdist, which is required for new-install to work.

Please include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • If the change is docs-only, [ci skip] is used to avoid triggering the build bots.

@typedrat
Copy link
Collaborator Author

typedrat commented Jun 19, 2018

It is stable at least on the same machine:

❯ shasum -a 256 dist-newstyle/sdist/Cabal-2.3.0.0.tar.gz ../test/cabal/dist-newstyle/sdist/Cabal-2.3.0.0.tar.gz
1e70454db3e883ef791fc851c63933d0017c790b0f592d7b9e7efeaf8a144042  dist-newstyle/sdist/Cabal-2.3.0.0.tar.gz
1e70454db3e883ef791fc851c63933d0017c790b0f592d7b9e7efeaf8a144042  ../test/cabal/dist-newstyle/sdist/Cabal-2.3.0.0.tar.gz

@hvr
Copy link
Member

hvr commented Jun 19, 2018

@23Skidoo this is kinda orthogonal, but do you know who actually benefits from .zip support? and why we're keeping it around? Afaik we don't support package repos with .zip files and I wonder if cabal install pkg-1.2.3.zip ever worked...

@typedrat
Copy link
Collaborator Author

Either way, it's there now at the cost of four (soon to be two, once jgm/zip-archive#47 goes through) dependencies for cabal-install.

@typedrat
Copy link
Collaborator Author

Formally note down:

  • Invariant that custom Setups can't change files in such a way that the file generated by cabal new-sdist changes
  • Distribution.Simple.UserHooks.UserHooks{preSDist, sDistHook, postSDist} are/will be deprecated now, because they interfere with the Glorious Deterministic Future. A replacement is needed, probably some sort of script hooks.

@typedrat
Copy link
Collaborator Author

I need to add tests for new-sdist, to confirm the determinicity.

@typedrat
Copy link
Collaborator Author

To a first approximation, if the sdist hooks completely disppeared tomorrow (and obviously it's more gradual than that), the following (actively maintained and used enough to get into certain curated systems) packages would break:

  • gl-0.8.0
  • hlibsass-0.1.6.1
  • ShellCheck-0.4.7

I've basically managed to fix gl, I'd try the other two but I do need some sleep. That's 0.12% of the packages in my sample (latest Stackage LTS).

@typedrat
Copy link
Collaborator Author

While I understand there are some weighty elements to this, at the same time it is blocking new-install.

@typedrat typedrat changed the title [WIP] Add new-sdist command. Add new-sdist command Jun 20, 2018
Copy link
Contributor

@quasicomputational quasicomputational left a comment

Choose a reason for hiding this comment

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

Looks good. Left some comments but they're just little niggles. Wouldn't mind having a test exercising --list-only, and maybe also the non-straightforward target selector cases: multiple valid selectors, all, some valid + an invalid one, and exercise all the invalid cases.

No comment on the over-all design or the hooks stuff, which I don't understand well enough to have an informed opinion on.

listPackageSources verbosity (flattenPackageDescription $ packageDescription pkg) knownSuffixHandlers

let write = if outputFile == "-" then BSL.putStrLn else BSL.writeFile outputFile
files = nub . sortBy (\(_, a) (_, b) -> compare a b) $ nonexec ++ exec
Copy link
Contributor

Choose a reason for hiding this comment

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

sortBy (\(_, a) (_, b) -> compare a b) === sortOn snd

if
| listSources -> do
notice verbosity $ "File manifest for package " ++ prettyShow (packageId pkg) ++ ":\n"
write (BSL.pack . unlines . fmap snd $ files)
Copy link
Contributor

Choose a reason for hiding this comment

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

--list-only is producing an extra newline at the end of its output, e.g.:

~/src/cabal$ cabal new-run cabal -- new-sdist Cabal --list-only
[...]
tests/custom-setup/IdrisSetup.hs
tests/hackage/check.sh
tests/hackage/download.sh
tests/hackage/unpack.sh
tests/misc/ghc-supported-languages.hs

~/src/cabal$ 

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, why is it putStrLn?

perm' = case perm of
Exec -> Tar.executableFilePermissions
NoExec -> Tar.ordinaryFilePermissions
needsEntry <- gets (notElem fileDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

Quadratic behaviour here in the number of files. Better to use a Set.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I generally don't feel like being too aggressively optimizing before we know it's an issue, but adding four characters to go from O(n^2) to O(n log n) is worth it.

[ optionVerbosity
sdistVerbosity (\v flags -> flags { sdistVerbosity = v })
, optionDistPref
sdistDistDir (\dd flags -> flags { sdistDistDir = dd })
Copy link
Contributor

Choose a reason for hiding this comment

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

This is claiming its default is dist, but it actually plops the tarball in dist-newstyle.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's #5380 and it's its own issue. optionDistPref needs fixing, but that's neither here nor there for this patchset.

{ commandName = "new-sdist"
, commandSynopsis = "Generate a source distribution file (.tar.gz)."
, commandUsage = \pname ->
"Usage: " ++ pname ++ " new-sdist [FLAGS]\n"
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing the component name argument.

@@ -261,6 +261,15 @@ HACKAGE_SECURITY_VER="0.5.3.0"; HACKAGE_SECURITY_VER_REGEXP="0\.5\.((2\.[2-9]|[3
# >= 0.5.2 && < 0.6
TAR_VER="0.5.1.0"; TAR_VER_REGEXP="0\.5\.([1-9]|1[0-9]|0\.[3-9]|0\.1[0-9])\.?"
# >= 0.5.0.3 && < 0.6
# These two are temporary: https://github.com/jgm/zip-archive/pull/47
Copy link
Contributor

Choose a reason for hiding this comment

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

Not actually a blocker for merging, right? I'd be OK with merging as-is and then fixing up when the PR is merged upstream.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nope, not a blocker. That's to remind us when it does get merged (hopefully, I don't see a reason it wouldn't be).

@@ -282,6 +282,7 @@ cabalG' global_args cmd args = do
| cmd `elem` ["v1-update", "outdated", "user-config", "manpage", "v1-freeze", "check"]
= [ ]
-- new-build commands are affected by testCabalProjectFile
| cmd == "new-sdist" = [ "--project-file", testCabalProjectFile env ]
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this covered by the next case, catching all the new- commands?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It can’t take and is confused by ‘-j1’.

, commandUsage = \pname ->
"Usage: " ++ pname ++ " new-sdist [FLAGS]\n"
, commandDescription = Just $ \_ ->
"Generates a tarball of a package suitable for upload to Hackage."
Copy link
Contributor

Choose a reason for hiding this comment

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

s/a package/one or more packages/?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whole thing needs to be pluralized, since there's one package per tarball but it can do multiple tarballs in one run.

``cabal new-sdist`` takes the following flags:

- ``--list-only``: Rather than creating an archive, lists files that would be included.
Output is to ``stdout`` by default.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should mention it takes an optional output file argument and that making its argument - sends it to stdout.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, sorry, I got confused. This is nonsense.

- ``--zip``: Output an archive in ``.zip`` format.

- ``--output-dir``: Sets the output dir, if a non-default one is desired. The default is
``dist-newstyle/sdist/``.
Copy link
Contributor

Choose a reason for hiding this comment

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

It's only dist-newstyle by default, and that's controlled by a separate flag; if --builddir=foo, then it's plopped in foo/sdist.

Copy link
Collaborator Author

@typedrat typedrat Jun 20, 2018

Choose a reason for hiding this comment

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

I mean, it is the default, and I feel like you'd know if it wasn't. BUILDDIR/sdist/ seems harder.

@quasicomputational
Copy link
Contributor

Oh, one more thing: can --list-only grow a -z / --null option, that will separate file names with a NUL byte rather than a newline, so that anything processing the file list downstream doesn't break when files have newlines in their name?

@quasicomputational
Copy link
Contributor

Ah, found a bug: cabal new-sdist Cabal --output-dir . drops the tarball in Cabal/. That should be relative to the CWD of the invocation. Same goes for cabal new-sdist Cabal --output-dir foo, where it'll try to put it in Cabal/foo, rather than ./foo

@quasicomputational
Copy link
Contributor

Sorry, I spotted another thing on second look: cabal new-sdist Cabal --list-only -o - does not work as advertised in the docs: it tries to write to -/Cabal-2.3.0.0.list, rather than stdout. I'm also not sure that -o - is sensible for generating multiple tarballs; maybe it should error out in that case?

@typedrat
Copy link
Collaborator Author

I forgot to make the base path absolute before we start hopping around, that's trivial to fix and... Oh wow I have no idea how I managed to not catch that.

@typedrat
Copy link
Collaborator Author

I will write more tests later today.

@quasicomputational
Copy link
Contributor

I can chip in with a bunch as well. Thanks for the changes so far; I think that tests and a little bit more documenting (which I'll prepare a PR into your branch for) and then it's in great shape in terms of nuts and bolts.

They're not all passing, but I think that that's a bug in the code
rather than the tests.
@typedrat
Copy link
Collaborator Author

I gotta say, huge huge props to @quasicomputational for how much help they've been today.

@typedrat
Copy link
Collaborator Author

The Appveyor failure is specious, it's ready to merge unless there is a substantial yet heretofore unvoiced objection.

@quasicomputational
Copy link
Contributor

The AppVeyor failure is actually half-legit: it's testing for a path with slashes in the .list file, but of course it's backslashes on Windows. I managed to make that exact same mistake a few weeks ago and apparently didn't learn from it the first time. typedrat#5 fixes that and makes a few other tweaks to the tests; with that merged I'm happy with the test coverage and the code itself.

This one wasn't actually testing what it said it was testing. Oops.
The code does the right thing, but it's slightly fragile since it
depends on directory switching, so make sure it stays right.
Copy link
Member

@23Skidoo 23Skidoo left a comment

Choose a reason for hiding this comment

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

LGTM modulo minor comments.


``cabal new-sdist [FLAGS] [TARGETS]`` takes the crucial files needed to build ``TARGETS``
and puts them into an archive format ready for upload to Hackage. These archives are stable
and two archives of the same format built from the same source will hash to the same value.
Copy link
Member

Choose a reason for hiding this comment

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

For autogen-modules we can't guarantee this 100% since we have no control over non-determinism in preprocessors, especially custom ones. For example, a preprocessor can add a comment with the date when the file was generated, or have output that depends on system locale settings. I guess that we can settle on saying that the behaviour is undefined when a preprocessor is non-deterministic in a way that can be observed from compiled code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

autogen-modules shouldn't (and it's a bug if they are, that probably needs a test to be sure) be included for this very reason.

Copy link
Member

Choose a reason for hiding this comment

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

You're correct! Sorry, it's getting a bit late here.

- ``-z``, ``--null``: Only used with ``--list-only``. Separates filenames with a NUL
byte instead of newlines.

``new-sdist`` is inherently incompatible with sdist hooks, not due to implementation but due
Copy link
Member

Choose a reason for hiding this comment

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

OK from me, let's deprecate them.

@@ -1,6 +1,8 @@
-*-change-log-*-

2.4.0.0 (current development version)
* Add 'new-sdist' command (#5389). Creates stable archives based on
Copy link
Member

Choose a reason for hiding this comment

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

Please also add a note to the Cabal changelog saying that sdist hooks are now deprecated and autogen-modules and build-tool-depends should be used instead.

@@ -165,6 +165,9 @@ data UserHooks = UserHooks {
}

{-# DEPRECATED runTests "Please use the new testing interface instead!" #-}
{-# DEPRECATED preSDist "SDist hooks violate the invariants of new-sdist." #-}
Copy link
Member

Choose a reason for hiding this comment

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

Should point the user towards what they should use instead.

{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE TupleSections #-}
{-# LANGUAGE ViewPatterns #-}
module Distribution.Client.CmdSdist ( sdistCommand, sdistAction, packageToSdist, OutputFormat(..), ArchiveFormat(..) ) where
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: please try to keep all lines <= 80 chars long. I know that this rule is not followed 100%, but we should try to follow it in new code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I typically don't care much about that, but that is getting a bit silly.


sdistAction :: SdistFlags -> [String] -> GlobalFlags -> IO ()
sdistAction SdistFlags{..} targetStrings globalFlags = do
let verbosity = fromFlagOrDefault normal sdistVerbosity
Copy link
Member

Choose a reason for hiding this comment

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

If you use Emacs, you can make formatting prettier here (and elsewhere) with M-x align-regexp =. If you don't, don't bother.

| listSources -> "-"
| otherwise -> distSdistFile distLayout (packageId pkg) archiveFormat

createDirectoryIfMissing True (distSdistDirectory distLayout)
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking, this is not needed when outputting to stdout, but no-one probably cares.

| otherwise ->
mapM_ (\pkg -> packageToSdist verbosity (distProjectRootDirectory distLayout) format (outputPath pkg) pkg) pkgs

data IsExec = Exec | NoExec
Copy link
Member

Choose a reason for hiding this comment

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

No boolean blindness, nice!


getPkg pid = case find ((== pid) . packageId) pkgs' of
Just pkg -> Right pkg
Nothing -> error "The impossible happened: we have a reference to a local package that isn't in localPackages."
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add a TargetProblemImpossible and use it instead of error here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Didn't do it since it's harder to get rid of the one in flatten and I don't feel like it's worth the work when there's another five lines above.

listPackageSources verbosity (flattenPackageDescription $ packageDescription pkg) knownSuffixHandlers

let write = if outputFile == "-"
then putStr . withOutputMarker verbosity . BSL.unpack
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a helper function for this to D.S.Utils?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

sdist fails with non-installed custom-depends:
4 participants