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

Unmark files as executable that can't actually be executed. #15353

Merged
merged 2 commits into from
Aug 21, 2019

Conversation

gnprice
Copy link
Contributor

@gnprice gnprice commented Aug 21, 2019

There are plenty of legitimate scripts in the tree that begin with a
#!, but also a few that seem to be marked executable by mistake.

Found them with this command -- it gets executable files known to Git,
filters to the ones that don't start with a #!, and then unmarks
them as executable:

$ git ls-files --stage \
  | perl -lane 'print $F[3] if (!/^100644/)' \
  | while read f; do
      head -c2 "$f" | grep -qxF '#!' \
      || chmod a-x "$f"; \
    done

Looking at the list by hand confirms that we didn't sweep up any
files that should have the executable bit after all. In particular

  • The .psd files are images from Photoshop.

  • The .bat files sure look like things that can be run.
    But we have lots of other .bat files, and they don't have
    this bit set, so it must not be needed for them.

Automerge-Triggered-By: @benjaminp

There are plenty of legitimate scripts in the tree that begin with a
`#!`, but also a few that seem to be marked executable by mistake.

Found them with this command -- it gets executable files known to Git,
filters to the ones that don't start with a `#!`, and then unmarks
them as executable:

  $ git ls-files --stage \
    | perl -lane 'print $F[3] if (!/^100644/)' \
    | while read f; do
        head -c2 "$f" | grep -qxF '#!' \
        || chmod a-x "$f"
      done

Looking at the list by hand confirms that we didn't sweep up any
files that should have the executable bit after all.  In particular

 * The `.psd` files are images from Photoshop.

 * The `.bat` files sure look like things that can be run.
   But we have lots of other `.bat` files, and they don't have
   this bit set, so it must not be needed for them.
@gnprice
Copy link
Contributor Author

gnprice commented Aug 21, 2019

@benjaminp ^ -- I noticed this after your d33e46d fixed another such file.

gnprice added a commit to gnprice/cpython that referenced this pull request Aug 21, 2019
This is the converse of pythonGH-15353 -- in addition to plenty of
scripts in the tree that are marked with the executable bit
(and so can be directly executed), there are a few that have
a leading `#!` which could let them be executed, but it doesn't
do anything because they don't have the executable bit set.

Here's a command which finds such files and marks them.  The
first line finds files in the tree with a `#!` line *anywhere*;
the next-to-last step checks that the *first* line is actually of
that form.  In between we filter out files that already have the
bit set, and some files that are meant as fragments to be
consumed by one or another kind of preprocessor.

    $ git grep -l '^#!' \
      | grep -vxFf <( \
          git ls-files --stage \
          | perl -lane 'print $F[3] if (!/^100644/)' \
        ) \
      | grep -ve '\.in$' -e '^Doc/includes/' \
      | while read f; do
          head -c2 "$f" | grep -qxF '#!' \
          && chmod a+x "$f"; \
        done
@gnprice
Copy link
Contributor Author

gnprice commented Aug 21, 2019

@benjaminp Thanks! Just sent the converse too, as #15354 .

@terryjreedy
Copy link
Member

On Windows, a file is 'executable' if is has the right registry entries for the extension. I don't think that Windows pays any more attention to the execute bit that is does to #! lines. So I don't care about it either way for idle.bat, and don't care whether you backport the change.

@miss-islington
Copy link
Contributor

@gnprice: Status check is done, and it's a failure ❌ .

1 similar comment
@miss-islington
Copy link
Contributor

@gnprice: Status check is done, and it's a failure ❌ .

@gnprice
Copy link
Contributor Author

gnprice commented Aug 21, 2019

@terryjreedy , thanks for that confirmation!

Meanwhile, this failed one of the test builds -- specifically "Ubuntu PR Tests" in the Azure pipelines. Here's the log:

Starting: Install dependencies
==============================================================================
Task         : Command line
Description  : Run a command line script using Bash on Linux and macOS and cmd.exe on Windows
Version      : 2.151.2
Author       : Microsoft Corporation
Help         : https://docs.microsoft.com/azure/devops/pipelines/tasks/utility/command-line
==============================================================================
Generating script.
Script contents:
sudo ./.azure-pipelines/posix-deps-apt.sh 1.1.1c
========================== Starting Command Output ===========================
in/bash --noprofile --norc /home/vsts/work/_temp/ba52b7c9-dcd5-438a-a36c-de38a96384aa.sh
sudo: ./.azure-pipelines/posix-deps-apt.sh: command not found
##[error]Bash exited with code '1'.

I... am puzzled that that's been working with no #! line. Easy to fix, though. To avoid confusion I'll add such a line, while also marking it executable again.

@miss-islington
Copy link
Contributor

@gnprice: Status check is done, and it's a failure ❌ .

@miss-islington
Copy link
Contributor

@gnprice: Status check is done, and it's a success ✅ .

@miss-islington miss-islington merged commit 9ece4a5 into python:master Aug 21, 2019
@gnprice gnprice deleted the pr-unexecutable branch August 21, 2019 05:36
Copy link
Contributor

@skrah skrah left a comment

Choose a reason for hiding this comment

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

Surely runall.bat is executable. Please revert.

@gnprice
Copy link
Contributor Author

gnprice commented Aug 21, 2019

Surely runall.bat is executable.

Yes. As I mentioned in the commit message / PR description:

  • The .bat files sure look like things that can be run.
    But we have lots of other .bat files, and they don't have
    this bit set, so it must not be needed for them.

Specifically, these (after the change):

$ git ls-files --stage | perl -lane 'print "$F[0] $F[3]" if (/\.bat$/)'
100644 Doc/make.bat
100644 Lib/ctypes/macholib/fetch_macholib.bat
100644 Lib/idlelib/idle.bat
100644 Lib/venv/scripts/nt/activate.bat
100644 Lib/venv/scripts/nt/deactivate.bat
100644 Modules/_decimal/tests/runall.bat
100644 PC/bdist_wininst/build.bat
100644 PCbuild/build.bat
100644 PCbuild/build_env.bat
100644 PCbuild/clean.bat
100644 PCbuild/env.bat
100644 PCbuild/find_msbuild.bat
100644 PCbuild/find_python.bat
100644 PCbuild/get_externals.bat
100644 PCbuild/idle.bat
100644 PCbuild/prepare_libffi.bat
100644 PCbuild/prepare_ssl.bat
100644 PCbuild/prepare_tcltk.bat
100644 PCbuild/rt.bat
100644 Tools/buildbot/build.bat
100644 Tools/buildbot/buildmsi.bat
100644 Tools/buildbot/clean.bat
100644 Tools/buildbot/remoteDeploy.bat
100644 Tools/buildbot/remotePythonInfo.bat
100644 Tools/buildbot/test.bat
100644 Tools/msi/build.bat
100644 Tools/msi/buildrelease.bat
100644 Tools/msi/get_externals.bat
100644 Tools/msi/testrelease.bat
100644 Tools/msi/uploadrelease.bat
100644 Tools/nuget/build.bat
100644 Tools/unicode/genwincodecs.bat

Many if not most of those are intended to be directly executed too. Yet before this change just two of them had the executable bit set in Git -- on the rest, our practice has been to leave it unset. Terry explained above why the executable bit doesn't matter for batch files. On those two, this change just brought them in line with our practice on the rest.

If you'd like to change that practice, I think that would be best as a separate change -- adding the executable bit to all the batch files that are meant to be directly executed.

(Though I think the current practice is the cleaner one: given that it doesn't affect actually executing the files, setting it seems like it would just be misleading.)

@skrah
Copy link
Contributor

skrah commented Aug 21, 2019

"Our practice"? I''m the author of Modules/_decimal/*, and my practice is to set the executable bit if there's a .sh file with an exact .bat counterpart. :)

I'm developing on Linux and I want to see the same color in directory listings when two files are, Windows practices notwithstanding, meant to be executed directly and do the same thing.

If you'd like to change that practice [...]

There doesn't need to be an overarching policy for that kind of unimportant matters. You are the one trying to establish such a practice.

@gnprice
Copy link
Contributor Author

gnprice commented Aug 21, 2019

Well, if you have a strong preference for the executable bit to be set on that file, then please go ahead and set it back. What to do with batch files in this respect isn't a thing I feel strongly about.

I'd encourage you if so to look at the other batch files in the tree (listed above) at the same time, and see if you think the same should apply to some of them.

I should perhaps clarify my meaning on this point, though:

If you'd like to change that practice [...]

There doesn't need to be an overarching policy for that kind of unimportant matters.

Policy != practice. Practice is just whatever is actually done; there is always a practice. In referring to "our practice on the rest" of the batch files, I'm not imagining any policy; I'm just summarizing the facts of what we actually do in the case of the other batch files.

Generally in a shared codebase I think it's helpful for practices to be consistent across the codebase. But, as you say, this one is just not that important. If you prefer a different practice which you don't care to try to apply to PCbuild/ and Tools/msi/ and the rest, then I don't expect anyone will care enough to object.

@skrah
Copy link
Contributor

skrah commented Aug 21, 2019 via email

@terryjreedy
Copy link
Member

terryjreedy commented Aug 21, 2019

Multi-owner patches usually sit around at least a few days, giving people time to object. This one was handled, it appears, super efficiently, within a couple of hours. You, Benjamin, and I all lacked the imagination to think of why someone might object, and that the merging should wait a couple of days.

idle.bat was added by David Scherer in 2000, and scarcely touched since. I suspect David set it, but have no idea why. A typical Windows developer would never notice that setting.

@skrah
Copy link
Contributor

skrah commented Aug 21, 2019

Thank you Terry, I've already reverted it in the meantime, so there's no need for further action.

@gnprice
Copy link
Contributor Author

gnprice commented Aug 22, 2019

[Terry] Multi-owner patches usually sit around at least a few days, giving people time to object. This one was handled, it appears, super efficiently, within a couple of hours. You, Benjamin, and I all lacked the imagination to think of why someone might object, and that the merging should wait a couple of days.

Thanks for that context. Yeah, I saw the "code owners" line up at the top of the thread with a medium-long list of people, but don't yet have a firm sense of how that's interpreted in this project.

I will certainly take this thread as data to predict a greater probability of there being an objection I didn't have the imagination to think of, even on a patch as minor as this one. Benjamin has quite a lot more past data here than I do 🙂, but perhaps his predictions will shift a bit too.

[Stefan] I've already reverted it in the meantime, so there's no need for further action.

Cool, and thanks for leaving the rest of the patch in place. Glad we're all set.

(For convenient reference for anyone else reading the thread: it looks like that commit is bcc446f .)

@terryjreedy
Copy link
Member

idlelib is another special directory in that nearly all PRs get backported unchanged to current 3.x versions (PEP 434). Github's code owner mechanism sends me an email about any patch, like this one, touching it. When people could patch it without me noticing, it was occasionally a nuisance.

Yhg1s pushed a commit that referenced this pull request Sep 9, 2019
This is the converse of GH-15353 -- in addition to plenty of
scripts in the tree that are marked with the executable bit
(and so can be directly executed), there are a few that have
a leading `#!` which could let them be executed, but it doesn't
do anything because they don't have the executable bit set.

Here's a command which finds such files and marks them.  The
first line finds files in the tree with a `#!` line *anywhere*;
the next-to-last step checks that the *first* line is actually of
that form.  In between we filter out files that already have the
bit set, and some files that are meant as fragments to be
consumed by one or another kind of preprocessor.

    $ git grep -l '^#!' \
      | grep -vxFf <( \
          git ls-files --stage \
          | perl -lane 'print $F[3] if (!/^100644/)' \
        ) \
      | grep -ve '\.in$' -e '^Doc/includes/' \
      | while read f; do
          head -c2 "$f" | grep -qxF '#!' \
          && chmod a+x "$f"; \
        done
lisroach pushed a commit to lisroach/cpython that referenced this pull request Sep 10, 2019
…-15353)

There are plenty of legitimate scripts in the tree that begin with a
`#!`, but also a few that seem to be marked executable by mistake.

Found them with this command -- it gets executable files known to Git,
filters to the ones that don't start with a `#!`, and then unmarks
them as executable:

    $ git ls-files --stage \
      | perl -lane 'print $F[3] if (!/^100644/)' \
      | while read f; do
          head -c2 "$f" | grep -qxF '#!' \
          || chmod a-x "$f"; \
        done

Looking at the list by hand confirms that we didn't sweep up any
files that should have the executable bit after all.  In particular

 * The `.psd` files are images from Photoshop.

 * The `.bat` files sure look like things that can be run.
   But we have lots of other `.bat` files, and they don't have
   this bit set, so it must not be needed for them.



Automerge-Triggered-By: @benjaminp
DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
…-15353)

There are plenty of legitimate scripts in the tree that begin with a
`#!`, but also a few that seem to be marked executable by mistake.

Found them with this command -- it gets executable files known to Git,
filters to the ones that don't start with a `#!`, and then unmarks
them as executable:

    $ git ls-files --stage \
      | perl -lane 'print $F[3] if (!/^100644/)' \
      | while read f; do
          head -c2 "$f" | grep -qxF '#!' \
          || chmod a-x "$f"; \
        done

Looking at the list by hand confirms that we didn't sweep up any
files that should have the executable bit after all.  In particular

 * The `.psd` files are images from Photoshop.

 * The `.bat` files sure look like things that can be run.
   But we have lots of other `.bat` files, and they don't have
   this bit set, so it must not be needed for them.



Automerge-Triggered-By: @benjaminp
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
…-15353)

There are plenty of legitimate scripts in the tree that begin with a
`#!`, but also a few that seem to be marked executable by mistake.

Found them with this command -- it gets executable files known to Git,
filters to the ones that don't start with a `#!`, and then unmarks
them as executable:

    $ git ls-files --stage \
      | perl -lane 'print $F[3] if (!/^100644/)' \
      | while read f; do
          head -c2 "$f" | grep -qxF '#!' \
          || chmod a-x "$f"; \
        done

Looking at the list by hand confirms that we didn't sweep up any
files that should have the executable bit after all.  In particular

 * The `.psd` files are images from Photoshop.

 * The `.bat` files sure look like things that can be run.
   But we have lots of other `.bat` files, and they don't have
   this bit set, so it must not be needed for them.



Automerge-Triggered-By: @benjaminp
websurfer5 pushed a commit to websurfer5/cpython that referenced this pull request Jul 20, 2020
This is the converse of pythonGH-15353 -- in addition to plenty of
scripts in the tree that are marked with the executable bit
(and so can be directly executed), there are a few that have
a leading `#!` which could let them be executed, but it doesn't
do anything because they don't have the executable bit set.

Here's a command which finds such files and marks them.  The
first line finds files in the tree with a `#!` line *anywhere*;
the next-to-last step checks that the *first* line is actually of
that form.  In between we filter out files that already have the
bit set, and some files that are meant as fragments to be
consumed by one or another kind of preprocessor.

    $ git grep -l '^#!' \
      | grep -vxFf <( \
          git ls-files --stage \
          | perl -lane 'print $F[3] if (!/^100644/)' \
        ) \
      | grep -ve '\.in$' -e '^Doc/includes/' \
      | while read f; do
          head -c2 "$f" | grep -qxF '#!' \
          && chmod a+x "$f"; \
        done
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.

7 participants