bpo-31955: Fixed incorrect string type check - use isinstance(value, basestring)… by PavelStishenko · Pull Request #4316

bpo-31955: Fixed incorrect string type check - use isinstance(value, basestring)… by PavelStishenko · Pull Request #4316 · python/cpython

Fixed incorrect string type check. Previously if unicode string was passed as executable value it was considered not as string, but as list.

https://bugs.python.org/issue31955

Previous PR on this issue (#4296) was mistaken - for Python3 str is equivalent to basestring.

… instead of isinstance(value, str)

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

Thanks again to your contribution and we look forward to looking at it!

This PR is instead of PR #4296 . I agree, that this fix is necessary only for Python 2.

		distutils.CCompiler.set_executables() method. Previously if unicode string
		was passed as executable value it was considered not as string, but as list.
		It caused TypeError: "coercing to Unicode: need string or buffer, list
		found" at unixccompiler.py, line 122, in _compile().

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NEWS entries are read by end users who don't care much to details. I suggest to be much more concise. For example:

Fix CCompiler.set_executable() of distutils to handle properly Unicode strings.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Do you think you could add tests for set_executables in Lib/distutils/tests/test_ccompiler.py ?


		def test_set_executables_unicode(self):
		class MyCCompiler(CCompiler):
		executables = {'compiler':'', 'compiler_cxx':'', 'linker':''}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP 8: {'compiler': '', 'compiler_cxx': '', 'linker': ''}


		class CCompilerTestCase(support.EnvironGuard, unittest.TestCase):

		def test_set_executables_unicode(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rename to test_set_executable(), since you test 3 types, not only Unicode.

		compiler = MyCCompiler()

		# set executable as list
		compiler.set_executables(compiler = ['/usr/bin/env', 'OMPI_MPICC=clang', '/usr/bin/mpicc.openmpi'])

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP 8: no space around = when passing keyword argument: set_executables(compiler=[...])


		# set executable as list
		compiler.set_executables(compiler = ['/usr/bin/env', 'OMPI_MPICC=clang', '/usr/bin/mpicc.openmpi'])
		print(compiler.executables)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove debug prints please.

		compiler.set_executables(compiler = ['/usr/bin/env', 'OMPI_MPICC=clang', '/usr/bin/mpicc.openmpi'])
		print(compiler.executables)
		self.assertEqual(len(compiler.executables['compiler']) == 3)
		self.assertEqual(len(compiler.executables['compiler'][2]) == '/usr/bin/mpicc.openmpi')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum, I dislike testing only one value. Please test:

self.assertEqual(compiler.executables['compiler'], ...) # fill the dots ;-)

		# set executable as unicode string
		compiler.set_executables(linker = u'/usr/bin/env OMPI_MPICXX=clang++ /usr/bin/mpicxx.openmpi')
		self.assertEqual(len(compiler.executables['linker']) == 3)
		self.assertEqual(len(compiler.executables['linker'][2]) == '/usr/bin/mpicxx.openmpi')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use a Unicode string here as well.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good

I have made the requested changes; please review again.

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.


		# set executable as list
		compiler.set_executables(compiler=['env', 'OMPI_MPICC=clang', 'mpicc'])
		self.assertEqual(len(compiler.compiler), 3)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking the length became redundant with the following check and so should be removed.

		'mpicxx'])

		# set executable as unicode string
		compiler.set_executables(linker=u'env OMPI_MPICXX=clang++ mpicxx')

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please use a different string, like "env OMPI_MPICXX=clang++ linker" or whatever different than the previous test.

The change now looks to me, but I have a last minor request, sorry!

I have made the requested changes; please review again.

Thanks for making the requested changes!

@Haypo: please review the changes made to this pull request.

Hum, @Mazay0, it seems like you didn't sign the CLA. What is your bugs.python.org login? If you are Dee Mee, please fill your GitHub login in your bugs.python.org profile: click on "Your Details".
https://bugs.python.org/user27322

If you are Dee Mee, please fill your GitHub login in your bugs.python.org profile: click on "Your Details".

Done!

Done!

I confirm: after removing the "CLA not signed" label, the bot added the green "CLA signed" label.

Thanks @Mazay0 and congratulations for your first PR! 🎉