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

Fix v2-sdist permissions #6454

Closed
wants to merge 2 commits into from
Closed

Fix v2-sdist permissions #6454

wants to merge 2 commits into from

Conversation

typedrat
Copy link
Collaborator

@typedrat typedrat commented Dec 21, 2019

Fixes #5813.


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.

Please also shortly describe how you tested your change. Bonus points for added tests!

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.

@phadej
Copy link
Collaborator

phadej commented Apr 7, 2020

merged as part of #6666

@phadej phadej closed this Apr 7, 2020
@phadej phadej mentioned this pull request Jul 10, 2020
@phadej phadej added this to the 3.4.0.0-rc1 milestone Jul 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

new-sdist adds executable permission to *.txt files
3 participants