This is an archive of the discontinued LLVM Phabricator instance.

[test] Fix clang-test for FreeBSD and NetBSD
ClosedPublic

Authored by lichray on Oct 21 2017, 8:35 PM.

Details

Summary

Lit tries to inject the shared library paths, but no action is taken when platform.system() is not recognized, results in an environment variable with an empty name, which is illegal.

The patch fixes this mechanism for FreeBSD and NetBSD, and gives an warning on other platforms, so that the latecomers don't have to spend time on debugging lit.

Diff Detail

Repository
rL LLVM

Event Timeline

lichray created this revision.Oct 21 2017, 8:35 PM
krytarowski added inline comments.Oct 21 2017, 8:46 PM
test/Unit/lit.cfg.py
39 ↗(On Diff #119771)

Please include 'NetBSD' next to 'FreeBSD'.

lichray updated this revision to Diff 119772.Oct 21 2017, 8:51 PM
lichray retitled this revision from [test] Fix clang-test for FreeBSD to [test] Fix clang-test for FreeBSD and NetBSD.
lichray edited the summary of this revision. (Show Details)
zturner requested changes to this revision.Oct 22 2017, 1:51 PM

Please don't throw an exception here. Instead, write this as:

shlibpath_var = None
if platform.system() in ['Linux', 'FreeBSD', 'NetBSD']:
    shilbpath = 'LD_LIBRARY_PATH'
elif platform.system() == 'Darwin':
    shlibpath_var = 'DYLD_LIBRARY_PATH'
elif platform.system() == 'Windows':
    shlibpath_var = 'PATH'

if shlibpath_var:
    shlibpath = os.path.pathsep.join((config.shlibdir, config.llvm_libs_dir, config.environment.get(shlibpath_var,'')))
    config.environment[shlibpath_var] = shlibpath
else:
    lit_config.warning('Unable to determine shared library path variable for platform {}'.format(platform.system()))
This revision now requires changes to proceed.Oct 22 2017, 1:51 PM
lichray marked an inline comment as done.Oct 22 2017, 1:55 PM

Please don't throw an exception here. Instead, write this as:

lit_config.warning('Unable to determine shared library path variable for platform {}'.format(platform.system()))

Why a warning rather than fatal?

joerg added a subscriber: joerg.Oct 22 2017, 1:56 PM

I think we should special case Darwin and Windows and fall-back to LD_LIBRARY_PATH for the rest. Can't remember if there is a UNIX-like platform left where it doesn't work.

I think we should special case Darwin and Windows and fall-back to LD_LIBRARY_PATH for the rest. Can't remember if there is a UNIX-like platform left where it doesn't work.

If those developers come to us, I would say. The script is for LLVM developers only, and to developers, a missed guess wastes more time than pointing out where it doesn't work.

I think we should special case Darwin and Windows and fall-back to LD_LIBRARY_PATH for the rest. Can't remember if there is a UNIX-like platform left where it doesn't work.

SHLIB_PATH in HP/UX

lichray updated this revision to Diff 119800.Oct 22 2017, 2:15 PM
lichray edited edge metadata.
lichray edited the summary of this revision. (Show Details)
krytarowski added a comment.EditedOct 22 2017, 2:19 PM

I think we should special case Darwin and Windows and fall-back to LD_LIBRARY_PATH for the rest. Can't remember if there is a UNIX-like platform left where it doesn't work.

SHLIB_PATH in HP/UX

Hmm SHLIB_PATH has been marked as legacy and they probably switched to LD_LIBRARY_PATH.

AIX might still ship with LIBPATH.

I agree that adding defines for OS'es that are barely developed (especially closed-source) without LLVM developers might be accumulating unused code.

lichray updated this revision to Diff 119803.Oct 22 2017, 5:10 PM
lichray edited the summary of this revision. (Show Details)

Changed to an warning given @zturner 's comments and experiments.

zturner accepted this revision.Oct 23 2017, 6:17 PM
This revision is now accepted and ready to land.Oct 23 2017, 6:17 PM
This revision was automatically updated to reflect the committed changes.