RFC: mysql: With universal_newlines, python3 subprocess assumes that...

RFC: mysql: With universal_newlines, python3 subprocess assumes that communicate() parameter is str() (not bytes()).

But mysql_script_file was opened as a binary files and thus a bytes() was returned:
    File "etc/run/mariadb_update", line 51, in <module>
      sys.exit(slapos.recipe.generic_mysql.mysql.updateMysql(mysql_upgrade_binary='/srv/slapgrid/slappart3/bin/mysql_upgrade', mysql_binary='/srv/slapgrid/slappart3/bin/mysql', mysql_script_file='/srv/slapgrid/slappart3/etc/mariadb_initial_setup.sql'))
    File "parts/slapos.cookbook-repository/slapos/recipe/generic_mysql/mysql.py", line 31, in updateMysql
      result = mysql.communicate(mysql_script)[0]
    File "parts/python3/lib/python3.7/subprocess.py", line 964, in communicate
      stdout, stderr = self._communicate(input, endtime, timeout)
    File "parts/python3/lib/python3.7/subprocess.py", line 1692, in _communicate
      self._save_input(input)
    File "parts/python3/lib/python3.7/subprocess.py", line 1774, in _save_input
      self._input = self._input.encode(self.stdin.encoding,
  AttributeError: 'bytes' object has no attribute 'encode'

slapos/recipe/generic_mysql/mysql.py

@@ -8,7 +8,7 @@ import pytz
 
 def updateMysql(mysql_upgrade_binary, mysql_binary, mysql_script_file):
   sleep = 0
-  with open(mysql_script_file, 'rb') as script_file:
+  with open(mysql_script_file, 'r') as script_file:
     mysql_script = script_file.read()
   mysql_list = mysql_binary, '-B'
   mysql_tzinfo_to_sql_list = (

[Jérome Perrin]

this is also blocking the python3 port of lamp softwares, maybe this commit is good to go already.

On the other hand, we configure mariadb with utf-8 so we could assume it's an utf-8 text file

[Jérome Perrin]

that's no longer true we now allow to configure mariadb's encoding b8584e14

but that file is created by us, so we know the encoding ... if we want to read it as text ... reading it as bytes and sending the bytes also seem completely OK.

edit: this comment is not clear I was misunderstanding the patch when I posted. That patch looks ok as it is

[Jérome Perrin]

/cc @HongzheWang

[Jérome Perrin]

this recipe does not have tests either ...

[Arnaud Fontaine]

@jerome Thank you for the review. Running ERP5 Unit Tests (for_testrunner branch) is enough before pushing, right?

[Jérome Perrin]

The "problem" of the code in slapos recipes is that for this code to be used we also need to make a new release of slapos.cookbook , upload it on pypi and use it in stack/slapos.cfg.

Ideally, this recipe should have an unit test and running the test should give us enough confidence to make a release, but there's no unit test, we only have integration tests that run after making a release.

If you tried the patch locally and it worked, we can do an optimistic approach: you can push to master, I'll make a release tomorrow and update the profile.

I just have a minor suggestion, since 'r' is the default mode both, maybe we want to omit it.

[Arnaud Fontaine]

I see. Yes this patch was tested locally and worked. I will push it to master then. Thanks!

[Jérome Perrin]

Thanks ! this is now released and slapos master uses the new version since 16656e00

@HongzheWang this should allow progress for lamp on python3