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

Update Python support scripts to Python 3.8 from Python 2. #24

Merged
merged 5 commits into from
Feb 8, 2021

Conversation

billyquith
Copy link

Python 2.7 is now deprecated.

@b-camacho
Copy link

I tried a sample build, and hit this error:

C:\Users\Wojtyla.DESKTOP-U5BDE0N\open-brush\Support\bin>build --platform Windows --vrsdk SteamVR
Project dir: C:\Users\Wojtyla.DESKTOP-U5BDE0N\open-brush
Traceback (most recent call last):
  File "C:\Users\Wojtyla.DESKTOP-U5BDE0N\open-brush\Support\bin\build.py", line 24, in <module>
    unitybuild.main.main()
  File "C:\Users\Wojtyla.DESKTOP-U5BDE0N\open-brush\Support\Python\unitybuild\main.py", line 777, in main
    revision = vcs.get_build_stamp(project_dir)
  File "C:\Users\Wojtyla.DESKTOP-U5BDE0N\open-brush\Support\Python\unitybuild\vcs.py", line 128, in get_build_stamp
    for match in re.finditer(r'^(.[MADR]|[MADR].) (.*)', status, re.MULTILINE):
  File "C:\Users\Wojtyla.DESKTOP-U5BDE0N\AppData\Local\Programs\Python\Python39\lib\re.py", line 248, in finditer
    return _compile(pattern, flags).finditer(string)
TypeError: cannot use a string pattern on a bytes-like object

Looks like this method in vsc.py

def git(cmd, cwd=None):
  """Runs git, returns stdout.
Raises CalledProcessError if process cannot be started, or exits with an error."""
  if cwd is None:
    cwd = os.getcwd()
  if type(cmd) is str:
    cmd = ['git'] + cmd.split()
  else:
    cmd = ['git'] + list(cmd)

  try:
    proc = Popen(cmd, cwd=cwd, stdout=PIPE, stderr=PIPE)
  except OSError as e:
    raise CalledProcessError(1, cmd, str(e))

  (stdout, stderr) = proc.communicate()
  if proc.wait() != 0:
    raise CalledProcessError(proc.wait(), cmd, "In %s:\nstderr: %s\nstdout: %s" % (
      cwd, stderr, stdout))
  return stdout

Tries to get a string out of proc.communicate(), but python3 gives bytes

@billyquith
Copy link
Author

I had that problem elsewhere. It can be fixed by putting str(bytes). We'll look at later.

@mikeskydev mikeskydev added the enhancement Feature added label Jan 31, 2021
@mikeage
Copy link
Member

mikeage commented Feb 1, 2021

I had a few issues with building on Mac OS; the full build is in progress, but so far, here's what I've changed (including the bytes->str fix). I think this should all be backwards compatible.

Full diff
commit 257a9c6bb659f375f6d2c66a6d781deae89f1b0d
Author: Mike Miller <[email protected]>
Date:   Mon Feb 1 05:11:16 2021 +0200

    Broaden hacky regex to support MacOS Paths

diff --git a/Support/Python/unitybuild/main.py b/Support/Python/unitybuild/main.py
index 278d2e80..10f62204 100644
--- a/Support/Python/unitybuild/main.py
+++ b/Support/Python/unitybuild/main.py
@@ -264,9 +264,9 @@ def get_editor_unity_version(editor_app, editor_data_dir):
 
   # I can't find a way to get the version out of 2019.x.
   # This is pretty janky so only use for Jenkins and 2019.
-  for m in re.finditer(r'/Users/jenkins/JenkinsCommon/Unity/Unity_(2019)\.(\d+)\.(\d+)',
+  for m in re.finditer(r'Unity/(Unity_)?(2019)\.(\d+)\.(\d+)',
                        editor_data_dir):
-    major, minor, point = m.groups()
+    _, major, minor, point = m.groups()
     ret = (major, minor, point)
     print("WARNING: %s using fallback to determine Unity version %s" % (editor_data_dir, ret))
     return ret

commit 261536dd7a15208c32c9359e2706a31989e95b33
Author: Mike Miller <[email protected]>
Date:   Mon Feb 1 05:09:45 2021 +0200

    Include Unity installed by Unity Hub on Mac

diff --git a/Support/Python/unitybuild/main.py b/Support/Python/unitybuild/main.py
index 39ac0167..278d2e80 100644
--- a/Support/Python/unitybuild/main.py
+++ b/Support/Python/unitybuild/main.py
@@ -212,6 +212,8 @@ def iter_editors_and_versions():
     else:
       # TODO: make it work with Unity hub?
       app_list = ['/Applications/Unity/Unity.app']
+      # Since we don't have Unity hub support (commented out above because headless isn't headless), look for where it installs directly
+      app_list.extend(glob.glob('/Applications/Unity/*/Unity.app'))
     for editor_dir in app_list:
       exe = os.path.join(editor_dir, 'Contents/MacOS/Unity')
       editor_data_dir = os.path.join(editor_dir, 'Contents')

commit 3387e94dd9e4f3a84659da1351e045b9389324e7
Author: Mike Miller <[email protected]>
Date:   Mon Feb 1 05:09:19 2021 +0200

    Fix bytes returned by git()

diff --git a/Support/Python/unitybuild/vcs.py b/Support/Python/unitybuild/vcs.py
index 106938c5..91d6b739 100644
--- a/Support/Python/unitybuild/vcs.py
+++ b/Support/Python/unitybuild/vcs.py
@@ -44,7 +44,7 @@ Raises CalledProcessError if process cannot be started, or exits with an error."
   if proc.wait() != 0:
     raise CalledProcessError(proc.wait(), cmd, "In %s:\nstderr: %s\nstdout: %s" % (
       cwd, stderr, stdout))
-  return stdout
+  return str(stdout)
 
 
 def create():

@mikeage
Copy link
Member

mikeage commented Feb 1, 2021

Yep, ran beautifully. The only thing left is that the apk is named TiltBrush.apk, and I suspect the .exe has the same problem ;-)

@billyquith
Copy link
Author

Added those fixes. I'm dev Windows 10 on a steam powered 2011 machine with Quest 1 target.

@mikeage
Copy link
Member

mikeage commented Feb 1, 2021

Thanks! One last suggestion before you merge this -- change the output name from TiltBrush to OpenBrush ;-)

@billyquith
Copy link
Author

Thanks! One last suggestion before you merge this -- change the output name from TiltBrush to OpenBrush ;-)

I was doing a general rename, I don't suppose that is need for the branding but it is good consistency.

@mikeage
Copy link
Member

mikeage commented Feb 1, 2021

It's probably not terribly urgent, and in any case, the output name still differs from the GUI build, but I think https://github.com/icosa-gallery/open-brush/pull/24/files#diff-887155a2240f26346d22317f3fa810889b30e5ebc176ff585f5ee0e3e571f126R61 should be changed. (I actually noticed that when looking at some python warnings, such as the next line not being in use at all)

Edit: sorry, it didn't get the line number correct. That's main.py, line 61

@mikeage
Copy link
Member

mikeage commented Feb 4, 2021

@billyquith , when you run this, does it update Support/ThirdParty/GeneratedThirdPartyNotices.txt with a unicode related change and re-ordering GraphicsAnton? As this script is the only place that this file is updated, if you're seeing the same diff, maybe include the update here as well?

This is what I see
diff --git i/Support/ThirdParty/GeneratedThirdPartyNotices.txt w/Support/ThirdParty/GeneratedThirdPartyNotices.txt
index b18d21b7..bf36ac42 100644
--- i/Support/ThirdParty/GeneratedThirdPartyNotices.txt
+++ w/Support/ThirdParty/GeneratedThirdPartyNotices.txt
@@ -9,7 +9,7 @@ https://www.geometrictools.com/Documentation/DistancePointEllipseEllipsoid.pdf

 ^L
 === FluxJpeg.Core ===
-Copyright (c) 2008-2009 Occipital Open Source
+<U+FEFF>Copyright (c) 2008-2009 Occipital Open Source

 Partial derivations: See IJG.txt and JAI.txt.

@@ -103,12 +103,6 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
 THE SOFTWARE.


-^L
-=== GraphicsAnton ===
-This product includes software developed by Anton Kirczenow.
-https://www.shadertoy.com/view/ldsGWX
-
-
 ^L
 === Ionic.Zip ===
 DotNetZip - Copyright (c) 2006 - 2011 Dino Chiesa
@@ -170,3 +164,9 @@ All rights reserved.
 This product includes software developed at:

 Pixar (http://www.pixar.com/).
+
+^L
+=== GraphicsAnton ===
+This product includes software developed by Anton Kirczenow.
+https://www.shadertoy.com/view/ldsGWX
+

Base automatically changed from master to main February 4, 2021 11:47
@billyquith
Copy link
Author

@billyquith , when you run this, does it update Support/ThirdParty/GeneratedThirdPartyNotices.txt with a unicode related change and re-ordering GraphicsAnton? As this script is the only place that this file is updated, if you're seeing the same diff, maybe include the update here as well?

Unicode/strange characters may be more byte/char issues. I'll have a look later. I don't believe I've done anything to change the scanning/export of this information.

@mikeage
Copy link
Member

mikeage commented Feb 4, 2021

It probably is byte/char/str. I just wanted to confirm that you were seeing the file updated, and if so, proposed that you commit the update as well.

@mikeage mikeage mentioned this pull request Feb 7, 2021
@billyquith
Copy link
Author

It probably is byte/char/str. I just wanted to confirm that you were seeing the file updated, and if so, proposed that you commit the update as well.

I ran this again and I don't get unicode/byte issues on an Oculus, Android build. I don't know if this a locale issue?

Other than that are there other issues stopping this going in?

@mikeage
Copy link
Member

mikeage commented Feb 8, 2021

I don't think this is a blocker at all. As far as I'm concerned, it's ready

@mikeskydev
Copy link
Member

Works great, thank you all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants