9979 Support python3 for in-gate tools

Review Request #1283 — Created Nov. 14, 2018 and submitted

citrus
illumos-gate
master
9979
2a85fda...
general
9979 Support python3 for in-gate tools

This change relates to the onbld tools which are written in python such as git-pbchk and wsdiff. It:

  • enables the building of python3 versions of onbld modules providing BUILDPY3TOOLS is not set to an octothorpe;

  • allows the python2 modules to be omitted from the onbld package via BUILDPY2TOOLS='#';

  • Provides a variable through which the default interpreter (shebang) for these can be set - TOOLS_PYTHON - which defaults to $(PYTHON) (or $(PYTHON3) if BUILDPY2TOOLS='#')

By default (if nothing is changed in the .env file) the developer/build/onbld package will gain python3 versions of the onbld modules alongside python2 versions. The tools themselves will continue to have a python2 shebang line. In the future, if a distribution wishes to switch to python3 for the tools, they can set TOOLS_PYTHON=/usr/bin/python3.5 or similar and if they want to stop building and shipping the python2 modules then BUILDPY2TOOLS='#' will achieve that. OmniOS bloody no longer uses python2 for the onbld components.

Testing

Tested all modified onbld tools from proto with both python3 and python2, for example:

% python3 proto/root_i386-nd/opt/onbld/bin/git-pbchk -p master
Comments:
These bug synopses don't match the database entries:
Synopsis of 9979 is wrong:
  should be: 'Support python3 for in-gate tools'
         is: 'Support python3 for in-gate tools-FRED'

% python2 proto/root_i386-nd/opt/onbld/bin/git-pbchk -p master
Comments:
These bug synopses don't match the database entries:
Synopsis of 9979 is wrong:
  should be: 'Support python3 for in-gate tools'
         is: 'Support python3 for in-gate tools-FRED'

Did three test builds with BUILDPY2TOOLS='#', BUILDPY3TOOLS='#' and neither set. Inspected developer/build/onbld package contents each time and ran the range of tools against some sample files.

  • 0
  • 0
  • 4
  • 0
  • 4
Description From Last Updated
citrus
yuripv
  1. All this python stuff is rather unfortunate. Can it be moved (at least, partially) out of the gate into "git patched for illumos-gate" or something?

    1. The git-pbchk probably could be. Other bits such as validate_pkg and wsdiff are not for git.

    2. As they aren't something that really needs to be installed (yes, still on the topic of onbld usefulness) and should be directly runnable without any need for "compilation", can we just have #!/usr/bin/env python as a shebang to use whatever version is installed?

  2. usr/src/data/locale/tools/mkwidths.py (Diff revision 1)
     
     

    Can this be moved to the other copyright statement?

  3. usr/src/tools/onbld/Scm/Makefile (Diff revision 1)
     
     

    This was removed why?

    1. Scm/Makefile is a completely new file. The original (along with the Joyent copyright) is now Scm/Makefile.com.

    2. Got it, sorry.

  4. 
      
citrus
tsoome
  1. Ship It!
  2. 
      
alp
  1. 
      
  2. Do I understand correctly that before this change python-35 was not hardcoded in manifest?
    Should we introduce one more parameter to make switching 3.5->3.x more transparent?

    1. You're right, the -35 should not be hardcoded here. I'll fix.

  3. 
      
ptribble
  1. Using the same toggles to enable/disable the tools as the gate components is wrong; distributions (Tribblix for one) may legitimately wish to have the tools but disable the artefacts coming from the gate.

    1. Good point, I'll fix.

  2. 
      
citrus
citrus
citrus
ptribble
  1. I'm not familiar enough with the innards of python to evaluate the python changes, bu structurally this now looks fine.

  2. 
      
alp
  1. 
      
  2. usr/src/tools/scripts/git-pbchk.py (Diff revision 3)
     
     

    How will it behave if run outside of git repository?

    1. % git-pbchk -p master
      failed to run git:
       b'fatal: not a git repository (or any parent up to mount point /data)\nStopping at filesystem boundary (GIT_DISCOVERY_ACROSS_FILESYSTEM not set).\n'
      

      The old git() function used to return a file handle and I've changed that to return an array of stdout lines. The original if not p: check was never doing anything since git() would always return a valid handle or throw an error.

  3. 
      
alp
  1. Ship It!
  2. 
      
tsoome
  1. Ship It!
  2. 
      
andy_js
  1. Ship It!
  2. 
      
citrus
citrus
citrus
tsoome
  1. Ship It!
  2. 
      
ptribble
  1. Ship It!
  2. 
      
alp
  1. Ship It!
  2. 
      
citrus
Review request changed

Status: Closed (submitted)

Loading...