Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Portable BLST #25

Merged
merged 3 commits into from
Sep 5, 2023
Merged

Portable BLST #25

merged 3 commits into from
Sep 5, 2023

Conversation

cam-schultz
Copy link
Collaborator

Why this should be merged

Fixes cross-platform application and unit test execution by adding portable BLST flags. Otherwise, execution fails with SIGILL

How this works

  • Adds portable BLST flags to constants.sh
  • Adds test entrypoint script test.sh that sources constants.sh

How this was tested

CI, Teleporter integration tests

scripts/test.sh Outdated
set -o pipefail

# Directory above this script
RELAYER_PATH=$( cd "$( dirname "${BASH_SOURCE[0]}" )"; cd .. && pwd )

Choose a reason for hiding this comment

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

nit: can you make this into two lines like above

Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets also do the same in constants.sh

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

#
# We use "export" here instead of just setting a bash variable because we need
# to pass this flag to all child processes spawned by the shell.
export CGO_CFLAGS="-O -D__BLST_PORTABLE__"

Choose a reason for hiding this comment

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

do we need this for teleporter as well?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The teleporter repo itself doesn't have any Go applications that use BLST. Those should just be avalanchego, subnet-evm, and the relayer.

Copy link

@minghinmatthewlam minghinmatthewlam left a comment

Choose a reason for hiding this comment

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

LGTM, just some questions

gwen917
gwen917 previously approved these changes Sep 5, 2023
Copy link
Contributor

@gwen917 gwen917 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@michaelkaplan13 michaelkaplan13 left a comment

Choose a reason for hiding this comment

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

LGTM, just the one nit from @minghinmatthewlam would be good to keep uniform.

Copy link
Contributor

@geoff-vball geoff-vball left a comment

Choose a reason for hiding this comment

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

👍

@cam-schultz cam-schultz merged commit 7168076 into main Sep 5, 2023
@cam-schultz cam-schultz deleted the portable-blst branch September 5, 2023 15:25
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants