- Download Diff

Summary:

9979 Support python3 for in-gate tools

Review Request #1283 — Created Nov. 14, 2018 and submitted Jan. 9, 2019, 3:36 p.m.

Information
Owner:	citrus
Repository:	illumos-gate
Branch:	master
Bugs:	9979
Depends On:
Commit:	2a85fda...
Reviewers
Groups:	general
People:

Description

9979 Support python3 for in-gate tools

Testing Done

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.

Issues

0
0
4
0
All Issues: 4

Description	From	Last Updated
Can this be moved to the other copyright statement?	yuripv	Nov. 15, 2018, 12:27 p.m.
Do I understand correctly that before this change python-35 was not hardcoded in manifest? Should we introduce one more parameter ...	alp	Nov. 15, 2018, 12:27 p.m.
This was removed why?	yuripv	Nov. 14, 2018, 3:46 p.m.
How will it behave if run outside of git repository?	alp	Nov. 19, 2018, 1:10 p.m.

Bugs:

+	9979

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?

citrus Nov. 14, 2018, 3:57 p.m.

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

yuripv Nov. 14, 2018, 7:01 p.m.

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?

usr/src/data/locale/tools/mkwidths.py (Diff revision 1)
Show all issues
```
Can this be moved to the other copyright statement?
```

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

Show all issues

This was removed why?

citrus Nov. 14, 2018, 3:46 p.m.

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

yuripv Nov. 14, 2018, 3:47 p.m.
```
Got it, sorry.
```

Testing Done:

		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 `BUILDPY3` is not set to an octothorpe;
		allows the python2 modules to be omitted from the onbld package via `BUILDPY2='#'`;
		Provides a variable through which the default interpreter (shebang) for these can be set - `TOOLS_PYTHON` - which defaults to `$(PYTHON)`.

		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 `BUILDPY2='#'` 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 four test builds with `BUILDPY2='#'`, `BUILDPY3='#'`, both and neither set. Inspected developer/build/onbld package contents each time.
	~	Did three test builds with `BUILDPY2='#'`, `BUILDPY3='#'` and neither set. Inspected developer/build/onbld package contents each time.

Ship it!

```
Ship It!
```

usr/src/pkg/manifests/developer-build-onbld.mf (Diff revision 1)

Show all issues

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?

citrus Nov. 15, 2018, 10:19 a.m.

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

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.

citrus Nov. 15, 2018, 10:19 a.m.
```
Good point, I'll fix.
```

Change Summary:


Use new BUILDPY2TOOLS and BUILDPY3TOOLS variables to control building of tool modules (thanks ptribble);
Replace hard-coded 3.5 and -35 in manifest with variables (thanks alp);
Fix problems when checking files containing non-ASCII characters found during further testing;
Add missing copyrights and some small whitespace fixes to satisfy pbchk.

Commit:

-	565bbde73164d98c464881abb6ebc1693a5e92ff
+	e19ce2584121dec1e34a31e5f4f33b73088b189b

Diff:

Revision 2 (+1030 -476)

Show changes

	usr/src/Makefile.master
	usr/src/data/locale/tools/mkwidths.py
	usr/src/pkg/Makefile
	usr/src/pkg/manifests/developer-build-onbld.mf
	usr/src/tools/Makefile.python
	usr/src/tools/Makefile
	usr/src/tools/onbld/Makefile.com
	usr/src/tools/onbld/Makefile
	26 more

Change Summary:

Automatically set TOOLS_PYTHON to $(PYTHON3) if BUILDPY2TOOLS='#'

Commit:

-	e19ce2584121dec1e34a31e5f4f33b73088b189b
+	3fd0e32366e4d62250dfb0d2284dab61de24f240

Diff:

Revision 3 (+1031 -476)

Show changes

	usr/src/Makefile.master
	usr/src/data/locale/tools/mkwidths.py
	usr/src/pkg/Makefile
	usr/src/pkg/manifests/developer-build-onbld.mf
	usr/src/tools/Makefile.python
	usr/src/tools/Makefile
	usr/src/tools/onbld/Makefile.com
	usr/src/tools/onbld/Makefile
	26 more

Change Summary:

Updating test notes to reflect new switch variable names and record additional testing.

Testing Done:

		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 `BUILDPY3` is not set to an octothorpe;
~		allows the python2 modules to be omitted from the onbld package via `BUILDPY2='#'`;
~		Provides a variable through which the default interpreter (shebang) for these can be set - `TOOLS_PYTHON` - which defaults to `$(PYTHON)`.
	~	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 `BUILDPY2='#'` will achieve that. OmniOS bloody no longer uses python2 for the onbld components.
	~	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 `BUILDPY2='#'`, `BUILDPY3='#'` and neither set. Inspected developer/build/onbld package contents each time.
	~	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.

Ship it!

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

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

Show all issues

How will it behave if run outside of git repository?

citrus Nov. 19, 2018, 1:10 p.m.


% 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.

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Needs update to fix use of PYTHON_VERSION.

Status: Re-opened

Change Summary:

Re-based on master and updated so that the build works properly if PYTHON_VERSION etc. are set explicitly in the .env file

Commit:

-	3fd0e32366e4d62250dfb0d2284dab61de24f240
+	2a85fdaec163a9d9a87599149848de0a3acf3d7a

Diff:

Revision 4 (+1039 -480)

Show changes

	usr/src/Makefile.master
	usr/src/data/locale/tools/mkwidths.py
	usr/src/pkg/Makefile
	usr/src/pkg/manifests/developer-build-onbld.mf
	usr/src/tools/Makefile.python
	usr/src/tools/Makefile
	usr/src/tools/onbld/Makefile.com
	usr/src/tools/onbld/Makefile
	26 more

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

Ship it!

```
Ship It!
```

You have a pending review.

Review Board 3.0.18

9979 Support python3 for in-gate tools

Testing

Bugs:

Testing Done:

Testing

Change Summary:

Commit:

Diff:

Change Summary:

Commit:

Diff:

Change Summary:

Testing Done:

Testing

Status: Closed (submitted)

Change Summary:

Status: Re-opened

Change Summary:

Commit:

Diff:

Status: Closed (submitted)