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

openbox: Use python3 instead of python2 #87846

Merged
merged 1 commit into from May 17, 2020
Merged

Conversation

@SFrijters
Copy link
Member

SFrijters commented May 14, 2020

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

doronbehar commented May 14, 2020

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

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

doronbehar commented May 15, 2020

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

SFrijters commented May 15, 2020

@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

@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 (#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

SFrijters commented May 16, 2020

@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 SFrijters force-pushed the SFrijters:openbox-python3 branch from 9bc0c89 to 5516772 May 16, 2020
@SFrijters
Copy link
Member Author

SFrijters commented May 16, 2020

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 SFrijters force-pushed the SFrijters:openbox-python3 branch from 5516772 to 17442c3 May 16, 2020
@SFrijters SFrijters force-pushed the SFrijters:openbox-python3 branch from 17442c3 to c4d7bd9 May 16, 2020
@SFrijters
Copy link
Member Author

SFrijters commented May 16, 2020

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

@SFrijters SFrijters force-pushed the SFrijters:openbox-python3 branch from c4d7bd9 to f1da952 May 16, 2020
@SFrijters SFrijters force-pushed the SFrijters:openbox-python3 branch from f1da952 to e38ef6f May 16, 2020
@veprbl
veprbl approved these changes May 17, 2020
Copy link
Member

veprbl left a comment

Looks good. Thank you!

@veprbl veprbl merged commit ab0adc2 into NixOS:master May 17, 2020
16 checks passed
16 checks passed
Evaluation Performance Report Evaluator Performance Report
Details
grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e38ef6f"; rev="e38ef6faa07d6011614b655fa7be9c4f7a35f6f6"; } ./pkgs/t
Details
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e38ef6f"; rev="e38ef6faa07d6011614b655fa7be9c4f7a35f6f6"; } ./nixos/
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e38ef6f"; rev="e38ef6faa07d6011614b655fa7be9c4f7a35f6f6"; } ./nixos/
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e38ef6f"; rev="e38ef6faa07d6011614b655fa7be9c4f7a35f6f6"; } ./nixos/
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e38ef6f"; rev="e38ef6faa07d6011614b655fa7be9c4f7a35f6f6"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e38ef6f"; rev="e38ef6faa07d6011614b655fa7be9c4f7a35f6f6"; } ./pkgs/t
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="e38ef6f"; rev="e38ef6faa07d6011614b655fa7be9c4f7a35f6f6"; } ./pkgs/t
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
openbox, openbox.passthru.tests on aarch64-linux Success
Details
openbox, openbox.passthru.tests on x86_64-linux Success
Details
Python 2 deprecation automation moved this from Needs review to Done May 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.