-
Notifications
You must be signed in to change notification settings - Fork 9
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
Repurpose builder to support crosscompilation. #690
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
==========================================
- Coverage 71.61% 71.33% -0.28%
==========================================
Files 281 287 +6
Lines 42500 42739 +239
==========================================
+ Hits 30437 30489 +52
- Misses 12063 12250 +187
|
BenchmarksComparisonBenchmark execution time: 2024-11-11 13:29:18 Comparing candidate commit c94e23f in PR branch Found 5 performance improvements and 6 performance regressions! Performance is the same for 40 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/ 3782-8224-6310-005
scenario:credit_card/is_card_number/ 378282246310005
scenario:credit_card/is_card_number/378282246310005
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:redis/obfuscate_redis_string
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
BaselineOmitted due to size. |
4fb2105
to
2439e7b
Compare
ebca9be
to
8b11d6a
Compare
795aa18
to
a560a21
Compare
8ac0bb3
to
c2e963d
Compare
Unrelated, but a nitpick that I noticed when playing around with the builder code. Any file not already added in the Here's the patch. diff --git .gitignore .gitignore
index a98646fb..37987d6c 100644
--- .gitignore
+++ .gitignore
@@ -7,7 +7,8 @@
.idea
.vs
.vscode
-build/
+**/build/*
+!/builder/build/*
cmake-build-debug
deliverables
target |
17687cd
to
6a68382
Compare
f18a645
to
c6c1d9b
Compare
23646d9
to
5830027
Compare
53df4ab
to
13ab2a8
Compare
037b718
to
71e67d0
Compare
71e67d0
to
ea4e7c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good foundational work 🎉 , hope we will remove the windows specific scripts soon.
Write-Host "Build finished" -ForegroundColor Magenta |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/question
I thought we are going to replace cargo build
from L25-28 with the new builder crate version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you mentioned that we are going to do it later in the description. However, we must add some CI step for building for Windows to keep the feedback loop short.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/question
I thought we are going to replace
cargo build
from L25-28 with the new builder crate version?
For now, on the gitlab builder, if we target x86, the build fails. so for now we keep this as it is and when the build is fix this will be update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you mentioned that we are going to do it later in the description. However, we must add some CI step for building for Windows to keep the feedback loop short.
Not sure to understand what do you mean by we must add some CI steps
. On each PR, this script is used to build on Windows (gitlab). So there are already CI steps.
let profile = env::var("PROFILE").unwrap(); | ||
let version = env::var("CARGO_PKG_VERSION").unwrap(); | ||
let host = env::var("TARGET").unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let profile = env::var("PROFILE").unwrap(); | |
let version = env::var("CARGO_PKG_VERSION").unwrap(); | |
let host = env::var("TARGET").unwrap(); | |
let profile = env::var("PROFILE").expect("PROFILE env var is not set"); | |
let version = env::var("CARGO_PKG_VERSION").expect("CARGO_PKG_VERSION env var is not set"); | |
let host = env::var("TARGET").expect("TARGET env var is not set"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not my area of expertise, but looks reasonable to me.
de5df3c
to
e24e3ed
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
e24e3ed
to
c94e23f
Compare
What does this PR do?
This PR adds support for crosscompilation in the builder crate. Relying solely on the cargo build script to carry all the work had the limitation that it would only work for the target in which the build script is executed. With this solution the build logic has been moved to a external binary which will be able to spawn different builds for the selected target.
Motivation
Windows CI compiles artifacts for x86 and x86_64 targets so the previous approach wasn't suitable for that environment.
Additional Notes
Despite the builder fully work with Linux and MacOS systems there is still some rough edges around the linking phase during the windows build, for that reason the windows script was not been fully ported to use the new method. This task will be tackled in another PR just to not overcomplicate this one.