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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CI 2.0 - unified CI for C++, Go and RUST - PHASE1 #680

Merged
merged 65 commits into from
Dec 23, 2024
Merged

CI 2.0 - unified CI for C++, Go and RUST - PHASE1 #680

merged 65 commits into from
Dec 23, 2024

Conversation

aviadingo
Copy link
Contributor

@aviadingo aviadingo commented Dec 8, 2024

Describe the changes

New CI combines tests for C++/GO/RUST and Examples on a single action.
Tests are divided into curves, fields and hash. each of these can run simultaniously.

Uses a precompiled version of the backend if available on the machine based on the ref to the backend, otherwise recompiles it.

@aviadingo aviadingo self-assigned this Dec 8, 2024
@aviadingo aviadingo changed the title added cpp-golang test for CI unified CI for C++ and Go Dec 8, 2024
@aviadingo aviadingo changed the title unified CI for C++ and Go CI 2.0 - unified CI for C++, Go and RUST - PHASE1 Dec 22, 2024
Copy link
Contributor

@mickeyasa mickeyasa left a comment

Choose a reason for hiding this comment

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

great

.define("FIELD", "bw6_761")
.define("HASH", "OFF")
.define("CMAKE_INSTALL_PREFIX", &icicle_install_dir);
// if cfg!(feature = "bw6-761") {
Copy link
Contributor

Choose a reason for hiding this comment

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

why is that commented out?
We still support 761

Copy link
Contributor Author

Choose a reason for hiding this comment

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

761 got a standalone build.rs and no longer relies on bls12-377. I will remove the comments

@@ -1,23 +1,91 @@
use std::env;
use std::path::PathBuf;
// use std::env;
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove commented code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

println!("cargo:rustc-link-lib=icicle_hash");
println!("cargo:rustc-link-arg=-Wl,-rpath,{}/lib", icicle_install_dir.display()); // Add RPATH linker arguments

// default backends dir
Copy link
Contributor

Choose a reason for hiding this comment

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

remove line

fn main() {
// Retrieve environment variables
let out_dir = env::var("OUT_DIR").expect("OUT_DIR is not set");
// fn main() {
Copy link
Contributor

Choose a reason for hiding this comment

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

what about the rest of the curves/fields?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

761 was the only curve using a build that relies on another curve bls12-377 (probably due to legacy code).

so only 761 needs that update. new build script was tested and is running on the new CI

Copy link
Contributor

@LeonHibnik LeonHibnik left a comment

Choose a reason for hiding this comment

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

lgtm, left one commet

- name: bw6_761
build_args: -DG2=ON -DECNTT=ON
- name: grumpkin
build_args:
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: all features are ON by default so maybe it's better to specify explicitly such -DG2=OFF -DECNTT=OFF

@LeonHibnik LeonHibnik merged commit 66b9c70 into main Dec 23, 2024
13 checks passed
@LeonHibnik LeonHibnik deleted the aviad/CI branch December 23, 2024 21:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants