Add new-sdist command by typedrat · Pull Request #5389 · haskell/cabal

typedrat · 2018-06-19T03:15:46Z

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 · 2018-06-19T08:29:15Z

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 · 2018-06-19T08:29:42Z

@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 · 2018-06-19T08:31:30Z

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 · 2018-06-19T09:01:58Z

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 · 2018-06-19T10:18:37Z

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

typedrat · 2018-06-19T12:09:16Z

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 · 2018-06-20T02:06:38Z

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

quasicomputational

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.

quasicomputational · 2018-06-20T09:25:26Z

+        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


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

quasicomputational · 2018-06-20T09:29:06Z

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


--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$

Whoops, why is it putStrLn?

quasicomputational · 2018-06-20T09:30:49Z

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


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

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.

quasicomputational · 2018-06-20T09:35:15Z

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


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

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

quasicomputational · 2018-06-20T09:35:50Z

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


Missing the component name argument.

quasicomputational · 2018-06-20T09:39:30Z

                       # >= 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


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.

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).

quasicomputational · 2018-06-20T09:41:09Z

          | 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 ]


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

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

quasicomputational · 2018-06-20T09:42:13Z

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


s/a package/one or more packages/?

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

quasicomputational · 2018-06-20T09:52:14Z

+``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.


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

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

quasicomputational · 2018-06-20T09:53:31Z

+- ``--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/``.


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.

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

quasicomputational · 2018-06-20T12:20:19Z

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 · 2018-06-20T14:33:54Z

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 · 2018-06-20T14:43:53Z

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 · 2018-06-20T15:16:05Z

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 · 2018-06-20T16:21:55Z

I will write more tests later today.

quasicomputational · 2018-06-20T16:28:03Z

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.

[ci skip]

They're not all passing, but I think that that's a bug in the code rather than the tests.

typedrat · 2018-06-20T20:05:35Z

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

typedrat · 2018-06-21T06:44:40Z

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

quasicomputational · 2018-06-21T10:37:58Z

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.

23Skidoo

LGTM modulo minor comments.

23Skidoo · 2018-06-24T22:29:23Z

+
+``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.


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.

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.

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

23Skidoo · 2018-06-24T22:30:10Z

+- ``-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


OK from me, let's deprecate them.

23Skidoo · 2018-06-24T22:31:48Z

 -*-change-log-*-

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


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.

23Skidoo · 2018-06-24T22:35:40Z

  }

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


Should point the user towards what they should use instead.

23Skidoo · 2018-06-24T22:37:59Z

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


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.

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

23Skidoo · 2018-06-24T22:44:46Z

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


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

23Skidoo · 2018-06-24T22:49:10Z

+                | listSources -> "-"
+                | otherwise   -> distSdistFile distLayout (packageId pkg) archiveFormat
+
+    createDirectoryIfMissing True (distSdistDirectory distLayout)


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

23Skidoo · 2018-06-24T22:53:41Z

+            | otherwise ->
+                mapM_ (\pkg -> packageToSdist verbosity (distProjectRootDirectory distLayout) format (outputPath pkg) pkg) pkgs
+
+data IsExec = Exec | NoExec


No boolean blindness, nice!

23Skidoo · 2018-06-24T23:02:24Z

+
+        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."


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

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.

23Skidoo · 2018-06-24T23:03:37Z

+        listPackageSources verbosity (flattenPackageDescription $ packageDescription pkg) knownSuffixHandlers
+
+    let write = if outputFile == "-"
+          then putStr . withOutputMarker verbosity . BSL.unpack


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

Add new-sdist command.

483cfe6

typedrat added the cabal-install: v2-build system Affecting v2-build and related commands that use v2-architecture (aka "nix local builds"). label Jun 19, 2018

typedrat self-assigned this Jun 19, 2018

typedrat requested review from 23Skidoo and hvr June 19, 2018 03:15

typedrat added 3 commits June 19, 2018 00:13

Fix mystery errors. No idea how I managed to build this earlier.

2bdbcaa

Add zip sdist support

40b6856

Add new-sdist documentation [ci skip]

2282c6e

Deprecate sdist user hooks

55dfc4f

typedrat added 2 commits June 19, 2018 05:54

Add test for base case

8680872

Make test present errors as hex

ef8ae9c

typedrat force-pushed the new-sdist branch from 886a500 to a62f4b9 Compare June 20, 2018 01:03

Normalize creator bytes so it is truly correct

71dcfef

typedrat force-pushed the new-sdist branch from a62f4b9 to 71dcfef Compare June 20, 2018 01:34

typedrat changed the title ~~[WIP] Add new-sdist command.~~ Jun 20, 2018

quasicomputational reviewed Jun 20, 2018

View reviewed changes

typedrat added 2 commits June 20, 2018 08:50

Fix small problems

79d164f

Add option to null-terminate filenames

d7cfcb1

typedrat force-pushed the new-sdist branch from 0453e8e to d7cfcb1 Compare June 20, 2018 16:20

quasicomputational added 4 commits June 20, 2018 10:14

Docs: short flags, -z, -o - behaviour

2a75135

[ci skip]

Missing space in error

7a9c971

[ci skip]

Some tests for new-sdist

543c7a3

They're not all passing, but I think that that's a bug in the code rather than the tests.

Fix up the new-sdist tests.

1a9b3d0

quasicomputational added 3 commits June 21, 2018 09:57

Make sure that all failure paths in new-sdist are tested

eed2a25

This one wasn't actually testing what it said it was testing. Oops.

Test that new-sdist gets globbing right

dad2e90

The code does the right thing, but it's slightly fragile since it depends on directory switching, so make sure it stays right.

Fix the Appveyor failure

10b54da

quasicomputational mentioned this pull request Jun 22, 2018

Applying flag normalisation to local packages. #5287

Merged

4 tasks

23Skidoo approved these changes Jun 24, 2018

View reviewed changes

typedrat added 2 commits June 24, 2018 20:15

Address last nitpicks

81e297d

Add to Cabal changelog

40bb3be

typedrat merged commit 4bdf54d into haskell:master Jun 25, 2018

typedrat mentioned this pull request Jun 30, 2018

setup sdist places implementation-dependent preprocessor output in the source bundle #130

Closed

RyanGlScott mentioned this pull request Oct 8, 2018

gl and cabal new-build don't mix ekmett/gl#17

Closed

andreasabel mentioned this pull request Feb 16, 2022

The changelog is mute about v2-cabal #7985

Closed

Uh oh!

Conversation

typedrat commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

typedrat commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

hvr commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

typedrat commented Jun 19, 2018

typedrat commented Jun 19, 2018

typedrat commented Jun 19, 2018

typedrat commented Jun 19, 2018

typedrat commented Jun 20, 2018

quasicomputational left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

typedrat Jun 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

quasicomputational commented Jun 20, 2018

quasicomputational commented Jun 20, 2018

quasicomputational commented Jun 20, 2018

typedrat commented Jun 20, 2018

typedrat commented Jun 20, 2018

quasicomputational commented Jun 20, 2018

typedrat commented Jun 20, 2018

typedrat commented Jun 21, 2018

quasicomputational commented Jun 21, 2018

23Skidoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Labels

4 participants

typedrat commented Jun 19, 2018 •

edited

Loading

typedrat commented Jun 19, 2018 •

edited

Loading

hvr commented Jun 19, 2018 •

edited

Loading

typedrat Jun 20, 2018 •

edited

Loading