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.

Sign up for GitHub

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

Jump to bottom

Fix v2-sdist permissions #6454

Closed
wants to merge 2 commits into from
Closed

Fix v2-sdist permissions #6454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
33336ee
Fix `v2-sdist` permissions
typedrat Dec 21, 2019
4a98eb7
Update changelog
typedrat Dec 21, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions cabal-install/Distribution/Client/CmdSdist.hs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,8 @@ import Data.List
import qualified Data.Set as Set
import System.Directory
( getCurrentDirectory, setCurrentDirectory
, createDirectoryIfMissing, makeAbsolute )
, createDirectoryIfMissing, makeAbsolute
, getPermissions, executable )
import System.FilePath
( (</>), (<.>), makeRelative, normalise, takeDirectory )

Expand Down Expand Up @@ -255,10 +256,11 @@ packageToSdist verbosity projectRootDir format outputFile pkg = do
Right path -> tell [Tar.directoryEntry path]

for_ files $ \(perm, file) -> do
realPerm <- getPermissions file
let fileDir = takeDirectory (prefix </> file)
perm' = case perm of
Exec -> Tar.executableFilePermissions
NoExec -> Tar.ordinaryFilePermissions
Exec | executable realPerm -> Tar.executableFilePermissions
Copy link
Member

Choose a reason for hiding this comment

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

It seems rather odd that we can have files marked as Exec that don't have executable permissions. Would it not be cleaner to change the implementation of listPackageSources so that these files are marked as NoExec instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

On reflection, that's weird. I think, instead of surgically fixing this issue, it might be worth considering a more thorough change to properly read the permissions and set it based on that, and not bother using the cruddy heuristic that we currently have at all, unless there's an important reason I'm not remembering for why we want to ignore some files' permissions.

This does have the downside that changing permissions (and not file contents) will now be able to invalidate package hashes, which I think is the original reason I had it not check.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The culrpit looks to be in SrcDist.hs

-- | List those source files that may be executable (e.g. the configure script).
listPackageSourcesMaybeExecutable :: Verbosity -> PackageDescription -> IO [FilePath]
listPackageSourcesMaybeExecutable verbosity pkg_descr =
  -- Extra source files.
  fmap concat . for (extraSrcFiles pkg_descr) $ \fpath ->
    matchDirFileGlob verbosity (specVersion pkg_descr) "." fpath

i.e. every extraSrcFiles are given executable bits now? e.g. for lens-4.18.1.tar.gz from Hackage:

% tar -tzvf lens-4.18.1.tar.gz|grep README
-rwxr-xr-x 0/0            7427 2001-09-09 04:46 lens-4.18.1/README.markdown

IMHO that's just plain wrong.

What would break if all files are 644 (and directories 755)?

For example, configure script is actually run by invoking sh on it, see runConfigureScript

  shConfiguredProg <- lookupProgram shProg
                      `fmap` configureProgram  verbosity shProg progDb
  case shConfiguredProg of
      Just sh -> runProgramInvocation verbosity $
                 (programInvocation (sh {programOverrideEnv = overEnv}) args')
                 { progInvokeCwd = Just (buildDir lbi) }
      Nothing -> die' verbosity notFoundMsg

(and #5813 (comment) suggest that if we unpack without executable bits, why store them in the first place).

I would be worried of invalidating cache. It's cache for a reason.

_ -> Tar.ordinaryFilePermissions
needsEntry <- gets (Set.notMember fileDir)

when needsEntry $ do
Expand Down
2 changes: 2 additions & 0 deletions cabal-install/changelog
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
-*-change-log-*-

3.1.0.0 (current development version)
* `v2-sdist` no longer gives files that are not already executable extra permissions
when generating a tarball. (#5813, #6454)
* `v2-build` (and other `v2-`prefixed commands) now accept the
`--benchmark-option(s)` flags, which pass options to benchmark executables
(analogous to how `--test-option(s)` works). (#6209)
Expand Down