Skip to content

Call llvm-lit the right way #202

Merged
h-vetinari merged 4 commits into
conda-forge:mainfrom
sumit0190:main
Mar 15, 2023
Merged

Call llvm-lit the right way #202
h-vetinari merged 4 commits into
conda-forge:mainfrom
sumit0190:main

Conversation

@sumit0190

Copy link
Copy Markdown
Contributor

Checklist

Fixes #200.

@conda-forge-webservices

Copy link
Copy Markdown

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@sumit0190

sumit0190 commented Mar 11, 2023

Copy link
Copy Markdown
Contributor Author

The logs show that the tests run correctly for win-64.

Testing Time: 610.80s
  Unsupported      :   584
  Passed           : 12968
  Expectedly Failed:    64

@h-vetinari h-vetinari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The osx timeouts are unrelated

Comment thread recipe/bld.bat Outdated

cd ..\llvm\test
..\..\build\bin\llvm-lit.py -vv Transforms ExecutionEngine Analysis CodeGen/X86
%SYS_PYTHON% ..\..\build\bin\llvm-lit.py -vv Transforms ExecutionEngine Analysis CodeGen/X86

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, SYS_PYTHON is not - AFAIK - a python known to conda (see e.g. here).

Is there a reason you chose that one, and why don't we simply use the python from the build env that should be on the path already? E.g.

Suggested change
%SYS_PYTHON% ..\..\build\bin\llvm-lit.py -vv Transforms ExecutionEngine Analysis CodeGen/X86
python ..\..\build\bin\llvm-lit.py -vv Transforms ExecutionEngine Analysis CodeGen/X86

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question. I've not found the time to look deeper into this, so I'll just note down my observations here and hope someone else can also help figure this out.

  1. SYS_PYTHON is recognized, and is also used in other recipes. See here, for instance.
  2. On linux, the llvm-lit.py script has this at the top: #!/home/sumit/miniconda3/envs/llvmtest/bin/python3.10. So you can see that it's actually calling the SYS_PYTHON, and this isn't even something that we manually configure (AFAIK, again, I haven't looked at the details).
  3. But on win-64, the llvm-lit.py script has this at the top: #!C:/Users/sumit/miniconda3/llvmtest/conda-bld/llvm-package_1678419847679/_build_env/python.exe. So this time it is actually trying to call the local Python executable...
  4. So what happens if I call it with the executable it wants me to use? This might be something wrong with Anaconda's Python, but this is what I see:
C:\Users\sumit\miniconda3\llvmtest\conda-bld\llvm-package_1678419847679\work\llvm\test>python ..\..\build\bin\llvm-lit.py -vv Transforms ExecutionEngine Analysis CodeGen/X86
Traceback (most recent call last):
  File "C:\Users\sumit\miniconda3\llvmtest\conda-bld\llvm-package_1678419847679\work\build\bin\llvm-lit.py", line 21, in <module>
    from pathlib import Path
  File "C:\Users\sumit\miniconda3\llvmtest\Lib\pathlib.py", line 1, in <module>
    import fnmatch
  File "C:\Users\sumit\miniconda3\llvmtest\Lib\fnmatch.py", line 14, in <module>
    import re
  File "C:\Users\sumit\miniconda3\llvmtest\Lib\re.py", line 125, in <module>
    import sre_compile
  File "C:\Users\sumit\miniconda3\llvmtest\Lib\sre_compile.py", line 17, in <module>
    assert _sre.MAGIC == MAGIC, "SRE module mismatch"
AssertionError: SRE module mismatch

But that's clearly messed up and expected in a way: it's trying to load the libraries for the SYS_PYTHON, probably because of incorrect path resolution somewhere. This is the part I haven't had the time to debug, so I'd appreciate any help here.

Still, it's worth noting that the linux version does call SYS_PYTHON, so maybe this could be an issue for later and we can simply make the change now. I leave that decision up to you.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, that sounds like it's accidentally picking up the wrong python (and somehow baking this into the produced llvm-lit). Could you try adding python (or $PYTHON) also to the lit-invocation in build.sh?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, on Linux both the SYS_PYTHON and the local Python work with the script. But I suppose for the sake of consistency we should either figure out why Windows doesn't work with it, or keep Linux the way it is now and make Windows use SYS_PYTHON.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I said, we should not use SYS_PYTHON anywhere, including a specific suggestion to (hopefully) fix this for both unix/windows.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's just something wrong with my system - I can try making the change and we'll see what happens. If it fails onwin-64 like it does for me, you (or someone else) might have a better idea of what's going on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@h-vetinari Looks like that worked! On my local machine that doesn't, and for some reason %PYTHON% isn't defined either (so I had to use python directly). Let me know if that looks good.

@h-vetinari

Copy link
Copy Markdown
Member

Thanks @sumit0190! :)

@h-vetinari h-vetinari merged commit ba5335b into conda-forge:main Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

llvm-lit.py is not run on windows in bld.bat

3 participants