[crypto] Fix relic build script for ARM #823

huitseeker · 2021-06-11T17:47:34Z

Detect if we're compiling on an ARM and drop the unsupported -march=native there.

Fixes #809

codecov-commenter · 2021-06-11T23:32:52Z

Codecov Report

Merging #823 (6f058dc) into master (26ed451) will increase coverage by 0.27%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #823      +/-   ##
==========================================
+ Coverage   56.44%   56.71%   +0.27%     
==========================================
  Files         427      429       +2     
  Lines       25088    25473     +385     
==========================================
+ Hits        14160    14447     +287     
- Misses       9009     9060      +51     
- Partials     1919     1966      +47

Flag	Coverage Δ
unittests	`56.71% <ø> (+0.27%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
fvm/handler/metrics.go	`65.21% <0.00%> (-6.22%)`	⬇️
model/flow/identifier.go	`75.00% <0.00%> (-5.00%)`	⬇️
engine/execution/state/delta/view.go	`73.07% <0.00%> (-3.76%)`	⬇️
engine/execution/state/delta/delta.go	`66.66% <0.00%> (-3.61%)`	⬇️
model/flow/address.go	`80.51% <0.00%> (-2.15%)`	⬇️
network/p2p/ping.go	`52.27% <0.00%> (-2.08%)`	⬇️
model/flow/role.go	`89.36% <0.00%> (-1.95%)`	⬇️
engine/consensus/sealing/core.go	`60.98% <0.00%> (-0.76%)`	⬇️
model/flow/ledger.go	`12.50% <0.00%> (-0.55%)`	⬇️
ledger/complete/mtrie/trie/trie.go	`48.73% <0.00%> (-0.50%)`	⬇️
... and 32 more

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 26ed451...6f058dc. Read the comment docs.

tarakby

Thanks for looking into this and updating the script 👌
I left a question below.

tarakby · 2021-06-14T20:26:38Z

crypto/relic_build.sh

@@ -17,12 +17,21 @@ pushd "$DIR/relic/build"
 #
 GENERAL=(-DTIMER=CYCLE -DCHECK=OFF -DVERBS=OFF)
 LIBS=(-DSHLIB=OFF -DSTLIB=ON)
-COMP=(-DCOMP="-O3 -funroll-loops -fomit-frame-pointer -march=native -mtune=native")
+
+# "-march=native" is not supported on ARM


Did compilation on ARM fail with gcc or clang ?

It seems that gcc supports the flag (a node operator is also using gcc on ARM), but I couldn't find the same information for clang.

Ah, yes, the flag is indeed recently supported on gcc. The OP in #809 almost certainly had issues with clang (because M1).

I've added 53ea1bc, which I hope addresses the issue more comprehensively, if at the cost of some complexity.

tarakby

Thank you for taking the initiative to work on this script 🏄
I left a few comment/questions below.

crypto/relic_build.sh

tarakby · 2021-06-23T03:39:00Z

crypto/relic_build.sh

+# de-mangle the CMakeLists file, done with a temp file
+# to be portable between GNU / BSD sed
+CMAKE_TEMP=$(mktemp)
+sed -e '/message ( STATUS "CC=$ENV{CC}" )/d' ../CMakeLists.txt > $CMAKE_TEMP


I think sed -i allows making changes in place to avoid using CMAKE_TEMP

Suggested change

sed -e '/message ( STATUS "CC=$ENV{CC}" )/d' ../CMakeLists.txt > $CMAKE_TEMP

sed -i -e '/message ( STATUS "CC=$ENV{CC}" )/d' ../CMakeLists.txt

Actually, I would avoid using sed as it requires a different command on GNU and macos.
Maybe we could just avoid deleting the extra line? or use grep and only write the extra line if it doesn't exist already?

As the comment above the line you comment on mentions, the -i option is not supported on BSD sed, and was therefore not used. See this comment for the sed variants I tested, though that may have been insufficient.

Maybe we could just avoid deleting the extra line? or use grep and only write the extra line if it doesn't exist already?

It's important to delete the added line, because the CMakeLists file is part of the relic build, which is part of a submodule. The change to the submodule made here could end up in a (flow) commit very easily.

If we add the line only if it's not here already, the tail logic we use to grab the CC=... line may fail (because a CC print may occur only earlier in the output), which may silently cause a divergence between cmake's notion of CC and that of the present script.

I agree deleting the line would be cleaner, I just wanted to avoid using sed.
The sed command to delete the line works fine on my machine 👌

tarakby · 2021-06-23T03:43:20Z

crypto/relic_build.sh

+mv $CMAKE_TEMP ../CMakeLists.txt
+
+# extract what cmake uses as CC and probe its version string
+CC_VAL="$(tail -n 5 "$CMAKE_OUTPUT" | grep -oE 'CC=.*$' | sed -re 's/CC=(.*)/\1/g')"


Another use of sed that might cause issues (it actually crashes on my macos).
We could just skip the 3 first characters instead of using sed.

That's strange, I've tested this both on GNU sed v4.8, and OSX sed at on Big Sur (OS version 11.4).
What does sw_vers -productversionsay on your crashing system?

I think sed -r was the problem

tarakby · 2021-06-23T03:44:21Z

crypto/relic_build.sh

+# extract what cmake uses as CC and probe its version string
+CC_VAL="$(tail -n 5 "$CMAKE_OUTPUT" | grep -oE 'CC=.*$' | sed -re 's/CC=(.*)/\1/g')"
+# default to which
+CC_VAL=${CC_VAL:-"$(which cc)"}


This might be a naive question : why don't we always rely on which cc ?

Because that may not be the compiler used by cmake - it's a relatively primitive fallback. In particular:

the cmake compiler is configurable, e.g. you can pass CMAKE_C_COMPILER, CC, -DCMAKE_C_COMPILER ...

CMake runs though its own detection and then testing of the compiler
I didn't want to reproduce all that.

Ah I understand, that makes sense

tarakby · 2021-06-23T06:31:50Z

I have implemented some of the suggestions, and also written the script in a way that looks easier to understand (at least to me) on this branch: https://github.com/onflow/flow-go/blob/tarak/arm-clang-relic-build/crypto/relic_build.sh.
Please have a look and feel free to port changes to your branch (we will still merge your branch eventually), there might be issues with ARM as I only tested on my laptop.

huitseeker · 2021-06-23T14:08:51Z

@tarakby I've integrated most of your changes, with a couple of tweaks:

the -mtune=native option is supported on ARM and I've therefore made it part of default options,
I've de-mangled the CMakeLists file to make sure we don't touch the relic submodule. That implies one single usage of sed that's portable not only on OSX & GNU sed, but on a vast range of BSD sed versions (being completely optionless).

tarakby

🏄

crypto/relic_build.sh

ramtinms

Looks good to me, just some questions.

Detect if we're compiling on an ARM and drop the unsupported `-march=native` there. Fixes onflow#809.

turbolent

Nice!

ShellCheck also 👍

huitseeker requested a review from tarakby as a code owner June 11, 2021 17:47

huitseeker force-pushed the huitseeker/809 branch from e35eb23 to 1566b16 Compare June 11, 2021 17:48

huitseeker requested review from Kay-Zee and vishalchangrani June 14, 2021 19:52

tarakby reviewed Jun 14, 2021

View reviewed changes

huitseeker force-pushed the huitseeker/809 branch 3 times, most recently from 868f1b0 to 53ea1bc Compare June 15, 2021 18:44

tarakby reviewed Jun 23, 2021

View reviewed changes

huitseeker force-pushed the huitseeker/809 branch 2 times, most recently from d68fe49 to 6f058dc Compare June 23, 2021 14:12

tarakby self-assigned this Jun 23, 2021

tarakby approved these changes Jun 24, 2021

View reviewed changes

crypto/relic_build.sh Outdated Show resolved Hide resolved

huitseeker force-pushed the huitseeker/809 branch from 6f058dc to 44c8c48 Compare June 25, 2021 13:55

ramtinms reviewed Jun 28, 2021

View reviewed changes

crypto/relic_build.sh Show resolved Hide resolved

ramtinms reviewed Jun 28, 2021

View reviewed changes

crypto/relic_build.sh Show resolved Hide resolved

ramtinms approved these changes Jun 28, 2021

View reviewed changes

huitseeker added 3 commits June 28, 2021 13:17

[crypto] Fix relic build script for ARM

04b7312

Detect if we're compiling on an ARM and drop the unsupported `-march=native` there. Fixes onflow#809.

Make sure we detect if we're set to compile with clang before we remove

c2c4b02

Apply review comments

606bcd5

huitseeker force-pushed the huitseeker/809 branch from 44c8c48 to 606bcd5 Compare June 28, 2021 17:17

turbolent approved these changes Jun 28, 2021

View reviewed changes

huitseeker merged commit 71fceaf into onflow:master Jun 28, 2021

tarakby mentioned this pull request Jul 15, 2021

Compile relic for generic x86-64 processor #756

Closed

tatujan mentioned this pull request Jan 4, 2022

Building on m1 mac #809

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[crypto] Fix relic build script for ARM #823

[crypto] Fix relic build script for ARM #823

huitseeker commented Jun 11, 2021 •

edited

codecov-commenter commented Jun 11, 2021 •

edited

tarakby left a comment

tarakby Jun 14, 2021

huitseeker Jun 14, 2021 •

edited

tarakby left a comment

tarakby Jun 23, 2021

tarakby Jun 23, 2021

huitseeker Jun 23, 2021 •

edited

huitseeker Jun 23, 2021 •

edited

tarakby Jun 24, 2021

tarakby Jun 23, 2021

huitseeker Jun 23, 2021

tarakby Jun 24, 2021

tarakby Jun 24, 2021

tarakby Jun 23, 2021

huitseeker Jun 23, 2021 •

edited

tarakby Jun 24, 2021

tarakby commented Jun 23, 2021 •

edited

huitseeker commented Jun 23, 2021 •

edited

tarakby left a comment

ramtinms left a comment

turbolent left a comment

	sed -e '/message ( STATUS "CC=$ENV{CC}" )/d' ../CMakeLists.txt > $CMAKE_TEMP
	sed -i -e '/message ( STATUS "CC=$ENV{CC}" )/d' ../CMakeLists.txt

[crypto] Fix relic build script for ARM #823

[crypto] Fix relic build script for ARM #823

Conversation

huitseeker commented Jun 11, 2021 • edited

codecov-commenter commented Jun 11, 2021 • edited

Codecov Report

tarakby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Jun 14, 2021 • edited

Choose a reason for hiding this comment

tarakby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Jun 23, 2021 • edited

Choose a reason for hiding this comment

huitseeker Jun 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huitseeker Jun 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tarakby commented Jun 23, 2021 • edited

huitseeker commented Jun 23, 2021 • edited

tarakby left a comment

Choose a reason for hiding this comment

ramtinms left a comment

Choose a reason for hiding this comment

turbolent left a comment

Choose a reason for hiding this comment

huitseeker commented Jun 11, 2021 •

edited

codecov-commenter commented Jun 11, 2021 •

edited

huitseeker Jun 14, 2021 •

edited

huitseeker Jun 23, 2021 •

edited

huitseeker Jun 23, 2021 •

edited

huitseeker Jun 23, 2021 •

edited

tarakby commented Jun 23, 2021 •

edited

huitseeker commented Jun 23, 2021 •

edited