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.

Sign up for GitHub

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

Jump to bottom

Streamline the processes of building libbls, testing it, and creating the blspy wheels #367

Merged
merged 1 commit into from
Jun 15, 2023
Merged

Streamline the processes of building libbls, testing it, and creating the blspy wheels #367

merged 1 commit into from
Jun 15, 2023

Conversation

AmineKhaldi
Copy link
Contributor

@AmineKhaldi AmineKhaldi commented May 2, 2023 •
edited
Loading

  • Speedup the process of creating wheels by skipping the compiling and linking of libbls' tests and benchmarks.
  • We no longer need Windows-specific CMake version checking in setup.py.
  • We no longer need Windows-specific architecture flag passing in setup.py.
  • We no longer treat Windows in a special way w.r.t. compiling and linking in setup.py.
  • We no longer need Windows-specific preparations before starting the process of creating wheels.
  • We no longer need Windows-specific repairs after wheels creation.
  • We no longer need the relic_ietf_64 repository and its related tasks.

Dedicated to @wjblanke

@AmineKhaldi
Streamline the processes of building libbls, testing it, and creating…
ec3192a 
… the blspy wheels.

* Speedup the process of creating wheels by skipping the compiling and linking of libbls' tests and benchmarks.
* We no longer need Windows-specific CMake version checking in setup.py.
* We no longer need Windows-specific architecture flag passing in setup.py.
* We no longer treat Windows in a special way w.r.t. compiling and linking in setup.py.
* We no longer need Windows-specific preparations before starting the process of creating wheels.
* We no longer need Windows-specific repairs after wheels creation.
* We no longer need the relic_ietf_64 repository and its related tasks.

Dedicated to @wjblanke
@AmineKhaldi AmineKhaldi self-assigned this May 2, 2023
hoffmang9
hoffmang9 approved these changes May 2, 2023
View reviewed changes
Copy link
Member

@hoffmang9 hoffmang9 left a comment

Choose a reason for hiding this comment

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

lgtm

@hoffmang9 hoffmang9 requested a review from wjblanke May 2, 2023 16:47
arvidn
arvidn reviewed May 11, 2023
View reviewed changes
Copy link
Contributor

@arvidn arvidn left a comment

Choose a reason for hiding this comment

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

looks good generally. just a few comments

CMakeLists.txt
@@ -47,7 +47,7 @@ if(DEFINED ENV{RELIC_MAIN})
set(RELIC_REPOSITORY "https://github.com/relic-toolkit/relic.git")
else()
# This is currently anchored to upstream aecdcae7956f542fbee2392c1f0feb0a8ac41dc5
set(RELIC_GIT_TAG "215c69966cb78b255995f0ee9c86bbbb41c3c42b")
set(RELIC_GIT_TAG "2a4ed8ded3a9a5ac5e39f3877f357264b2e16f11")
Copy link
Contributor

Choose a reason for hiding this comment

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

what changed here? We don't have any regression tests for the BLS functionality itself (afaik), so we should be careful when updating relic to ensure it's still running regression tests

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can check the changes here, and we run tests (just like upstream) in addition to our own bls-signatures tests so we should be reasonably careful (at least to the degree that our tests cover).

CMakeLists.txt
@@ -47,7 +47,7 @@ if(DEFINED ENV{RELIC_MAIN})
set(RELIC_REPOSITORY "https://github.com/relic-toolkit/relic.git")
else()
# This is currently anchored to upstream aecdcae7956f542fbee2392c1f0feb0a8ac41dc5
Copy link
Contributor

Choose a reason for hiding this comment

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

this comment looks out-dated now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're still anchored to upstream aecdcae.

CMakeLists.txt
@@ -141,6 +154,17 @@ set(PP_METHD "LAZYR;OATEP" CACHE STRING "")

FetchContent_MakeAvailable(relic)

function(handle_mpir_dlls _target)
file(GLOB chia_mpir_dlls ${chia_mpir_SOURCE_DIR}/*.dll)
Copy link
Contributor

Choose a reason for hiding this comment

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

GLOB makes me nervous. Do we not know what the filename is of this DLL?
The reason it makes me nervous is that it makes the build less deterministic, it depends on what files happen to be on your filesystem. perhaps you have some old files left-over from a previous build or a previous version

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a build dependency so in order to alter the state of that GLOB you would need to manually intervene inside the CMake build's _deps folder and add/remove dlls from our fetched MPIR dependency there. I don't mind making this list manual if that makes us feel better, with that said.

CMakeLists.txt
FetchContent_Declare(
chia_mpir
GIT_REPOSITORY https://github.com/Chia-Network/mpir_gc_x64.git
GIT_SHALLOW true
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want to pin this to a specific commit?
I would think it would make for fewer moving parts as we can update our MPIR build independently of taking that update in the blspy build

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're in control of this repo, we rarely update those DLLs and we would like to take the latest. Furthermore, having inconsistency in what dlls we fetch from this dependency, across repos, would result in issues.

@wallentx wallentx merged commit 96e2bd2 into Chia-Network:main Jun 15, 2023
wallentx added a commit that referenced this pull request Jun 20, 2023
@emlowe @altendky
@AmineKhaldi @ChiaAutomation @snyk-bot @lgtm-com @lgtm-migrator @wallentx @hoffmang9 @wjblanke
Merge main into long_lived and fixup conflicts (#397)
7b3c3e3 
* correct wheel matrix arch -> arm (#393)

* Add in some small test cases (#394)

* Add in some small test cases

* minor update

* test fixes

* small test addition

* Significantly speedup javascript bindings tasks by skipping libbls tests and benchmarks (#372)

* Streamline the processes of building libbls, testing it, and creating the blspy wheels (#367)

* Speedup the process of creating wheels by skipping the compiling and linking of libbls' tests and benchmarks.
* We no longer need Windows-specific CMake version checking in setup.py.
* We no longer need Windows-specific architecture flag passing in setup.py.
* We no longer treat Windows in a special way w.r.t. compiling and linking in setup.py.
* We no longer need Windows-specific preparations before starting the process of creating wheels.
* We no longer need Windows-specific repairs after wheels creation.
* We no longer need the relic_ietf_64 repository and its related tasks.

Dedicated to @wjblanke

* [Snyk] Upgrade karma-firefox-launcher from 1.3.0 to 2.1.2 (#382)

Snyk has created this PR to upgrade karma-firefox-launcher from 1.3.0 to 2.1.2.

See this package in npm:
https://www.npmjs.com/package/karma-firefox-launcher

See this project in Snyk:
https://app.snyk.io/org/chia-network/project/e887fc45-cae4-47fc-931a-813664d517cd?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <snyk-bot@snyk.io>

* Add CodeQL workflow for GitHub code scanning (#350)

Co-authored-by: LGTM Migrator <lgtm-migrator@users.noreply.github.com>

* [Snyk] Upgrade typescript from 3.9.7 to 5.0.4 (#376)

Snyk has created this PR to upgrade typescript from 3.9.7 to 5.0.4.

See this package in npm:
https://www.npmjs.com/package/typescript

See this project in Snyk:
https://app.snyk.io/org/chia-network/project/e887fc45-cae4-47fc-931a-813664d517cd?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <snyk-bot@snyk.io>

* [Snyk] Upgrade @types/node from 11.15.18 to 20.1.5 (#377)

Snyk has created this PR to upgrade @types/node from 11.15.18 to 20.1.5.

See this package in npm:
https://www.npmjs.com/package/@types/node

See this project in Snyk:
https://app.snyk.io/org/chia-network/project/e887fc45-cae4-47fc-931a-813664d517cd?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <snyk-bot@snyk.io>

* [Snyk] Upgrade mime from 1.4.1 to 3.0.0 (#378)

Snyk has created this PR to upgrade mime from 1.4.1 to 3.0.0.

See this package in npm:
https://www.npmjs.com/package/mime

See this project in Snyk:
https://app.snyk.io/org/chia-network/project/e887fc45-cae4-47fc-931a-813664d517cd?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <snyk-bot@snyk.io>

* [Snyk] Upgrade karma from 6.3.16 to 6.4.2 (#381)

Snyk has created this PR to upgrade karma from 6.3.16 to 6.4.2.

See this package in npm:
https://www.npmjs.com/package/karma

See this project in Snyk:
https://app.snyk.io/org/chia-network/project/e887fc45-cae4-47fc-931a-813664d517cd?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <snyk-bot@snyk.io>
Co-authored-by: William Allen <wallentx@users.noreply.github.com>

* [Snyk] Upgrade webpack from 5.76.0 to 5.82.1 (#380)

* fix: upgrade webpack from 5.76.0 to 5.82.1

Snyk has created this PR to upgrade webpack from 5.76.0 to 5.82.1.

See this package in npm:
https://www.npmjs.com/package/webpack

See this project in Snyk:
https://app.snyk.io/org/chia-network/project/e887fc45-cae4-47fc-931a-813664d517cd?utm_source=github&utm_medium=referral&page=upgrade-pr

* Update package.json

---------

Co-authored-by: snyk-bot <snyk-bot@snyk.io>
Co-authored-by: William Allen <wallentx@users.noreply.github.com>

* [Snyk] Upgrade @types/mocha from 5.2.7 to 10.0.1 (#379)

Snyk has created this PR to upgrade @types/mocha from 5.2.7 to 10.0.1.

See this package in npm:
https://www.npmjs.com/package/@types/mocha

See this project in Snyk:
https://app.snyk.io/org/chia-network/project/e887fc45-cae4-47fc-931a-813664d517cd?utm_source=github&utm_medium=referral&page=upgrade-pr

Co-authored-by: snyk-bot <snyk-bot@snyk.io>
Co-authored-by: William Allen <wallentx@users.noreply.github.com>

* Update build-wheels.yml to add Windows pre processing back (#398)

It looks like the windows steps are missing.

* Update setup.py

* integrate amine's cmake unification (#399)

* pin windows blst clone

* import setuptools

* setuptools

* revert

* back to no changes

* amine

* remove gmp

* add assembly

* windows cmake

* cmake syntax

* avoid gcc flags on win

* full name

* Keeping pre-release logic and changes from known working builds

---------

Co-authored-by: Kyle Altendorf <sda@fstab.net>
Co-authored-by: Amine Khaldi <amine.khaldi@reactos.org>
Co-authored-by: ChiaAutomation <85647627+ChiaAutomation@users.noreply.github.com>
Co-authored-by: snyk-bot <snyk-bot@snyk.io>
Co-authored-by: lgtm-com[bot] <43144390+lgtm-com[bot]@users.noreply.github.com>
Co-authored-by: LGTM Migrator <lgtm-migrator@users.noreply.github.com>
Co-authored-by: William Allen <wallentx@users.noreply.github.com>
Co-authored-by: Gene Hoffman <30377676+hoffmang9@users.noreply.github.com>
Co-authored-by: wjblanke <wjb98672@gmail.com>
Co-authored-by: wallentx <william.allentx@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hoffmang9 hoffmang9 hoffmang9 approved these changes

@arvidn arvidn arvidn approved these changes

@wjblanke wjblanke Awaiting requested review from wjblanke

Assignees

@AmineKhaldi AmineKhaldi

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AmineKhaldi @arvidn @hoffmang9 @wallentx