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: buids on various archs/platforms #4331

Merged
merged 1 commit into from
Feb 9, 2024
Merged

fix: buids on various archs/platforms #4331

merged 1 commit into from
Feb 9, 2024

Conversation

zone117x
Copy link
Member

@zone117x zone117x commented Feb 1, 2024

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

ty @wileyj and @hugocaillard

Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

Error when running cargo check

➜  stacks-core git:(fix/c-deps) cargo check
    Checking wsts v7.0.0
error: expected identifier, found `32`
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:307:39
    |
307 |             33 => Some(self.message[::32]),
    |                                       ^^ expected identifier

error: functions are not allowed in struct definitions
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:304:5
    |
288 |   pub struct SignatureShareRequest {
    |              --------------------- while parsing this struct
...
304 | /     fn hash_bytes(&self) -> Result<Hash, Error> {
305 | |         match self.message.len() {
306 | |             32 => Some(self.message),
307 | |             33 => Some(self.message[::32]),
308 | |             _ => None,
309 | |         }
310 | |     }
    | |_____^
    |
    = help: unlike in C++, Java, and C#, functions are declared in `impl` blocks
    = help: see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information

error: expected identifier
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:304:5
    |
304 |     fn hash_bytes(&self) -> Result<Hash, Error> {
    |     ^^

error[E0277]: the trait bound `SignatureShareRequest: Serialize` is not satisfied
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:55:17
    |
55  | #[derive(Clone, Serialize, Deserialize, Debug, PartialEq)]
    |                 ^^^^^^^^^ the trait `Serialize` is not implemented for `SignatureShareRequest`
...
75  |     SignatureShareRequest(SignatureShareRequest),
    |                           --------------------- required by a bound introduced by this call
    |
    = help: the following other types implement trait `Serialize`:
              bool
              char
              isize
              i8
              i16
              i32
              i64
              i128
            and 158 others
note: required by a bound in `serialize_newtype_variant`
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.156/src/ser/mod.rs:942:12
    |
934 |     fn serialize_newtype_variant<T: ?Sized>(
    |        ------------------------- required by a bound in this associated function
...
942 |         T: Serialize;
    |            ^^^^^^^^^ required by this bound in `Serializer::serialize_newtype_variant`

error[E0277]: the trait bound `SignatureShareRequest: Deserialize<'_>` is not satisfied
    --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:75:27
     |
75   |     SignatureShareRequest(SignatureShareRequest),
     |                           ^^^^^^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `SignatureShareRequest`
     |
     = help: the following other types implement trait `Deserialize<'de>`:
               bool
               char
               isize
               i8
               i16
               i32
               i64
               i128
             and 170 others
note: required by a bound in `newtype_variant`
    --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.156/src/de/mod.rs:2122:12
     |
2120 |     fn newtype_variant<T>(self) -> Result<T, Self::Error>
     |        --------------- required by a bound in this associated function
2121 |     where
2122 |         T: Deserialize<'de>,
     |            ^^^^^^^^^^^^^^^^ required by this bound in `VariantAccess::newtype_variant`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `wsts` (lib) due to 5 previous errors

Copy link

codecov bot commented Feb 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9690ecd) 83.35% compared to head (163bb08) 76.88%.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4331      +/-   ##
==========================================
- Coverage   83.35%   76.88%   -6.48%     
==========================================
  Files         446      446              
  Lines      318774   318774              
==========================================
- Hits       265706   245080   -20626     
- Misses      53068    73694   +20626     

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

Copy link
Contributor

@wileyj wileyj 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! matches what i found for next

@wileyj
Copy link
Contributor

wileyj commented Feb 1, 2024

Error when running cargo check

➜  stacks-core git:(fix/c-deps) cargo check
    Checking wsts v7.0.0
error: expected identifier, found `32`
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:307:39
    |
307 |             33 => Some(self.message[::32]),
    |                                       ^^ expected identifier

error: functions are not allowed in struct definitions
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:304:5
    |
288 |   pub struct SignatureShareRequest {
    |              --------------------- while parsing this struct
...
304 | /     fn hash_bytes(&self) -> Result<Hash, Error> {
305 | |         match self.message.len() {
306 | |             32 => Some(self.message),
307 | |             33 => Some(self.message[::32]),
308 | |             _ => None,
309 | |         }
310 | |     }
    | |_____^
    |
    = help: unlike in C++, Java, and C#, functions are declared in `impl` blocks
    = help: see https://doc.rust-lang.org/book/ch05-03-method-syntax.html for more information

error: expected identifier
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:304:5
    |
304 |     fn hash_bytes(&self) -> Result<Hash, Error> {
    |     ^^

error[E0277]: the trait bound `SignatureShareRequest: Serialize` is not satisfied
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:55:17
    |
55  | #[derive(Clone, Serialize, Deserialize, Debug, PartialEq)]
    |                 ^^^^^^^^^ the trait `Serialize` is not implemented for `SignatureShareRequest`
...
75  |     SignatureShareRequest(SignatureShareRequest),
    |                           --------------------- required by a bound introduced by this call
    |
    = help: the following other types implement trait `Serialize`:
              bool
              char
              isize
              i8
              i16
              i32
              i64
              i128
            and 158 others
note: required by a bound in `serialize_newtype_variant`
   --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.156/src/ser/mod.rs:942:12
    |
934 |     fn serialize_newtype_variant<T: ?Sized>(
    |        ------------------------- required by a bound in this associated function
...
942 |         T: Serialize;
    |            ^^^^^^^^^ required by this bound in `Serializer::serialize_newtype_variant`

error[E0277]: the trait bound `SignatureShareRequest: Deserialize<'_>` is not satisfied
    --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/wsts-7.0.0/src/net.rs:75:27
     |
75   |     SignatureShareRequest(SignatureShareRequest),
     |                           ^^^^^^^^^^^^^^^^^^^^^ the trait `Deserialize<'_>` is not implemented for `SignatureShareRequest`
     |
     = help: the following other types implement trait `Deserialize<'de>`:
               bool
               char
               isize
               i8
               i16
               i32
               i64
               i128
             and 170 others
note: required by a bound in `newtype_variant`
    --> /Users/hankstoever/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde-1.0.156/src/de/mod.rs:2122:12
     |
2120 |     fn newtype_variant<T>(self) -> Result<T, Self::Error>
     |        --------------- required by a bound in this associated function
2121 |     where
2122 |         T: Deserialize<'de>,
     |            ^^^^^^^^^^^^^^^^ required by this bound in `VariantAccess::newtype_variant`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `wsts` (lib) due to 5 previous errors

i think this might be localized issue - running on a glibc-x64 system, cargo check returns successully. can you try it on different arch ?

Copy link
Member

@jcnelson jcnelson left a comment

Choose a reason for hiding this comment

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

You need to keep wsts and p256k1 using bindgen if it's supported by the target OS, but disable it if not. bindgen is used to create Rust FFI bindings for the host's libc, which can differ from OS to OS.

@zone117x
Copy link
Member Author

zone117x commented Feb 2, 2024

@jcnelson that suggests that the wsts and/or p256k1 packages have a very unfriendly C binding implementation. The per-target matrix config required just for those packages is unreasonable. The original secp256k1 and the sqlite deps don't require this. What's special about those packages?

Copy link
Contributor

@hstove hstove left a comment

Choose a reason for hiding this comment

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

I fixed the error I mentioned in this PR, it was in fact a local issue :)

@zone117x
Copy link
Member Author

zone117x commented Feb 2, 2024

@jcnelson I'm not going to be able to get the change you requested working in this PR (doesn't seem reasonable IMO). If that's a blocker then we should close this PR.

@wileyj wileyj self-requested a review February 2, 2024 19:43
@wileyj
Copy link
Contributor

wileyj commented Feb 2, 2024

removing my review for now so this isn't inadvertently merged until we can resolve @jcnelson comment

@hstove
Copy link
Contributor

hstove commented Feb 2, 2024

Also tested that the published Docker images have a working stacks-signer binary:

docker run -it --platform linux/amd64 blockstack/stacks-core:pr-4331 stacks-signer --help

👍🏼

@zone117x
Copy link
Member Author

zone117x commented Feb 4, 2024

@jcnelson it looks like the binaries work without your suggested change. Are you sure this isn't okay as-is?

@wileyj wileyj requested a review from xoloki February 5, 2024 16:06
@wileyj
Copy link
Contributor

wileyj commented Feb 5, 2024

@jcnelson it looks like the binaries work without your suggested change. Are you sure this isn't okay as-is?

@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.

LGTM

Copy link
Contributor

@wileyj wileyj left a comment

Choose a reason for hiding this comment

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

shipit

@wileyj wileyj merged commit efbef05 into next Feb 9, 2024
1 of 2 checks passed
@blockstack-devops
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@stacks-network stacks-network locked as resolved and limited conversation to collaborators Nov 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants