Page MenuHomePhabricator

Autogenerate the shebang lines for tools/opt-viewer
ClosedPublic

Authored by cbiesinger on Aug 8 2019, 4:19 PM.

Details

Summary

Since these files depend on the built python modules, they need to use
the right python binary to run them. So use configure_file
to set the right shebang line.

Diff Detail

Repository
rL LLVM

Event Timeline

cbiesinger created this revision.Aug 8 2019, 4:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 8 2019, 4:19 PM

This seems .... somewhat unfortunate. Adding Chris Bieneman to see if this is really the right thing to do or there are any other alternatives.

beanz added a comment.Aug 16 2019, 3:05 PM

Sadly I'm not sure that there is a better way than generating the shebang line, but I can imagine some problems with the way this patch does it. PYTHON_EXECUTABLE could be a full path, which means that this can't be used to generate binary distributions because you'd be relying on python being in the same place on disk, which defeats the purpose of env being on the shebang.

Chris is correct, PYTHON_EXECUTABLE will be an absolute path. For this to work you'll need to remove /usr/bin/env.

Use python@PYTHON_VERSION_MAJOR@

It actually worked with the env for me.

Anyway, beanz, does this address your concern? It uses #!/usr/bin/env python@PYTHON_VERSION_MAJOR@

beanz added a comment.Aug 16 2019, 3:20 PM

It actually worked with the env for me.

It will always work locally, it just may not work in a binary distribution (i.e. take your built binaries and move them somewhere else with a different path to python).

Anyway, beanz, does this address your concern? It uses #!/usr/bin/env python@PYTHON_VERSION_MAJOR@

Is there any situation where the python tool isn't named that way? What happens if PYTHON_EXECUTABLE points to a Python 3.2 install, but my system default is Python 3.5? Doesn't that make the python3 link point to python35?

What about using CMake's get_filename_component (https://cmake.org/cmake/help/v3.0/command/get_filename_component.html) function to find the name of the executable? That seems more robust.

Use python's basename

I'm not sure that such a python3 vs 3.5 difference matters, but anyway, here's a version that does arguably the right thing.

At least for the from-source distribution, where the destination of the image is the build host, Python_EXECUTABLE is definitely preferable (e.g. #!@Python_EXECUTABLE@). Furthermore, there is no reason to assume that /usr/bin/env python3 will give you the same python 3 as what was given to cmake (you can control that with -DPython_EXECUTABLE=...). I think that requiring the user to specify the tool is possibly the best option - that is completely remove the shebang as that ensures that shifts the burden of selection to the user. Another option may be to use something like p2exe and create self-contained tools.

Python eggs may be another option which might be an easier way forward.

This looks like a correct approach to me.

Ping? I think this version is a good compromise that will work well both for binary distributions and compiling from source...

beanz added a comment.Aug 20 2019, 3:53 PM

Seems reasonable to me. @anemet any concerns?

anemet accepted this revision.Aug 20 2019, 4:53 PM

No, concerns. Looks good to me too.

This revision is now accepted and ready to land.Aug 20 2019, 4:53 PM

Thank you! I am not a committer, could one of you land this?

beanz added a comment.Aug 20 2019, 6:46 PM

I'm happy to commit this for you. Will do so shortly.

This revision was automatically updated to reflect the committed changes.
rnk added a subscriber: rnk.Aug 27 2019, 11:21 AM

How does this work at all? In llvm's lit.cfg.py I see this line:

opt_viewer_cmd = '%s %s/tools/opt-viewer/opt-viewer.py' % (sys.executable, config.llvm_src_root)

After this change, the referenced file does not exist.

I can probably update the path to refer into the object tree, but I'm wondering if there's a gap in testing somewhere.

If anyone is doing meaningful opt-viewer development, they might also want to consider separating most of the code out of the configured file so that they don't have to re-run cmake in order to test their changes.

rnk added a comment.Aug 27 2019, 11:31 AM

I ended up reverting this in rL370095 since fixing the tests didn't seem easy.