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

fix: builds on various archs/platforms re: PR 4331 #4335

Merged
merged 3 commits into from Feb 9, 2024
Merged

Conversation

wileyj
Copy link
Contributor

@wileyj wileyj commented Feb 2, 2024

ref: #4331

these are the equivalent changes for master and develop.

fix building & cross compilation, including stacks-signer, tested building for the following:

  • linux-glibc-x86-64
  • linux-glibc-arm64
  • linux-glibc-armv7
  • linux-musl-x86_64
  • linux-musl-arm64
  • linux-musl-armv7
  • windows-x86_64
  • macos-x86_64
  • macos-arm64

note: target branch here is for develop since there is a workaround in place already in master. this change won't affect that workaround the next time develop is merged to master

cc: @zone117x @hugocaillard

Copy link

codecov bot commented Feb 2, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (228e9fb) 82.35% compared to head (28f2cc3) 48.34%.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop    #4335       +/-   ##
============================================
- Coverage    82.35%   48.34%   -34.02%     
============================================
  Files          404      404               
  Lines       292642   292642               
============================================
- Hits        241018   141482    -99536     
- Misses       51624   151160    +99536     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

rand_core = "0.6"
reqwest = { version = "0.11.22", features = ["blocking", "json"] }
reqwest = { version = "0.11.22", default-features = false, features = ["blocking", "json", "rustls-tls"] }
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to simply build without TLS? If not, then this is fine.

@wileyj wileyj marked this pull request as draft February 2, 2024 17:07
@wileyj
Copy link
Contributor Author

wileyj commented Feb 2, 2024

following discussion in #4331 , converting this to draft.
there may be another option vs modifying the cargo.toml's here - eg using a native arm runner to build the binaries.

@wileyj
Copy link
Contributor Author

wileyj commented Feb 5, 2024

@xoloki can you confirm this is OK to do, or should be continue looking for build alternatives where we're not cross-compiling?

Copy link
Collaborator

@xoloki xoloki left a comment

Choose a reason for hiding this comment

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

The general approach of using default-features = false is correct.

I'm a little concerned that we are using different versions of wsts in the root and stacks-signer Cargo.toml files, but that was present before this PR so not a blocker.

@@ -11,6 +11,12 @@ members = [
"stacks-signer",
"testnet/stacks-node"]

# Dependencies we want to keep the same between workspace members
[workspace.dependencies]
wsts = { version = "7.0", default-features = false }
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the correct way to include wsts to avoid including bindgen and friends.

@@ -42,7 +42,7 @@ thiserror = "1.0"
toml = "0.5.6"
tracing = "0.1.37"
tracing-subscriber = { version = "0.3.17", features = ["env-filter"] }
wsts = "4.0.0"
wsts = { version = "4.0.0", default-features = false }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why we have such an old version of wsts here in the stacks-signer Cargo.toml? And why we have different versions here and in the root Cargo.toml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't say for certain, but best to leave as-is since this branch has been genesis-sync tested as working.

@wileyj
Copy link
Contributor Author

wileyj commented Feb 8, 2024

The general approach of using default-features = false is correct.

I'm a little concerned that we are using different versions of wsts in the root and stacks-signer Cargo.toml files, but that was present before this PR so not a blocker.

any concerns with the similar PR #4331 ?

@xoloki
Copy link
Collaborator

xoloki commented Feb 8, 2024

The general approach of using default-features = false is correct.
I'm a little concerned that we are using different versions of wsts in the root and stacks-signer Cargo.toml files, but that was present before this PR so not a blocker.

any concerns with the similar PR #4331 ?

#4331 is for next which uses a workspace dependency for wsts so no concerns there.

@saralab saralab marked this pull request as ready for review February 8, 2024 20:25
@wileyj wileyj merged commit 2c2afbc into develop Feb 9, 2024
1 of 2 checks passed
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.

None yet

5 participants