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

Jump to bottom

openbox: Use python3 instead of python2 #87846

Merged
merged 1 commit into from May 17, 2020
Merged

openbox: Use python3 instead of python2 #87846

merged 1 commit into from May 17, 2020

Conversation

SFrijters
Copy link
Member

Motivation for this change

Openbox contains a single python script that is trivially convertible to python3 (print statements to print functions). With an automatic conversion in the patch phase we can drop the python2 dependency.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@doronbehar
Copy link
Contributor

Could you show a diff please of the changes `2to3 does to the file? I'm thinking it might be more reliable to add a static patch that would do that. Arch Linux has such patches:

https://git.archlinux.org/svntogit/community.git/tree/trunk?h=packages/openbox

@SFrijters
Copy link
Member Author

SFrijters commented May 15, 2020 •
edited

Sure, I'm also fine with a static patch. The autogenerated changes are pretty much the same as in Arch (comparing to https://git.archlinux.org/svntogit/community.git/tree/trunk/py3.patch?h=packages/openbox). They modify the first chunk of the python script slightly differently, and they also patch some autoconfig files, which we won't need to do because we rewrite the shebangs anyway. Just let me know what you prefer, I'll just be happy to get rid of the last python2 thing in my nix store. (Note that I got this diff by diffing the old and the new version in my nix store, so the line numbers in this example will be off because of Nix's path shenanigans; I will make a proper patch if we move in this direction.)

32,34c32,34
<     print
<     print >>sys.stderr, "ERROR:", ME, "requires PyXDG to be installed"
<     print
---
>     print()
>     print("ERROR:", ME, "requires PyXDG to be installed", file=sys.stderr)
>     print()
55c55
<                 print "Invalid .desktop file: " + path
---
>                 print("Invalid .desktop file: " + path)
103c103
<             print "\t ", str
---
>             print("\t ", str)
105c105
<             print "\t*", str
---
>             print("\t*", str)
150c150
<             print "[*] " + self.de.getName()
---
>             print("[*] " + self.de.getName())
152c152
<             print "[ ] " + self.de.getName()
---
>             print("[ ] " + self.de.getName())
157c157
<         print
---
>         print()
169,191c169,191
<     print "Usage:", ME, "[OPTION]... [ENVIRONMENT]..."
<     print
<     print "This tool will run xdg autostart .desktop files"
<     print
<     print "OPTIONS"
<     print "  --list        Show a list of the files which would be run"
<     print "                Files which would be run are marked with an asterix"
<     print "                symbol [*].  For files which would not be run,"
<     print "                information is given for why they are excluded"
<     print "  --help        Show this help and exit"
<     print "  --version     Show version and copyright information"
<     print
<     print "ENVIRONMENT specifies a list of environments for which to run autostart"
<     print "applications.  If none are specified, only applications which do not "
<     print "limit themselves to certain environments will be run."
<     print
<     print "ENVIRONMENT can be one or more of:"
<     print "  GNOME         Gnome Desktop"
<     print "  KDE           KDE Desktop"
<     print "  ROX           ROX Desktop"
<     print "  XFCE          XFCE Desktop"
<     print "  Old           Legacy systems"
<     print
---
>     print("Usage:", ME, "[OPTION]... [ENVIRONMENT]...")
>     print()
>     print("This tool will run xdg autostart .desktop files")
>     print()
>     print("OPTIONS")
>     print("  --list        Show a list of the files which would be run")
>     print("                Files which would be run are marked with an asterix")
>     print("                symbol [*].  For files which would not be run,")
>     print("                information is given for why they are excluded")
>     print("  --help        Show this help and exit")
>     print("  --version     Show version and copyright information")
>     print()
>     print("ENVIRONMENT specifies a list of environments for which to run autostart")
>     print("applications.  If none are specified, only applications which do not ")
>     print("limit themselves to certain environments will be run.")
>     print()
>     print("ENVIRONMENT can be one or more of:")
>     print("  GNOME         Gnome Desktop")
>     print("  KDE           KDE Desktop")
>     print("  ROX           ROX Desktop")
>     print("  XFCE          XFCE Desktop")
>     print("  Old           Legacy systems")
>     print()
194,196c194,196
<     print ME, VERSION
<     print "Copyright (c) 2008        Dana Jansens"
<     print
---
>     print(ME, VERSION)
>     print("Copyright (c) 2008        Dana Jansens")
>     print()

@doronbehar
Copy link
Contributor

Just let me know what you prefer, I'll just be happy to get rid of the last python2 thing in my nix store.

I feel you body :) I also had some PRs motivated by getting rid of Python2. Not all of them have even reached the approval state. Also, note I'm only a member of @NixOS/nixpkgs-maintainers as well so the best I'll be able to help you is approve the PR.

My personal opinion, also judging from other PRs I saw that added patches similar to or based on patches from other distros (#84072 ), I think it'll be best to download that patch from Arch and put in openbox/py3.patch and add it as ./py3.patch.

@SFrijters
Copy link
Member Author

@FRidh Could you maybe weigh in on this and add it to the Python 2 deprecation project?

@veprbl
Copy link
Member

veprbl commented May 15, 2020

Upstream PR Mikachu/openbox#9

, I think it'll be best to download that patch from Arch and put in openbox/py3.patch and add it as ./py3.patch.

Please use fetchpatch.

@veprbl veprbl moved this from WIP to Needs review in Python 2 deprecation May 15, 2020
@SFrijters
Copy link
Member Author

SFrijters commented May 15, 2020 •
edited

@veprbl I tried to implement your requested change but I ran into two problems

  • The patch from the PR you linked doesn't apply to the tarball nixpkgs uses. I tried to switch over our src to pull from GitHub, but then we need an additional ./bootstrap command, which doesn't seem to play nicely with the autoreconfHook?
  • The PR patch includes a rename which doesn't work with fetchpatch (fetchpatch: ignores file renames #32084). Indeed when I do a build with -K and look in the build dir, files are not correctly renamed. Note that we don't actually need the part of the patch that renames, since it's about using autoconf to set the Python shebangs, which we modify ourselves anyway.

@veprbl
Copy link
Member

veprbl commented May 15, 2020

@SFrijters Does this not work?

fetchpatch {
  name = "openbox-py3.patch";
  url = "https://git.archlinux.org/svntogit/community.git/plain/trunk/py3.patch?h=packages/openbox&id=147b9734e4bbdf5558e3f7d11c6541e2c429203c";
  sha256 = "019q8yq334wfrhmbh58wsh3cfpbj48pf08s5nlp51nak7dkn0hgi";
}

@SFrijters
Copy link
Member Author

@veprbl Yes, that patch does apply. But I'm still left with a #!@PYTHON@ shebang in my installation, so the script doesn't work properly anymore.

The build log does say Rewriting #!@PYTHON@ to #!/nix/store/2dcsn57cgaxs92ha5swihrab0g3l2h6g-python3-3.7.7, but it doesn't actually happen.

$ /nix/store/py75wi3hislh1llgmillg074lr43z8as-openbox-3.6.1/libexec/openbox-xdg-autostart
bash: /nix/store/py75wi3hislh1llgmillg074lr43z8as-openbox-3.6.1/libexec/openbox-xdg-autostart: @PYTHON@: bad interpreter: No such file or directory

In this case we would also need to use autoconf again to make sure the file is configured correctly, but then we're still left with the problem that the renaming of the file data/autostart/openbox-xdg-autostart to data/autostart/openbox-xdg-autostart.in doesn't actually work.

@SFrijters
Copy link
Member Author

Pushed a new attempt. It uses fetchpatch, but then also includes a workaround to rename the required files and an autoconf step so they are actually configured. And since the tarball src is already autoconfed I switched to the github source. At least the patch is now a PR on the actual src.

@SFrijters
Copy link
Member Author

@veprbl Thanks for shepherding this; I hope I have done it correctly this time around, with minimal changes.

veprbl
veprbl approved these changes May 17, 2020
View reviewed changes
Copy link
Member

@veprbl veprbl left a comment

Choose a reason for hiding this comment

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

Looks good. Thank you!

@veprbl veprbl merged commit ab0adc2 into NixOS:master May 17, 2020
Python 2 deprecation automation moved this from Needs review to Done May 17, 2020
@SFrijters SFrijters deleted the openbox-python3 branch September 30, 2020 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@veprbl veprbl veprbl approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10
Projects
Python 2 deprecation
  
Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@SFrijters @doronbehar @veprbl