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

Add CI Support #1

Merged
merged 48 commits into from
Dec 6, 2024
Merged

Add CI Support #1

merged 48 commits into from
Dec 6, 2024

Conversation

ozcodes
Copy link
Collaborator

@ozcodes ozcodes commented Nov 17, 2024

Building using CI for linux x86 and macOS arm uploading the build to github actions and to the release page.
The binaries are signed for macOS except for rustc binary which currently cause a failure with cargo build.

Main changes:

  • Created a CI for building on linux and macos.
  • Created a new bash script for signing macos binaries.
  • Changed package.sh script to include signing the binaries just before creating the compressed file.
  • Changed justfile to include update cargo in the build command.

@ozcodes ozcodes requested review from caballa and 1arie1 November 17, 2024 17:35
@ozcodes ozcodes requested a review from pickx November 17, 2024 18:41
Copy link
Collaborator

@caballa caballa left a comment

Choose a reason for hiding this comment

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

We do not want to include any binary or .tar.bz2 file in the repo.
Instead, we put them in Releases

@ozcodes
Copy link
Collaborator Author

ozcodes commented Nov 18, 2024

We do not want to include any binary or .tar.bz2 file in the repo. Instead, we put them in Releases

The ci doesn't upload the the files to the repository, its uploaded to the github actions, you can see here an example:
https://github.com/Certora/certora-solana-platform-tools/actions/runs/11880630885
should it be removed from there ?

From the comment i uderstand that we need to upload the files when we create a new release\tag, i'll add support for it in the ci.

update:
@caballa i didn't notice that i added a .tar.bz2 file by mistake to the repo, i removed it, thanks for noticing.

@ozcodes ozcodes requested a review from caballa December 3, 2024 13:19
justfile Outdated

[linux]
build-cargo:
cd {{ out_dir }}/cargo && env OPENSSL_STATIC=1 OPENSSL_LIB_DIR=/usr/lib/x86_64-linux-gnu OPENSSL_INCLUDE_DIR=/usr/include/openssl cargo build --release
cd {{ out_dir }}/cargo && cargo update && env OPENSSL_STATIC=1 OPENSSL_LIB_DIR=/usr/lib/x86_64-linux-gnu OPENSSL_INCLUDE_DIR=/usr/include/openssl cargo build --release
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is cargo update needed? This will update Cargo.lock, leading to inconsistent builds. It is strange if it was required for the CI but not for me locally.

Copy link
Collaborator Author

@ozcodes ozcodes Dec 3, 2024

Choose a reason for hiding this comment

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

yes, it was required by the ci.
this is the error from the ci build:

Compiling time v0.3.29
error[E0282]: type annotations needed for `Box<_>`
  --> /Users/runner/.cargo/registry/src/index.crates.io-6f17d22bba15001f/time-0.3.29/src/format_description/parse/mod.rs:83:9
   |
83 |     let items = format_items
   |         ^^^^^
...
86 |     Ok(items.into())
   |              ---- type must be known at this point
   |
   = note: this is an inference error on crate `time` caused by an API change in Rust 1.80.0; update `time` to version `>=0.3.35` by calling `cargo update`

maybe we can replace cargo update with just updating time crate version ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. I remember getting this error. The issue is that we are compiling old cargo using new cargo and there some library no longer works. The proper fix is to update time package to newer version and apply patch to Cargo.lock. I have to do it locally, check that it works, and then commit the patch to the repo.

Current workaround of calling cargo update each time is ok for now

@@ -82,6 +82,26 @@ if [[ "${HOST_TRIPLE}" != "x86_64-pc-windows-msvc" ]] ; then
#cp -R rust/build/${HOST_TRIPLE}/llvm/lib/python* deploy/llvm/lib/
fi

# Sign macOS binaries
if [[ $HOST_TRIPLE == *apple-darwin* ]]; then
Copy link
Collaborator

Choose a reason for hiding this comment

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

this has to be outside of package.sh and has to be optional.

In the current state, I will not be able to build this locally since signing will fail.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

your right it will failed locally.
i tried to keep it outside package.sh at first but eventually added it there because we need to sign the binaries right after we copy all of binaries to a single folder (deploy dir) and before we zip them.

how about adding a flag so the signing process will not run by default but only when ci is running ?
and also make the root directory of the binaries as a variable ?
this way we will have control over it in local builds.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you already use some local variables as part of singing process to communicate the key. You can just check whether they are set and not sign if they are not.

"$RUST_LIB/librustc_driver-b4e91886a4c059a0.dylib" \
"$RUST_LIB/libstd-6eff127b55c063c2.dylib" \
"$RUST_LIB_BIN/rust-lld"
# "$RUST_BIN/rustc" # Not signing 'rustc' duo to failing cargo build
Copy link
Collaborator

Choose a reason for hiding this comment

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

perhaps it is safer to not sign anything until we understand what is the cause of the problem.
Perhaps signing other binaries creates a problem we just don't see yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it might be the case, we are signing the gambit binary for some time now without any issues, on the other hand i'm not sure its the right comparison.
we can comment it out for now until we feel more comfortable with it of course.

Copy link
Contributor

Choose a reason for hiding this comment

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

I had people use the unsigned binaries from CI and I know that they work well. I am worried that if signed binaries have some problem that only appears under some conditions, it will be hard to debug since the developers do not use the same binaries and thus will not be able to reproduce the problem.

Since until we are able to sign all binaries, we still will give instructions on how to manually quarantine using xattr, it is safer to not sign.

I will try to setup local signing key and see if I can reproduce the problem using local build and also look if there are instructions on how to sign the rustc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok, i will disable signing for now.
can you share more details on the instructions ? do you mean that we are removing the quarantine attribute from the binaries in order to bypass macos gatekeeper ?

Copy link
Contributor

Choose a reason for hiding this comment

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

specifically, we ask users to do this

sudo xattr -rd com.apple.quarantine $HOME/platform-tools-certora

@1arie1 1arie1 merged commit 27096ce into main Dec 6, 2024
4 checks passed
@ozcodes ozcodes deleted the oz/ci branch December 16, 2024 07:43
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.

4 participants