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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes for big-endian platforms #60

Merged
merged 5 commits into from Mar 4, 2019

Conversation

Projects
3 participants
@mosra
Copy link
Owner

mosra commented Mar 1, 2019

Should cover everything reported in #58 (and https://pastebin.com/GkSgVGyd), except the System test, as I have no idea what preprocessor macros to use to detect a Power9 architecture. I'm also a bit unsure about the SHA-1 fix, but hopefully it's the correct place to make that change.

@pkubaj would be wonderful if you could run the tests again on this branch 馃檹 thanks in advance!

@mosra mosra added this to the 2019.0b milestone Mar 1, 2019

@mosra mosra added this to TODO in Platforms via automation Mar 1, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Mar 1, 2019

Codecov Report

Merging #60 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #60   +/-   ##
=======================================
  Coverage   98.02%   98.02%           
=======================================
  Files          79       79           
  Lines        5105     5105           
=======================================
  Hits         5004     5004           
  Misses        101      101
Impacted Files Coverage 螖
src/Corrade/Utility/Sha1.cpp 100% <100%> (酶) 猬嗭笍

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 12a0fd7...e20d61a. Read the comment docs.

@pkubaj

This comment has been minimized.

Copy link
Contributor

pkubaj commented Mar 1, 2019

I'm testing it right now (UtilityDirectoryTest has been running for the last 10 minutes, is that normal)?

One test that failed so far is:

15/67 Test #15: ContainersStridedArrayViewTest .........***Failed    0.12 sec
Starting Corrade::Containers::Test::StridedArrayViewTest with 22 test cases...
    OK [01] constructEmpty()
    OK [02] constructNullptr()
    OK [03] constructNullptrSize()
    OK [04] construct()
    OK [05] constructFixedSize()
    OK [06] constructDerived()
    OK [07] constructView()
    OK [08] constructStaticView()
    OK [09] convertBool()
    OK [10] convertConst()
    OK [11] convertFromExternalView()
    OK [12] convertConstFromExternalView()
    OK [13] emptyCheck()
    OK [14] access()
    OK [15] accessConst()
    OK [16] accessInvalid()
    OK [17] iterator()
    OK [18] rangeBasedFor()
    OK [19] sliceInvalid()
    OK [20] slice()
  FAIL [21] cast() at /usr/local/poudriere/ports/default/devel/corrade/work/corrade-2019.01/src/Corrade/Containers/Test/StridedArrayViewTest.cpp on line 589
        Values b[2] and (4 << 16) | 40 are not the same, actual is
        196638
        but expected
        262184
    OK [22] castInvalid()
Finished Corrade::Containers::Test::StridedArrayViewTest with 1 errors out of 173 checks.

You won't able to test for POWER9 (this is like testing specifically for Skylake or Ryzen, you need to ping kernel directly for that), but you can test for POWER:

root@talos:$~$ echo | g++8 -dM -E - | egrep -i 'ppc|powerpc'
#define _ARCH_PPCGR 1
#define __PPC64__ 1
#define _ARCH_PPCSQ 1
#define _ARCH_PPC 1
#define __ppc__ 1
#define __powerpc64__ 1
#define __PPC__ 1
#define __powerpc__ 1
#define _ARCH_PPC64 1
@pkubaj

This comment has been minimized.

Copy link
Contributor

pkubaj commented Mar 1, 2019

OK, it finished.

The only failing tests are:

15/67 Test #15: ContainersStridedArrayViewTest .........***Failed    0.12 sec
Starting Corrade::Containers::Test::StridedArrayViewTest with 22 test cases...
    OK [01] constructEmpty()
    OK [02] constructNullptr()
    OK [03] constructNullptrSize()
    OK [04] construct()
    OK [05] constructFixedSize()
    OK [06] constructDerived()
    OK [07] constructView()
    OK [08] constructStaticView()
    OK [09] convertBool()
    OK [10] convertConst()
    OK [11] convertFromExternalView()
    OK [12] convertConstFromExternalView()
    OK [13] emptyCheck()
    OK [14] access()
    OK [15] accessConst()
    OK [16] accessInvalid()
    OK [17] iterator()
    OK [18] rangeBasedFor()
    OK [19] sliceInvalid()
    OK [20] slice()
  FAIL [21] cast() at /usr/local/poudriere/ports/default/devel/corrade/work/corrade-2019.01/src/Corrade/Containers/Test/StridedArrayViewTest.cpp on line 589
        Values b[2] and (4 << 16) | 40 are not the same, actual is
        196638
        but expected
        262184
    OK [22] castInvalid()
Finished Corrade::Containers::Test::StridedArrayViewTest with 1 errors out of 173 checks.
55/67 Test #55: UtilitySystemTest ......................***Failed    0.10 sec
Starting Corrade::Utility::Test::SystemTest with 2 test cases...
  FAIL [1] target() at /usr/local/poudriere/ports/default/devel/corrade/work/corrade-2019.01/src/Corrade/Utility/Test/SystemTest.cpp on line 45
        Expression !"No suitable CORRADE_TARGET_* defined on this platform" failed.
    OK [2] sleep()
Finished Corrade::Utility::Test::SystemTest with 1 errors out of 2 checks.

The 1st is mentioned above, the 2nd you can fix by checking e.g. for __powerpc64__.

@mosra mosra force-pushed the big-endian branch from a52f42a to e20d61a Mar 1, 2019

@mosra

This comment has been minimized.

Copy link
Owner Author

mosra commented Mar 1, 2019

Thanks a lot! 馃憤 The StridedArrayViewTest was just a silly copypaste typo, the System test should be working now as well, I added a CORRADE_TARGET_POWERPC macro that checks for __powerpc64__ (or _M_PPC for MSVC, but I have no idea if that's still a thing).

UtilityDirectoryTest has been running for the last 10 minutes, is that normal?

Yeah sorry there's a little benchmark comparing various file copy approaches. It runs in a second on a SSD and in around five seconds on a HDD. Didn't expect it to run on such a device, so that's why 馃槄

I force-pushed an updated version, if you could do one last check with just the two formerly failing tests, like this, would be great. Thanks for all your help! 馃憤

ctest --output-on-failure -R (SystemTest|StridedArrayViewTest)
@pkubaj

This comment has been minimized.

Copy link
Contributor

pkubaj commented Mar 4, 2019

All tests have passed.
Thanks!

@mosra mosra merged commit e20d61a into master Mar 4, 2019

7 checks passed

LGTM analysis: C/C++ No new or fixed alerts
Details
codecov/patch 100% of diff hit (target 98.02%)
Details
codecov/project 98.02% (+0%) compared to 12a0fd7
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

Platforms automation moved this from TODO to Done Mar 4, 2019

@mosra mosra deleted the big-endian branch Mar 4, 2019

@mosra

This comment has been minimized.

Copy link
Owner Author

mosra commented Mar 4, 2019

Likewise, thanks for the confirmation! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.