LilyIssues / Issues / #5646 Switch to Python 3.x

Jonas Hahnfeld - 2019-12-19

Description has changed:

Diff:

Needs: -->

Patch: new --> needs_work
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Jonas Hahnfeld - 2019-12-19

The switch itself requires 3 changes which I would propose to squash into one commit:
1) Adapt the build system to find and require Python 3.
patch: https://codereview.appspot.com/545370043
2) The largest part of the switch is running 2to3 which is now able to handle the rest of the conversion automatically. For reference, changes for current 'master' are here: https://codereview.appspot.com/573340043
3) Fix-up two places in the scripts afterwards:
3a) Manual changes for compatibility with Python 3, such as removing the call to sys.setdefaultencoding.
3b) Replace cgi.escape by html.escape. While not strictly needed, I think we should include this change because they removed cgi.escape in Python 3.8 after only deprecating it in Python 3.2. The replacement html.escape is only available since that Python 3.2, so we can't do this before switching.
patch: https://codereview.appspot.com/561270043

All of this also available in branch dev/hahnjo/python3 in the repository.

Last edit: Jonas Hahnfeld 2020-01-22

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Jonas Hahnfeld - 2020-01-22

Patch: needs_work --> new
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Jonas Hahnfeld - 2020-01-22

Putting into new state to solicit comments

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- David Kastrup - 2020-01-22
  
  That is not how it works. "new" generally triggers regression testing, and success of it puts the patch into "review". So to get it to review, you need to set it to "review" (usually the privilege of James) and when James puts it into "countdown", dial that back again to "review". And if it migrates to "push" without ever having seen a regtest, of course dial it back again too. When you are convinced that it passes regtests, "new" is fine. It's your choice then what to do with the eventually resulting "push".
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - Jonas Hahnfeld - 2020-01-22
    
    It passes all regtests on my system as explained already in December. So yes, that's exactly what I want.
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2020-01-22

Patch: new --> needs_work
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2020-01-22

Not sure what has changed since patch 1 but if I apply patch 2 on a 'clean' master my 'configure' command fails with

config.status: creating config.make config.status: creating config.hh WARNING: Please consider installing optional programs or files: URW++ OTF fonts (these files are missing: C059-BdIta.otf C059-Bold.otf C059-Italic.otf C059-Roman.otf NimbusMonoPS-Bold.otf NimbusMonoPS-BoldItalic.otf NimbusMonoPS-Italic.otf NimbusMonoPS-Regular.otf NimbusSans-Bold.otf NimbusSans-BoldItalic.otf NimbusSans-Italic.otf NimbusSans-Regular.otf) tidy ERROR: Please install required programs: TeX Gyre fonts OTF (these files are missing: texgyreschola-regular.otf texgyreschola-italic.otf texgyreschola-bold.otf texgyreschola-bolditalic.otf texgyreheros-regular.otf texgyreheros-italic.otf texgyreheros-bold.otf texgyreheros-bolditalic.otf texgyrecursor-regular.otf texgyrecursor-italic.otf texgyrecursor-bold.otf texgyrecursor-bolditalic.otf) See INSTALL.txt for more information on how to build LilyPond

Yet (apart from Tidy) I have all this installed.

i.e.

on a clean master I get this

config.status: creating config.make config.status: creating config.hh WARNING: Please consider installing optional programs or files: tidy See INSTALL.txt for more information on how to build LilyPond Type: make all to build LilyPond make install to install LilyPond make help to see all possible targets Edit local.make for local Makefile overrides.
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Jonas Hahnfeld - 2020-01-22
  
  Did you apply all three patches?
  https://codereview.appspot.com/545370043
  https://codereview.appspot.com/573340043
  https://codereview.appspot.com/561270043
  
  I have them in separate Rietveld issues because the second one is actually generated, but as I wrote on lilypond-devel I propose to commit them as one patch.
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
  - David Kastrup - 2020-01-22
    
    "Jonas Hahnfeld" hahnjo@users.sourceforge.net writes:
    
    Did you apply all three patches?
    https://codereview.appspot.com/545370043
    https://codereview.appspot.com/573340043
    https://codereview.appspot.com/561270043
    
    I have them in separate Rietveld issues because the second one is
    actually generated, but as I wrote on lilypond-devel I propose to
    commit them as one patch.
    
    It's usually supposed the other way round: put them into one review (but
    as separate diffs, so basically commit by commit) and, if you are fine
    with that, as one merge commit with a short branch. Try
    
    git log --graph 5385d029e3e4e89c1a7dba20d3e136f594321816
    
    to get an example for how it looks when done right.
    
    Basically reviews should compile, and so should mainline commits (for
    the sake of bisection). If you feel insecure about creating such a
    setup (basically the final merge has to be done with --no-ff in order to
    actually get a separate merge commit), ask for someone else to do the
    push.
    
    --
    David Kastrup
    
    If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
    - Jonas Hahnfeld - 2020-01-22
      
      David, did you even carefully read my response?!? I'm doing exactly that!
      
      Let me try again:
      I have three commits locally: the switch itself, a generated patch by 2to3 and another patch to make it actually work. See above for the three links, with this one being the SF issue to link them.
      Now I can put this into one large issue on Rietveld, but you actually don't want to review the second patch. So I have them separate to see the manual changes that do need review. On push, I will fuse them into one commit because a patch won't help with bisection (git bisect might end up picking a commit from the branch that doesn't actually compile).
      
      If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
      - Jonas Hahnfeld - 2020-01-22
        
        PS: Sorry if this sounds impolite, but I'm frustrated that I have to explain over and over again.
        
        If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
        
        David Kastrup - 2020-01-22
        
        "Jonas Hahnfeld" hahnjo@users.sourceforge.net writes:
        
        PS: Sorry if this sounds impolite, but I'm frustrated that I have to
        explain over and over again.
        
        I can sympathise with that.
        
        Let's try a different example: it appears my last example wasn't clear
        enough to keep you from reexplaining.
        
        Try
        
        https://sourceforge.net/p/testlilyissues/issues/3815
        
        and its related Rietveld issue
        
        https://codereview.appspot.com/53120044/#ps1
        
        and the commit
        
        git log --graph 809c04e37b8d3d368eda7e4b576e0d80d1934cc3
        
        as another example of a merge commit where the automated conversion run
        has been committed to the same Rietveld review as a separate patch
        and the whole has been committed as a merge commit in order to have
        everything compile well in the mainline.
        
        --
        David Kastrup
        
        If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
      - David Kastrup - 2020-01-22
        
        "Jonas Hahnfeld" hahnjo@users.sourceforge.net writes:
        
        David, did you even carefully read my response?!? I'm doing exactly
        that!
        
        No, you aren't. They are not supposed to be 3 separate Rietveld issues
        but a single issue with 2 updates to it.
        
        Let me try again:
        I have three commits locally: the switch itself, a generated patch by
        2to3 and another patch to make it actually work. See above for the
        three links, with this one being the SF issue to link them.
        Now I can put this into one large issue on Rietveld, but you actually
        don't want to review the second patch. So I have them separate to see
        the manual changes that do need review.
        
        That's why one puts them into separate updates so that people can select
        the diffs to review.
        
        On push, I will fuse them into one commit because a patch won't help
        with bisection (git bisect might end up picking a commit from the
        branch that doesn't actually compile).
        
        That's why one puts just a merge commit in the mainline. See the
        example I gave.
        
        --
        David Kastrup
        
        If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
        
        Jonas Hahnfeld - 2020-01-22
        
        (apparently I can't reply via email, so copy-pasting here)
        
        Am Mittwoch, den 22.01.2020, 13:03 +0000 schrieb David Kastrup:
        
        "Jonas Hahnfeld" hahnjo@users.sourceforge.net writes:
        
        David, did you even carefully read my response?!? I'm doing exactly
        that!
        
        No, you aren't. They are not supposed to be 3 separate Rietveld issues
        but a single issue with 2 updates to it.
        
        This means it will be impossible to update the individual commits. But
        I can do this if you insist.
        
        Let me try again:
        I have three commits locally: the switch itself, a generated patch by
        2to3 and another patch to make it actually work. See above for the
        three links, with this one being the SF issue to link them.
        Now I can put this into one large issue on Rietveld, but you actually
        don't want to review the second patch. So I have them separate to see
        the manual changes that do need review.
        
        That's why one puts them into separate updates so that people can select
        the diffs to review.
        
        On push, I will fuse them into one commit because a patch won't help
        with bisection (git bisect might end up picking a commit from the
        branch that doesn't actually compile).
        
        That's why one puts just a merge commit in the mainline. See the
        example I gave.
        
        This is what I get for
        
        $ git log --oneline --graph 5385d029e3...92ae8d8679 * 5385d029e3 Merge branch 'issue3648' into staging |\ | * 703dca1fd6 Run scripts/auxiliar/makelsr.py | * b570097f37 Issue 3648/7: Give several examples of durations without explicit pitches | * 80e4d58800 Issue 3648/6: Regtest for rhythmic sequences. | * f1163b25f8 Issue 3648/5: Explain isolated durations in NR "Rhythms" | * 33dd448c34 Issue 3648/4: Let \displayLilyMusic deal with pure rhythms | * 1a1c0afede Issue 3648/3: Pitchless durations inherit previous pitch when scorifying | * 7958611915 Let copy-repeat-chord return the copy target for efficiency | * 2527d43953 Issue 3648/2: Slight documentation amendment for RhythmicStaff in NR | * 3dc88bc7db Issue 3648/1: Isolated durations in music sequences now stand for unpitched notes |/ * fc830bf324 Interpret #{ -3 #} as postevent rather than negative number
        
        So $ git bisect might pick up an intermediate commit (3dc88bc7db to
        703dca1fd6) and ask you to test it. This might work in your example,
        but actually not for switching to Python 3.x for the mentioned reasons.
        
        Jonas
        
        If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
        
        David Kastrup - 2020-01-22
        
        "Jonas Hahnfeld" hahnjo@users.sourceforge.net writes:
        
        (apparently I can't reply via email, so copy-pasting here)
        
        Am Mittwoch, den 22.01.2020, 13:03 +0000 schrieb David Kastrup:
        
        "Jonas Hahnfeld" hahnjo@users.sourceforge.net writes:
        
        David, did you even carefully read my response?!? I'm doing exactly
        that!
        
        No, you aren't. They are not supposed to be 3 separate Rietveld issues
        but a single issue with 2 updates to it.
        
        This means it will be impossible to update the individual commits.
        
        Indeed. But the fixes on top tend to be small.
        
        But I can do this if you insist.
        
        It would be a bit hypocritical for me to insist since when digging
        through the repository for a useful example, it turned out that almost
        all of my merge-structured commits were, after all, mashed into a single
        Rietveld review patch. But that's the course if you want to make life
        for reviewers simpler in the way you indicated.
        
        It's one use case of Reitveld-based review where our current setup does
        not provide a satisfactory workflow.
        
        This is what I get for
        ~~~
        $ git log --oneline --graph 5385d029e3...92ae8d8679
        * 5385d029e3 Merge branch 'issue3648' into staging
        |\
        | * 703dca1fd6 Run scripts/auxiliar/makelsr.py
        | * b570097f37 Issue 3648/7: Give several examples of durations without explicit pitches
        | * 80e4d58800 Issue 3648/6: Regtest for rhythmic sequences.
        | * f1163b25f8 Issue 3648/5: Explain isolated durations in NR "Rhythms"
        | * 33dd448c34 Issue 3648/4: Let \displayLilyMusic deal with pure rhythms
        | * 1a1c0afede Issue 3648/3: Pitchless durations inherit previous pitch when scorifying
        | * 7958611915 Let copy-repeat-chord return the copy target for efficiency
        | * 2527d43953 Issue 3648/2: Slight documentation amendment for RhythmicStaff in NR
        | * 3dc88bc7db Issue 3648/1: Isolated durations in music sequences now
        | stand for unpitched notes
        |/
        * fc830bf324 Interpret #{ -3 #} as postevent rather than negative number
        ~~~
        
        So $ git bisect might pick up an intermediate commit (3dc88bc7db to
        703dca1fd6) and ask you to test it. This might work in your example,
        but actually not for switching to Python 3.x for the mentioned reasons.
        
        In my experience, Git travels into non-mainline branches only if it "has
        to". I have yet to land in one such side branch, but maybe I have just
        been lucky.
        
        --
        David Kastrup
        
        If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
        
        Jonas Hahnfeld - 2020-01-22
        
        Am Mittwoch, den 22.01.2020, 14:00 +0000 schrieb David Kastrup:
        
        "Jonas Hahnfeld" hahnjo@users.sourceforge.net writes:
        
        (apparently I can't reply via email, so copy-pasting here)
        
        Am Mittwoch, den 22.01.2020, 13:03 +0000 schrieb David Kastrup:
        
        "Jonas Hahnfeld" hahnjo@users.sourceforge.net writes:
        
        David, did you even carefully read my response?!? I'm doing exactly
        that!
        
        No, you aren't. They are not supposed to be 3 separate Rietveld issues
        but a single issue with 2 updates to it.
        
        This means it will be impossible to update the individual commits.
        
        Indeed. But the fixes on top tend to be small.
        
        Usually yes. But in this scenario I might be forced to re-run 2to3 in the middle of the series if somebody commits a change to any of the touched python files in the mean time.
        
        [...]
        It's one use case of Reitveld-based review where our current setup does
        not provide a satisfactory workflow.
        
        +1, sadly. On the positive side, this kind of changes with incompatible dependency versions are hopefully rare enough.
        
        [...]
        
        So $ git bisect might pick up an intermediate commit (3dc88bc7db to
        703dca1fd6) and ask you to test it. This might work in your example,
        but actually not for switching to Python 3.x for the mentioned reasons.
        
        In my experience, Git travels into non-mainline branches only if it "has
        to". I have yet to land in one such side branch, but maybe I have just
        been lucky.
        
        Apparently the latter. Not really related to this issue, but I hit this with a constructed example on my first try. In the attached repo,
        
        $ git bisect start $ git bisect bad $ git bisect good fd7f669 Bisecting: 2 revisions left to test after this (roughly 1 step) [90f9202f1a0f49349895ac0118776b55e4b642c2] c
        
        which is the first commit on branch 'issue' :-(
        
        test.tar.gz
        
        If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Description has changed:

Diff:

--- old
+++ new
@@ -6,4 +6,10 @@
 be available in all major distributions, including Ubuntu LTS 16.04 and
 18.04 as well as CentOS/RHEL 7.x and 8.x.

-http://codereview.appspot.com/545370043
+Invidivial changes:
+https://codereview.appspot.com/545370043
+https://codereview.appspot.com/573340043
+https://codereview.appspot.com/561270043
+
+Full patch (with Patch Sets)
+https://codereview.appspot.com/553420043

Patch: needs_work --> new

Jonas Hahnfeld - 2020-01-22

Full patch as requested: https://codereview.appspot.com/553420043

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Anonymous - 2020-01-22

Patch: new --> needs_work
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Fails make doc (passes make test-baseline).

I cannot get more information out than this (even with VERBOSE=1)

...
# Making out-www/xref-maps/AAA-intro-regression.xref-map < texi
/home/james/lilypond-git/build/scripts/build/out/extract_texi_filenames -I ./out-www -I /home/james/lilypond-git/input/regression  -q -o /home/james/lilypond-git/build/./out-www/xref-maps out-www/AAA-intro-regression.texi
Traceback (most recent call last):
  File "/home/james/lilypond-git/build/../scripts/lilypond-book.py", line 770, in <module>
    main ()
  File "/home/james/lilypond-git/build/../scripts/lilypond-book.py", line 753, in main
    chunks = do_file (files[0])
  File "/home/james/lilypond-git/build/../scripts/lilypond-book.py", line 602, in do_file
    do_process_cmd (chunks, input_fullname, global_options)
  File "/home/james/lilypond-git/build/../scripts/lilypond-book.py", line 484, in do_process_cmd
    abs_output_dir)
  File "/home/james/lilypond-git/build/python/out/book_snippets.py", line 621, in link_all_output_files
    existing, missing = self.all_output_files (output_dir, output_dir_files)
  File "/home/james/lilypond-git/build/python/out/book_snippets.py", line 719, in all_output_files
    required_files = self.formatter.required_files (self, base, full, result)
  File "/home/james/lilypond-git/build/python/out/book_texinfo.py", line 373, in required_files
    return self.required_files_png (snippet, base, full, required_files)
  File "/home/james/lilypond-git/build/python/out/book_base.py", line 191, in required_files_png
    page_count = BookSnippet.ps_page_count (full + '.eps')
  File "/home/james/lilypond-git/build/python/out/book_snippets.py", line 235, in ps_page_count
    header = open (ps_name).read (1024)
  File "/usr/lib/python3.6/encodings/ascii.py", line 26, in decode
    return codecs.ascii_decode(input, self.errors)[0]
UnicodeDecodeError: 'ascii' codec can't decode byte 0xf8 in position 2182: ordinal not in range(128)
/home/james/lilypond-git/build/.././make/ly-rules.make:44: recipe for target 'out-www/collated-files.texi' failed
make[3]: *** [out-www/collated-files.texi] Error 1
make[3]: Leaving directory '/home/james/lilypond-git/build/input/regression'
/home/james/lilypond-git/build/../stepmake/stepmake/generic-targets.make:171: recipe for target 'WWW-1' failed
make[2]: *** [WWW-1] Error 2
make[2]: Leaving directory '/home/james/lilypond-git/build/input'
/home/james/lilypond-git/build/../stepmake/stepmake/generic-targets.make:171: recipe for target 'WWW-1' failed
make[1]: *** [WWW-1] Error 2
make[1]: Leaving directory '/home/james/lilypond-git/build'
/home/james/lilypond-git/build/../stepmake/stepmake/generic-targets.make:187: recipe for target 'doc-stage-1' failed
make: *** [doc-stage-1] Error 2

P.S. I also did the check with the three patches (sorry I didn't see them before) and got the same error. Just in case it was to do with the 'mass patch' you kindly re-did.

Jonas Hahnfeld - 2020-01-22

You mean make check? The test-baseline should be with an unpatched master, right?

I guess then I'll have to compile a Python 3.6, I tested with 3.5.9, 3.7.? and 3.8.1 so far. The solution for this problem is to call codecs.open (ps_name, 'r', 'utf-8') but I want to test locally before updating the patch.

P.S.: The 'mass patch' should be identical to the combination of the other three.

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
Jonas Hahnfeld - 2020-01-22

Unfortunately I can't reproduce this on my system, all of my .eps files are fully ASCII. If you still have a build tree around (not necessarily from this patch), could you run the following?

$ find . -name "*.eps" | xargs grep -q '[![:cntrl:][:print:]]'

If this does return files, I need to try replicate your setup (Ubuntu, right?). As I mentioned before, this might solve the issue but I don't see the need yet:

diff --git a/python/book_snippets.py b/python/book_snippets.py index 8ef3bd013d..0ca885ab74 100644 --- a/python/book_snippets.py +++ b/python/book_snippets.py @@ -232,7 +232,7 @@ FRAGMENT_LY = r''' #################################################################### def ps_page_count (ps_name): - header = open (ps_name).read (1024) + header = codecs.open (ps_name, 'r', 'utf-8').read (1024) m = re.search ('\n%%Pages: ([0-9]+)', header) if m: return int (m.group (1))

Edit: For completeness, make doc also passes with Python 3.6.10 for me.

Last edit: Jonas Hahnfeld 2020-01-22
If you would like to refer to this comment somewhere else in this project, copy and paste the following link:
- Jonas Hahnfeld - 2020-01-22
  
  Sorry, the command is wrong, it should be
  
  $ find . -name "*.eps" | xargs grep -P "[\x80-\xFF]" | grep -v test
  
  So I indeed have non-ASCII .eps files, but not for the doc build. Let me check again tomorrow.
  
  If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Jonas Hahnfeld - 2020-01-24

@lilypond-pkx Unfortunately (or not?) I can't get the failure you were seeing above, it works fine in my VM with Ubuntu 18.04. However I found the whole process to heavily depend on the installed fonts, so maybe your system has more fonts installed than I have. If anything pops to your mind please let me know, otherwise I'm a bit out of ideas right now...

If you would like to refer to this comment somewhere else in this project, copy and paste the following link:

Switch to Python 3.x

Issue Tracker for LilyPond

Searches

Help

#5646 Switch to Python 3.x

Discussion