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

BUG: Update MSVC compiler version for IntelCCompiler #13492

Closed
wants to merge 2 commits into from
Closed

BUG: Update MSVC compiler version for IntelCCompiler #13492

wants to merge 2 commits into from

Conversation

r-mike
Copy link

@r-mike r-mike commented May 7, 2019

Build numpy with intelw compiler currently fails with the error:
Microsoft Visual C++ 14.1 is required. Get it with "Microsoft Visual C++ Build Tools": https://visualstudio.microsoft.com/downloads/
even when the VS toolset 14.1 is installed. The IntelCCompiler uses
an old MSVC version to compile. This commit updates the MSCV compiler version, which fixes the error.

Build numpy with intelw compiler currently fails with the error:
Microsoft Visual C++ 14.1 is required. Get it with "Microsoft Visual C++ Build Tools": https://visualstudio.microsoft.com/downloads/
even when the VS toolset 14.1 is installed. The IntelCCompiler uses
an old MSVC version to compile. This commit updates the MSCV compiler version, which fixes the error.
@mattip
Copy link
Member

mattip commented May 8, 2019

Why do you need the new file? It seems exactly like numpy/distutils/msvc9compiler.py, or did I miss something subtle

self.linker = self.find_exe('xilink')
self.cc = _find_exe('icl.exe')
self.lib = _find_exe('xilib')
self.linker = _find_exe('xilink')
Copy link
Member

@mattip mattip May 8, 2019

Choose a reason for hiding this comment

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

At least for new code (python 3) _find_exe should be spelled shutil.which to avoid using "private" interfaces. If this is to be backported, then distutils.spawn.find_executable function is still a better, although seemingly undocumented, alternative.

Copy link
Author

Choose a reason for hiding this comment

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

Since this is a backport-candidate I think I should use distutils.spawn.find_executable. There is also another find_executable in numpy.distutils.exec_command. I compare results of the different find_executable on Monday.

Copy link
Author

Choose a reason for hiding this comment

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

With the new commit I use distutils.spawn.find_executable. Since distutils._msvccompiler returns the original program name if no path is found, I created a wrapper around distutils.spawn.find_executable to ensure that the both functions are having the same results. A few tests are failing at the moment, but I can't see how my changes are causing these tests to fail. Do I miss something?

@r-mike
Copy link
Author

r-mike commented May 9, 2019

Yes, it is almost the same file, except for the import from distutils._msvccompiler import MSVCCompiler as _MSVCCompiler. My first thought was to make a new file for keeping support with the older versions of VS like in python distutils. The header in distutils._msvccompiler says:

"""distutils._msvccompiler

Contains MSVCCompiler, an implementation of the abstract CCompiler class
for Microsoft Visual Studio 2015.

The module is compatible with VS 2015 and later. You can find legacy support
for older versions in distutils.msvc9compiler and distutils.msvccompiler.
"""

However, I found a comment in distutils._msvccompiler, which says that VS 2005 and VS 2008 is also supported: # ported to VS 2005 and VS 2008 by Christian Heimes. Since distutils._msvccompiler then should support >=VS2005 numpy/distutils/msvc9compiler.py should be obsolete and can be replaced by numpy/distutils/_msvccompiler.py. Any thoughts on this?

Implemented _find_exe with the same behaviour as distutils._msvccompiler._find_exe.
def initialize(self, plat_name=None):
# The 'lib' and 'include' variables may be overwritten
# by MSVCCompiler.initialize, so save them for later merge.
environ_lib = os.getenv('lib')
Copy link
Member

Choose a reason for hiding this comment

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

What happens when this key is missing? Maybe os.environ['libs'] -> os.environ.get('libs', ''), and below as well.

@charris charris removed the 09 - Backport-Candidate PRs tagged should be backported label Apr 17, 2020
Base automatically changed from master to main March 4, 2021 02:04
@charris
Copy link
Member

charris commented Jun 9, 2022

@r-mike I'm going to close this. If you still need this enhancement, feel free to open an new PR.

@charris charris closed this Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants