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

fix trying to generate RPATH wrappers for Clang #4088

Merged
merged 5 commits into from
Oct 18, 2022

Conversation

casparvl
Copy link
Contributor

@casparvl casparvl commented Oct 5, 2022

This loop is supposed to loop over all compilers for which wrappers need to be created. However, this line returns (['clang', 'clang++'],[None, None, None]) for the Clang toolchain. The subsequent call to which happening here then fails.

This PR fixes that by explicitely checking if cmd is None. If so, it proceeds to the next loop iteration and doens't try to create an RPATH wrapper for this 'None' command.

…he 'which' call in 'prepare_rpath_wrappers' fails when looping over those None objects. Thus, skip those and continue with the next loop iteration
@boegel
Copy link
Member

boegel commented Oct 12, 2022

@casparvl It would be nice if we could enhance the test_toolchain_prepare_rpath test to cover this fix.

Without this change, what happens? A hard crash, or RPATH linking just doesn't happen?

@casparvl
Copy link
Contributor Author

Yes, without this, a prepare_rpath_wrappers() call on a Clang toolchain will fail (some complaint about a Nonetype blabla since it expects a string)

@casparvl
Copy link
Contributor Author

casparvl commented Oct 17, 2022

@boegel is this more or less what you had in mind in terms of test? Can you review this PR, in conjuction with this EasyBlock PR easybuilders/easybuild-easyblocks#2799 ? From my perspective, they are ready for review.

@easybuilders easybuilders deleted a comment from boegelbot Oct 18, 2022
@boegel
Copy link
Member

boegel commented Oct 18, 2022

@casparvl That test is perfect: if I run it after temporarily undoing the fix in easybuild/tools/toolchain/toolchain.py, then it reproduces the problem being fixed:

ERROR: test_toolchain_prepare_rpath (test.framework.toolchain.ToolchainTest)
Test toolchain.prepare under --rpath
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Volumes/work/easybuild-framework/test/framework/toolchain.py", line 2627, in test_toolchain_prepare_rpath
    tc_clang.prepare_rpath_wrappers()
  File "/Volumes/work/easybuild-framework/easybuild/tools/toolchain/toolchain.py", line 994, in prepare_rpath_wrappers
    orig_cmd = which(cmd)
  File "/Volumes/work/easybuild-framework/easybuild/tools/filetools.py", line 521, in which
    cmd_path = os.path.join(path, cmd)
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/posixpath.py", line 90, in join
    genericpath._check_arg_types('join', a, *p)
  File "/Library/Developer/CommandLineTools/Library/Frameworks/Python3.framework/Versions/3.9/lib/python3.9/genericpath.py", line 152, in _check_arg_types
    raise TypeError(f'{funcname}() argument must be str, bytes, or '
TypeError: join() argument must be str, bytes, or os.PathLike object, not 'NoneType'

Copy link
Member

@boegel boegel left a comment

Choose a reason for hiding this comment

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

lgtm

@boegel boegel merged commit 5c81f74 into easybuilders:develop Oct 18, 2022
@boegel boegel changed the title Fix trying to generate RPATH wrappers for Clang fix trying to generate RPATH wrappers for Clang Oct 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants