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

feat(build): link openssl statically #2984

Merged
merged 5 commits into from
Jun 5, 2022
Merged

Conversation

skyzh
Copy link
Contributor

@skyzh skyzh commented Jun 4, 2022

Signed-off-by: Alex Chi iskyzh@gmail.com

What's changed and what's your intention?

Statically link openssl.

Need to test if works with s3.

Checklist

  • I have written necessary docs and comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Refer to a related PR or issue link (optional)

close #2973

@skyzh skyzh changed the title link openssl statically chore(build): link openssl statically Jun 4, 2022
@skyzh
Copy link
Contributor Author

skyzh commented Jun 5, 2022

Basically, enabling statically linked openssl is as easy as

openssl = { version = "0.10", features = ["vendored"] }

However, this change involves too many other changes, like enabling cert discovery by ourselves. So I'd close this and see if we really need statically link openssl in the future. Indeed, we can use rustls instead.

@skyzh skyzh closed this Jun 5, 2022
@skyzh skyzh reopened this Jun 5, 2022
@skyzh
Copy link
Contributor Author

skyzh commented Jun 5, 2022

~/risingwave$ ldd target/debug/risingwave
        linux-vdso.so.1 (0x00007fff97fae000)
        libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007fea8225c000)
        libgcc_s.so.1 => /lib/x86_64-linux-gnu/libgcc_s.so.1 (0x00007fea8223c000)
        libm.so.6 => /lib/x86_64-linux-gnu/libm.so.6 (0x00007fea82155000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fea81f2d000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fea8c8ef000)

@skyzh skyzh marked this pull request as ready for review June 5, 2022 03:56
@skyzh
Copy link
Contributor Author

skyzh commented Jun 5, 2022

Tested and works with s3. However, unit test OOM. We should have a flag to enable / disable statically linked openssl...

Signed-off-by: Alex Chi <iskyzh@gmail.com>
@skyzh skyzh force-pushed the skyzh/statically-link-openssl branch from eabeb84 to 685bb84 Compare June 5, 2022 04:50
@skyzh
Copy link
Contributor Author

skyzh commented Jun 5, 2022

There're still bugs with cargo-hakari that prevents setting the feature flag. Need to report to upstream 😇

@skyzh skyzh changed the title chore(build): link openssl statically feat(build): link openssl statically Jun 5, 2022
skyzh added 2 commits June 5, 2022 12:56
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Signed-off-by: Alex Chi <iskyzh@gmail.com>
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 821 files.

Valid Invalid Ignored Fixed
819 1 1 0
Click to see the invalid file list
  • src/utils/static-link/src/lib.rs

@@ -0,0 +1,8 @@
#[cfg(test)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[cfg(test)]
// Copyright 2022 Singularity Data
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
#[cfg(test)]

Signed-off-by: Alex Chi <iskyzh@gmail.com>
@skyzh skyzh enabled auto-merge (squash) June 5, 2022 05:28
@skyzh
Copy link
Contributor Author

skyzh commented Jun 5, 2022

Also a bug with rustc. It seems that compile RisingWave 1. with static-link 2. and then without it will cause linker to panic. Anyway, cargo clean always works.

@skyzh skyzh disabled auto-merge June 5, 2022 05:28
@skyzh skyzh enabled auto-merge (squash) June 5, 2022 05:28
Signed-off-by: Alex Chi <iskyzh@gmail.com>
@skyzh skyzh disabled auto-merge June 5, 2022 05:44
@codecov
Copy link

codecov bot commented Jun 5, 2022

Codecov Report

Merging #2984 (600cfb9) into main (2110778) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2984   +/-   ##
=======================================
  Coverage   72.97%   72.97%           
=======================================
  Files         720      721    +1     
  Lines       96471    96472    +1     
=======================================
+ Hits        70400    70401    +1     
  Misses      26071    26071           
Flag Coverage Δ
rust 72.97% <100.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/utils/static-link/src/lib.rs 100.00% <100.00%> (ø)

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@skyzh skyzh merged commit 2a3e88a into main Jun 5, 2022
@skyzh skyzh deleted the skyzh/statically-link-openssl branch June 5, 2022 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ci: statically link libssl
2 participants