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

[OSX] skbuild.setup defaults cmake_args without checking kwargs for the arguments. #342

Closed
yonip opened this Issue Jul 19, 2018 · 3 comments

Comments

2 participants
@yonip

yonip commented Jul 19, 2018

OS: OSX/Darwin (testing on travis).

setup checks the command line arguments for -DCMAKE_OSX_DEPLOYMENT_TARGET and for -DCMAKE_OSX_ARCHITECTURES, and if they don't exist, it defaults them here. The issue with this is that if the user passed either of those arguments to setup via a kwarg. i.e. setup(..., cmake_args=[..., "-DCMAKE_OSX_DEPLOYMENT_TARGET:STRING=10.7"]), it gets overwritten with the default, even when nothing had been passed to the command line.
This causes builds to break for no apparent reason (see the reasons for this PR). A simple fix is moving the lines where the command line arguments are added to the arguments passed via kwarg from here to right above the block mentioned above.
Since cmake_args is not referenced between line 477 and line 503, nothing will get more args than it expected, and CLI will still override the args passed via kwarg, but default will only be set if neither the CLI or kwarg cmake_arg are set.
If this is approved I will PR the fix described above.
(This is an issue first because the contribution guidelines specify filing bugs before trying to fix them).

yonip pushed a commit to yonip/scikit-build that referenced this issue Jul 19, 2018

yonip pushed a commit to yonip/scikit-build that referenced this issue Jul 19, 2018

yonip pushed a commit to yonip/scikit-build that referenced this issue Jul 19, 2018

@jcfr

This comment has been minimized.

Contributor

jcfr commented Jul 22, 2018

Thanks for reporting the issue 👍 , and also providing a detailed explanation 😄

Your proposed fix makes complete sense. Would be great if you could create a PR 🎆

To prevent future regression, I suggest you also add a test.

The test could be named tests/test_issue342_cmake_osx_args_in_setup.py. Looking at existing tests in the corresponding directory should be helpful.

Since we only want to check if cmake is invoked with the expected parameter, you should mock both the upstream setup call and the call to CMaker.configure.

Here are some pointers:

  • mocker.patch('skbuild.setuptools_wrap.upstream_setup')

  • execute_setup_py should be called with disable_languages_test=True. For example, see here

  • I think there is no need to create a sample project, both setup.py and CMakeLists.txt as it is done in this test.

  • mock for configure could probably be done using mocker.patch('skbuild.cmaker.CMaker.configure') then you would check if the function has been called and if the clargs keyword value is expected.

@yonip

This comment has been minimized.

yonip commented Jul 25, 2018

I have found another bug, in Cmaker.has_cmake_cache_arg, which would cause the tests to fail: since it traverses the list of parameters in order, has_cmake_cache_arg(['-DOVERRIDE:STRING=A', '-DOVERRIDE:STRING=B'], 'OVERRIDE', 'B') returns False, while the way command line arguments are parsed, OVERRIDE should be set to 'B'.
The simple solution here is replacing the iteration line with for arg in reversed(cmake_args):, which will check that the last occurrence of arg_name is set to arg_value.
If you agree, I can put the fix and additional test in this PR. Should the new test go in tests/test_cmaker.py or in a separate file? Should I copy this to another issue and resolve it in a separate set of commits?

@jcfr

This comment has been minimized.

Contributor

jcfr commented Jul 25, 2018

Thanks for following up with this 👍 Very much appreciated 👏

I have found another bug, in CMaker.has_cmake_cache_arg,

Good catch

if you agree, I can put the fix and additional test in this PR.
Should the new test go in tests/test_cmaker.py or in a separate file? Should I copy this to another issue and resolve it in a separate set of commits?

For this one, adding a new entry to the existing test is sensible.

def test_has_cmake_cache_arg():
cmake_args = ['-DFOO:STRING=42', '-DBAR', '-DCLIMBING:BOOL=ON']
assert has_cmake_cache_arg(cmake_args, "FOO", "42")
assert not has_cmake_cache_arg(cmake_args, "foo", "42")
assert not has_cmake_cache_arg(cmake_args, "FOO", "43")
assert not has_cmake_cache_arg(cmake_args, "BAR")
assert not has_cmake_cache_arg(cmake_args, "BA")
assert not has_cmake_cache_arg(cmake_args, "BAR", None)
assert not has_cmake_cache_arg(cmake_args, "BAR", "42")
assert has_cmake_cache_arg(cmake_args, "CLIMBING")
assert has_cmake_cache_arg(cmake_args, "CLIMBING", None)
assert has_cmake_cache_arg(cmake_args, "CLIMBING", "ON")

It would be great to submit this change in a dedicated Pull Request referencing this issue.

Consider also updating CHANGES.rs

yonip pushed a commit to yonip/scikit-build that referenced this issue Jul 25, 2018

yonip pushed a commit to yonip/scikit-build that referenced this issue Jul 26, 2018

jcfr added a commit to yonip/scikit-build that referenced this issue Jul 27, 2018

jcfr added a commit to yonip/scikit-build that referenced this issue Jul 27, 2018

jcfr added a commit to jcfr/scikit-build that referenced this issue Jul 29, 2018

test_issue342_cmake_osx_args_in_setup: Regression test for scikit-bui…
…ld#342

Co-authored-by: Jean-Christophe Fillion-Robin <JChris.FillionR@kitware.com>

jcfr added a commit to jcfr/scikit-build that referenced this issue Jul 29, 2018

setuptoos_wrap: Consider CMAKE_OSX_* set using cmake_args keyword
This commit fixes scikit-build#342 and ensures the default values for
CMAKE_OSX_* variables are not always overriding the values
passed either from the command line or as setup keyword arguments.

Co-authored-by: Jean-Christophe Fillion-Robin <JChris.FillionR@kitware.com>

jcfr added a commit to jcfr/scikit-build that referenced this issue Jul 29, 2018

test_issue342_cmake_osx_args_in_setup: Regression test for scikit-bui…
…ld#342

Co-authored-by: Jean-Christophe Fillion-Robin <JChris.FillionR@kitware.com>

jcfr added a commit to jcfr/scikit-build that referenced this issue Jul 29, 2018

setuptoos_wrap: Consider CMAKE_OSX_* set using cmake_args keyword
This commit fixes scikit-build#342 and ensures the default values for
CMAKE_OSX_* variables are not always overriding the values
passed either from the command line or as setup keyword arguments.

Co-authored-by: Jean-Christophe Fillion-Robin <JChris.FillionR@kitware.com>

@jcfr jcfr closed this in #349 Jul 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment