Build with python3 on unix

[omajid]

If I am reading it correctly, the windows build scripts (build.cmd) try finding python in order of python3, python2 and then python:

coreclr/build.cmd

Line 379 in b4131b9

echo import sys; sys.stdout.write(sys.executable) | (py -3 || py -2 || python3 || python2 || python) > %TEMP%\pythonlocation.txt 2> NUL

The unix build scripts don't. They just try python2 variants and then fail. This change makes brings them closer together by letting users build using only python3 on unix.

The support for using python3 for Windows was added here: 54a362c (#15611)

As an side, I see both the unix build scripts and Windows build scripts test for python and fail if it's not found. However CMakeLists.txt also tests for python again (and then doesn't use it?):

coreclr/CMakeLists.txt

Lines 40 to 43 in b4131b9

	find_program(PYTHON NAMES python2.7 python2 python)
	if (PYTHON STREQUAL "PYTHON-NOTFOUND")
	message(FATAL_ERROR "PYTHON not found: Please install Python 2.7.9 or later from https://www.python.org/downloads/")
	endif()

Is that useful to keep? It will prevent someone from building when python3 is available but not python2 or python.

[omajid]

Guessing some CC's based on reviewers on the linked PR @gkhanna79 @leemgs @jkotas @janvorli @BruceForstall @nategraf

[BruceForstall]

I don't build on Linux much, but this seems reasonable to me. I there a reason to drop/replace the "python2.7" check instead of just adding the python3 check?

As for:

# Ensure that python is present                                                                                                                                                                                                                                                          find_program(PYTHON NAMES python2.7 python2 python)                                                                                                                                                                                                                                      if (PYTHON STREQUAL "PYTHON-NOTFOUND")                                                                                                                                                                                                                                                       message(FATAL_ERROR "PYTHON not found: Please install Python 2.7.9 or later from https://www.python.org/downloads/")                                                                                                                                                                 endif()                                                                                                                          

it does seem like this is not used. Maybe you should add a python3 check here as well, to be consistent?

It looks like src\build.proj has:

<Exec Command="python $(MSBuildThisFileDirectory)scripts\pgocheck.py @(PGOEnforcedFiles)" />  

but that doesn't look like it's using the cmake found PYTHON.

• @jashook

[jashook]

We have a few different moving parts that have python dependencies. Currently the expected/supported version is 2.7; however, long term I believe we are planning to move to python3. That being said there are several other scripts which use python3 explicitly. Pulling in some people who are more familiar with other dependencies: @adiaaida @jorive @nategraf

I can speak to the build/run steps of coreclr's infrastructure. We currently have a requirement on python 2.7, and during build time I believe we should be enforcing this requirement until we move off it to python3.

Note that python isn't explicitely during the build, the cmake addition is basically a captcha for whether your system is setup to build and run tests with coreclr.

[jashook]

@RussKeldorph @BruceForstall opinions?

[RussKeldorph]

It sounds like we need to get to python3 soon if we are going to keep a python dependency, but we should enforce what the build scripts require. We should first make the build scripts work with either version and then start enforcing python3 after a grace period where we print out big warnings that 2.7 will no longer sufficient.

[BruceForstall]

My opinion would be that we can officially depend on 2.7, and make that primary. But if 3 "works", then that's fine. And if it "works" on 3 on Windows but not on Linux, due to this minor script issue, then adding support for Linux makes sense.

[jorive]

@jashook Performance infrastructure relies on >=Python3.5

[omajid]

@BruceForstall said:

I don't build on Linux much, but this seems reasonable to me. I there a reason to drop/replace the "python2.7" check instead of just adding the python3 check?

No, I just wanted to bring Linux closer in line to Windows.

As for:

# Ensure that python is present

it does seem like this is not used. Maybe you should add a python3 check here as well, to be consistent?

Would it make more sense to just remove it? If it's not being used (I don't know this for sure), why keep the code around?

It looks like src\build.proj has:

but that doesn't look like it's using the cmake found PYTHON.

Thanks for pointing me to this. I fixed a variant of this (#19043) but missed this one because it was not getting executed on my machine. I will open a separate PR for this, which should be much less controversial.

[omajid]

It sounds like we need to get to python3 soon if we are going to keep a python dependency

Yes! We have about a year to move away from python 2: https://pythonclock.org/

but we should enforce what the build scripts require.

How do we deal with the difference between what the Windows build scripts currently require - 3 is preferred over 2 - with what the *nix build scripts do - 2 without any fallback ?

We should first make the build scripts work with either version and then start enforcing python3 after a grace period where we print out big warnings that 2.7 will no longer sufficient.

Okay. I can do that.

[jashook]

Yes! We have about a year to move away from python 2: https://pythonclock.org/

I know its so sad :(

[jashook]

How do we deal with the difference between what the Windows build scripts currently require - 3 is preferred over 2 - with what the *nix build scripts do - 2 without any fallback ?

Currently for scripting used by coreclr the (somewhat poorly enforced) requirement is python2, for both Windows and Unix. We have python3 dependencies and will continue to get more as python2 goes away. As Russ mentioned, for now we should support either python3 or python2, but enforce having python2.

In the longer term there are scripts we need to port to python3. This is mostly to clarify.

[BruceForstall]

@omajid Is this still active?

@jashook Assuming it's still a valid change, do you think it requires more work, or is it ok?

[omajid]

@BruceForstall I would like to get this in, yes. We are even closer to the python2 EOL date and the build scripts don't even support python3 out of the box yet :(

I don't know how to meet this request from earlier in this thread:

but we should enforce what the build scripts require. We should first make the build scripts work with either version and then start enforcing python3 after a grace period where we print out big warnings that 2.7 will no longer sufficient.

[jashook]

I'm good with the change.

[jashook]

I thinks its good to merge once testing is done.

[jashook]

@RussKeldorph @BruceForstall @echesakov for a second opinion

[jashook]

@omajid in regards to you comment, we have been slowly changing the python scripts to use both python3 and python2. Recently both runtest.py, run-pmi-diffs.py have both been changed to support python3, so this has mostly been in flight. I believe we are at the point where we can start moving entirely to python3 and give a warning if python2 is the only dependency found.

I don't know how to meet this request from earlier in this thread:

but we should enforce what the build scripts require. We should first make the build scripts work with either version and then start enforcing python3 after a grace period where we print out big warnings that 2.7 will no longer sufficient.

[jashook]

Failures look unrelated merging. Thanks for the contribution!