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

Use utf-8 to open cmake files #334

Closed
wants to merge 2 commits into
base: master
from

Conversation

3 participants
@bgermann
Contributor

bgermann commented Jul 12, 2018

On Windows the default encoding for opening files is not utf-8.
This changes the scanning of .cmake files to open with utf-8.
If there is any character in a file that is not available in
CP-1252 the build fails.

Use utf-8 to open cmake files
On Windows the default encoding for opening files is not utf-8.
This changes the scanning of .cmake files to open with utf-8.
If there is any character in a file that is not available in
CP-1252 the build fails.
@codecov-io

This comment has been minimized.

codecov-io commented Jul 13, 2018

Codecov Report

Merging #334 into master will increase coverage by 3.45%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #334      +/-   ##
=========================================
+ Coverage   87.14%   90.6%   +3.45%     
=========================================
  Files          27      27              
  Lines        1159    1160       +1     
  Branches      209     209              
=========================================
+ Hits         1010    1051      +41     
+ Misses        107      71      -36     
+ Partials       42      38       -4
Impacted Files Coverage Δ
skbuild/cmaker.py 74.64% <100%> (+4.36%) ⬆️
skbuild/platform_specifics/abstract.py 97.56% <0%> (+1.21%) ⬆️
skbuild/setuptools_wrap.py 95.03% <0%> (+1.56%) ⬆️
skbuild/command/generate_source_manifest.py 90% <0%> (+6.66%) ⬆️
skbuild/command/bdist_wheel.py 100% <0%> (+7.69%) ⬆️
skbuild/platform_specifics/windows.py 93.5% <0%> (+9.09%) ⬆️
skbuild/compat.py 87.5% <0%> (+12.5%) ⬆️
skbuild/platform_specifics/osx.py 100% <0%> (+16.66%) ⬆️
skbuild/platform_specifics/linux.py 70.83% <0%> (+41.66%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed72d41...804facb. Read the comment docs.

@jcfr

This comment has been minimized.

Contributor

jcfr commented Jul 14, 2018

Thanks @bgermann for the PR and for fixing the python 2 support 👍

@jcfr

This comment has been minimized.

Contributor

jcfr commented Jul 14, 2018

  • To prevent future regression, I suggest you also add a test 😄
  • Adding an entry to the CHANGES.rst would also be stellar 🌠

Inspiring from this test, could you add a test named test_issue334_configure_cmakelists_non_cp1252_encoding (a similar name).

For example it could be similar to this (but you would still need to make sure the written CMakeLists.txt) is effectively failing without your patch:

def test_issue334_configure_cmakelists_non_cp1252_encoding():
    tmp_dir = _tmpdir('test_issue334_configure_cmakelists_non_cp1252_encoding')
    with push_dir(str(tmp_dir)):
        tmp_dir.join('CMakeLists.txt').write(textwrap.dedent(
            """
            cmake_minimum_required(VERSION 3.5.0)
            project(foobar NONE)
            # Todo: Add here some problematic characters
            install(CODE "execute_process(COMMAND \${CMAKE_COMMAND} -E sleep 0)")
            """
        ))
        cmkr = CMaker()
        cmkr.configure()

If writing the CMakeLists.txt from within the test is too cumbersome, adding a sample project in tests/samples is also possible (name for the project could be issue334-configure-cmakelists-non-cp1252-encoding). In that case, the test would look like this:

def test_issue334_configure_cmakelists_non_cp1252_encoding():
    tmp_dir = _tmpdir('test_issue334_configure_cmakelists_non_cp1252_encoding')
    prepare_project('issue334-configure-cmakelists-non-cp1252-encoding', tmp_dir)
    with push_dir(str(tmp_dir)):
        cmkr = CMaker()
        cmkr.configure()

Ps: I haven't tried the suggest sample ... they may fail at the first try.

Ps2: To run the test, you can invoke pytest test_cmaker.py::test_configure_cmakelists_non_cp1252_encoding::test_issue334_configure_cmakelists_non_cp1252_encoding

Let me know if you have any questions,
Jc

jcfr added a commit that referenced this pull request Jul 15, 2018

jcfr added a commit that referenced this pull request Jul 15, 2018

test: Configure a project with CMakeLists.txt having UTF-8 characters
To more quickly get feedback from the continuous integration system, this
commit also excludes all tests expect the one added by this commit:

* test_issue334_configure_cmakelists_non_cp1252_encoding

See #334

jcfr added a commit that referenced this pull request Jul 15, 2018

cmaker: Use utf-8 to open cmake files
On Windows the default encoding for opening files is not utf-8.
This changes the scanning of .cmake files to open with utf-8.
If there is any character in a file that is not available in
CP-1252 the build fails.

See #334

jcfr added a commit that referenced this pull request Jul 15, 2018

@jcfr

This comment has been minimized.

Contributor

jcfr commented Jul 15, 2018

Closing. Superseded by #338

@jcfr jcfr closed this Jul 15, 2018

jcfr added a commit that referenced this pull request Jul 15, 2018

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