#738 Mangling shebangs
Closed: accepted a year ago Opened a year ago by churchyard.

Guidelines change proposal

Is in https://pagure.io/packaging-committee/issue/738#comment-490366

Original post follows

I'd like to propose that we automatically mangle shebangs. The motivation for this is to get rid of unversioned python shebangs (and convert them to explicit python2 shebangs).

There are basically three options here, I'll name them and mark them with A, B and C. So it's clear when voting about.

Note: All options only mangle shebangs in executable files. All options can be disabled per spec by unsetting %__brp_mangle_shebangs or setting it to /usr/bin/true.

Option A: Full mangling

Converts all /usr/bin/env anything shebangs to /usr/bin/anything (where anything is a placeholder for, well, anything), later it converts /usr/bin/python to /usr/bin/python2.

Note that both /usr/bin/env anything and /usr/bin/python are already forbidden by the guidelines, but still common.

Implementation for this is at https://src.fedoraproject.org/rpms/redhat-rpm-config/pull-request/9

Option B: Python only mangling

The same like option A except it only changes Python related shebengs. (Instead of anything, read python(2|3)?.)

/usr/bin/env python gets mangled to /usr/bin/python2.
/usr/bin/python gets mangled to /usr/bin/python2
/usr/bin/env python2 gets mangled to /usr/bin/python2.
/usr/bin/env python3 gets mangled to /usr/bin/python3.

/usr/bin/env php stays unchanged.

Option C: Unversioned Python only magnling

/usr/bin/env python gets mangled to /usr/bin/env python2.
/usr/bin/python gets mangled to /usr/bin/python2

Nothing else is touched.


Any option works for us. Option C is the least intrusive one, however I see option B as a better solution. Other languages may benefit from option A, however some might not like it, so it's a bit dangerous.

If we do this, it would be great to do this before the F28 mass rebuild.


We now have two tickets about this, which is too many. I know I've seen this script before.

In any case, I don't have any issue with automatic mangling of shebang lines as long as it's well documented (unlike a lot of the current magic that happens in brp-* scripts) and there is an easy and documented method for disabling it. I looked at the current PR I believe it fails on both accounts.

Chatted with Igor a bit on IRC about this. Since there's both a PR and this ticket it gets tough to decide what discussion should go, and different people will see each comment. So I'll just have to cut-n-paste.

In any case, on the issue of disabling the script, there's a problem because there is no standard way to disable brp-* scripts and apparently Panu has requested that Fedora not ad-hoc start adding them until RPM upstream has chosen a method. But my opinion is that they've had 20 years to do that and haven't, and I simply don't think this should be added without any means to disable it because:

  • This affects 1500+ packages. Can we be absolutely sure it doesn't break any of them?
  • I personally can't be sure that there isn't some valid use of env which would be wiped out here.
  • At least one prominent packager objects very strongly to any prohibition on /usr/bin/env, and simply making it automatic without the potential to be disabled is a rather underhanded way to bypass any complaints about the guidelines. Yes, /usr/bin/env is forbidden by the guidelines, but you can bypass a guideline if you need to. That's significantly different from just wiping env from existence.

So, I'll propose making it conditional on %brp_mangle_shebangs, and have that set to true by default. Then if you don't want it, you just do %undefine %brp_mangle_shebangs. And if RPM upstream does choose a different way to deal with it, then we'll just have to fix the few packages which might be using it.

As to the documentation, I realize that no matter how much you put in the script nobody's ever going to know to look there, but I do think there should at least be something. I can ever find any more time I can add a page or something which talks about what RPM does behind your back after %install and what you can do about it. That would be filled in using information from the documentation in the script.

How to prevent the BRP script changing shebangs for files I don't want?

@vondruch

Easy way: Don't set them executable (I forgot to mention that).

Complicated way if the one is not enough: Redefine os install post

So, I'll propose making it conditional on %brp_mangle_shebangs, and have that set to true by default. Then if you don't want it, you just do %undefine %brp_mangle_shebangs. And if RPM upstream does choose a different way to deal with it, then we'll just have to fix the few packages which might be using it.

I can do that.

At least one prominent packager objects very strongly to any prohibition on /usr/bin/env...

Feel free to go with option B. That wouldn't affect his use case, because it would only change Python.

As to the documentation, I realize that no matter how much you put in the script nobody's ever going to know to look there, but I do think there should at least be something.

I'll gladly accompany this new brp script with some notes to the guidelines. Based on the option we use, the place and content would differ. (E.g. B and C would go to Python guidelines, A would go next to the env guideline + some bits to Python guidelines.)

I've added this to the original post:

Note: All options only mangle shebangs in executable files.

@tibbs This was historically in the script:

 %{!?__jar_repack:/usr/lib/rpm/redhat/brp-java-repack-jars}

Should I do it in similar fashion?

 %{!?__disable_brp_mangle_shebangs:/usr/lib/rpm/redhat/brp-mangle-shebangs}

It seems easier to check for non-availability of the macro and let people set one.

There are several pre-existing examples, with the only common thing between them being that they are all different... (see
py_auto_byte_compile, __debug_package and of course the late __jar_repack one)

One possibility would be introducing a common mangling theme like this:

%__brp_mangle_shebangs /usr/lib/rpm/redhat/brp-mangle-shebangs

...and then:

%__os_install_post    \
   %{?__brp_mangle_shebangs} \
   %{?__brp_this} \
   %{?__brp_that} \
   ...

...and then to disable you just undefine that macro (or you can override it to use a custom one)

I don't have a strong opinion. I'm just unsure where is the right place for the definition if we go that way.

Yeah there'd be some inconsistencies and migration pains no matter what kind of scheme gets selected...

...but lets just say that you could do a whole lot worse than going by that example, even if you only do it for the shebang-mangler.

FWIW to get things moving, I converted the existing brp-scripts to use a common theme to the extent possible without breaking stuff (hopefully):
https://src.fedoraproject.org/rpms/redhat-rpm-config/c/c4646d791dea6b976fd5a04e2562c7b21b9389e0?branch=master

It is now possible to disable this BRP by unsetting %__brp_mangle_shebangs or setting it to /usr/bin/true.

At this point I have no real objection to this and would like to move it forward. I think we should indeed document all of the brp_* stuff but whether that ends up being part of the packaging guidelines or elsewhere (even a file installed as part of redhat-rpm-config) isn't something we should worry about here.

Before just pushing this into redhat-rpm-config, though, I was hoping to get some input from other FPC folks.

In addition, to make sure there aren't any obvious problems I'm gearing things up to do a rebuild of at least a fair portion of rawhide to look for warnings or failures generated by the proposed script. It's been a while since I last did this so my tools for it need to be brought up to snuff. (I have something which spreads mockbuilds out over a couple hundred hosts, but it bitrotted a bit.) I do hope to have some useful results soon. I don't expect any real issues but I think it's fair to at least try.

@tibbs at the moment of writing initial brp I rebuilt standard buildroot and didn't spot any problems.

Guidelines/documentation proposal:

To the end of https://fedoraproject.org/wiki/Packaging:Guidelines#Shebang_lines please add:


Since Fedora 28, shebangs are automatically mangled. All env shebangs are converted to their correct /usr/bin/... counterpart. You might see the following warnings in the build log:

*** WARNING: mangling shebang in ./usr/share/gems/gems/builder-3.2.3/test/test_xchar.rb from #!/usr/bin/env ruby to #!/usr/bin/ruby

You SHOULD fix those shebangs manually to remove the warning.

If this mangling breaks your package, you can turn it off by putting %undefine __brp_mangle_shebangs into your spec. You MUST accompany the line with an explaining comment about why is this needed.


Into https://fedoraproject.org/wiki/Packaging:Python#Multiple_Python_Runtimes after "...used in shebang lines or as a dependency of a package." please add:


All forbidden ambiguous python shebangs are converted to /usr/bin/python2 since Fedora 28. You might observe the following warnings in the build log:

*** WARNING: mangling shebang in ./usr/bin/taskotron_result from #!/usr/bin/python to #!/usr/bin/python2

You MUST fix those shebangs manually to remove the warning to comply with the above guidelines.


Feel free to reword this as you see fit.

There's an interesting question here: If this is being done automatically, why make packagers duplicate the effort by telling them to fix the shebangs themselves?

Certainly don't do things silently. But does this even need to be a WARNING as opposed to just an informative line? And if it is something that people need to be alerted about, why bother fixing anything automatically? Just fail the build instead. That way things would actually get fixed.

As it is now, I can't imagine many people actually fixing this just to quiet some warning deep down in the build logs since in the end they get an identical package and a nastier spec file. And I honestly don't think they should.

Edit: OK, that does sort of ignore <= F27 and EPEL. But as long as nobody takes out any existing shebang mangling, the situation doesn't get any worse for those releases, and simply evaporates for F28+.

Edit2: Left out a word somehow.

As it is now, I can't imagine many people actually fixing this just to quiet some warning deep down in the build logs since in the end they get an identical package and a nastier spec file. And I honestly don't think they should.

As for env, this make sense, so we can change it to INFO and don't bother anyone with fixing. For python, the situation is different because if they don't fix it, we cannot ever change it to python3. We can consider making it hard fail later.

So I guess this builds down to printing "INFO" or something when something is changed, except for the two cases of unversioned python usage:

  • #!/usr/bin/env python
  • #!/usr/bin/python

Those would be a warning and potentially a failure. Eventually we could even have %_unversioned_python_terminate_build (following the existing examples of %_missing_build_ids_terminate_build and %_python_bytecompile_errors_terminate_build).

I agree. I won't get to the implementation changes until tomorrow though.

Updated guidelines/documentation proposal (implementation changed accordingly):

To the end of https://fedoraproject.org/wiki/Packaging:Guidelines#Shebang_lines please add:


Since Fedora 28, shebangs are automatically mangled. All env shebangs are converted to their appropriate /usr/bin/... counterpart. You don't have to change them manually. If this mechanism breaks your package, you can turn it off by putting %undefine __brp_mangle_shebangs into your spec. If used, you MUST accompany the line with an explaining comment about why is this needed.
You can also opt out for specific paths or shebangs only be defining __brp_mangle_shebangs_exclude_from or __brp_mangle_shebangs_exclude following similar rules as in Packaging:AutoProvidesAndRequiresFiltering. Only executable files are mangled, removing the executable permission MUST be preferred over utilizing the exclude macros where possible.


Into https://fedoraproject.org/wiki/Packaging:Python#Multiple_Python_Runtimes after "...used in shebang lines or as a dependency of a package." please add:


All forbidden ambiguous python shebangs are automatically converted to /usr/bin/python2 since Fedora 28. You might observe the following warnings in the build log:

*** WARNING: mangling shebang in ./usr/bin/taskotron_result from #!/usr/bin/python to #!/usr/bin/python2. This will become an ERROR, fix it manually!

You MUST fix those shebangs manually to remove the warning to comply with the above guidelines. The warnings will eventually become errors and will fail the build.

For opting out (e.g. for templates) see XXX LINK TO THE ABOVE XXX.


Feel free to reword this as you see fit.

Changelog:

  1. changed "correct" to "appropriate"
  2. added notes about __brp_mangle_shebangs_exclude_from and __brp_mangle_shebangs_exclude

All env shebangs are converted to their correct /usr/bin/... counterpart.

I might be wrong, but the script does not check if the /usr/bin counterpart exists, so the "correct" might not be precise. Sorry for nitpicking.

It doesn't have to exist during the buildtime, so there is no way to cheek. Changed to "appropriate".

@tibbs can we please make this happen before the mass rebuild?

Does this need tagging with "meeting"?

@mbooth I think this needs draft (if there is none already).

Metadata Update from @tibbs:
- Issue tagged with: meeting

a year ago

We looked at this ticket this week (https://meetbot-raw.fedoraproject.org/fedora-meeting-1/2018-03-15/fpc.2018-03-15-16.00.txt):

Metadata Update from @james:
- Issue untagged with: meeting
- Issue tagged with: writeup

a year ago

Sorry, I'm a little late to this discussion. According to the Python Developer's Guide, the /usr/bin/python shebang is valid for scripts that are compatible with both versions of python:

"
- for the time being, all distributions should ensure that python refers to the same target as python2.
- however, end users should be aware that python refers to python3 on at least Arch Linux (that change is what prompted the creation of this PEP), so python should be used in the shebang line only for scripts that are source compatible with both Python 2 and 3.
"

https://www.python.org/dev/peps/pep-0394/#recommendation

I bring this up because we have python scripts in ZFS that are compatible with both versions of python, and we don't necessary want to pick one.

We are monitoring that PEP, but we are trying to prepare Fedora for the time when this PEP changes. While it is perfectly valid for upstream to use python shebang that indicates "supports both", in Fedora packages we need to make sure it always runs on a specific one, because RPM packages have dependencies and those are not python version agnostic.

Also we want to run as much as possible on 3. So if you support 3, please use that in Fedora packages. You can use pathfix.py (from python3-devel) to fix the shebangs.

Announcement text:

Information about the automatic shebang line checking and modification has been added to both the main guidelines and the Python guidelines.

Metadata Update from @tibbs:
- Issue untagged with: writeup
- Issue assigned to tibbs
- Issue tagged with: announce

a year ago

Metadata Update from @tibbs:
- Issue untagged with: announce
- Issue close_status updated to: accepted
- Issue status updated to: Closed (was: Open)

a year ago

Login to comment on this ticket.

Metadata