-
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
Add build system infrastructure. #586
Conversation
70c4fe3
to
7ad37b5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #586 +/- ##
==========================================
- Coverage 72.93% 72.69% -0.25%
==========================================
Files 245 246 +1
Lines 34951 34940 -11
==========================================
- Hits 25490 25398 -92
- Misses 9461 9542 +81
|
BenchmarksComparisonBenchmark execution time: 2024-09-05 08:13:47 Comparing candidate commit 498e0ed in PR branch Found 7 performance improvements and 13 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics. scenario:credit_card/is_card_number/
scenario:credit_card/is_card_number/37828224631
scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/
scenario:credit_card/is_card_number_no_luhn/37828224631
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:normalization/normalize_name/normalize_name/Too-Long-.Too-Long-.Too-Long-.Too-Long-.Too-Long-.Too-Lo...
scenario:normalization/normalize_name/normalize_name/bad-name
scenario:tags/replace_trace_tags
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. |
bb811d9
to
05bd0a3
Compare
8057d2c
to
c8e242b
Compare
6a4bd51
to
628d9e2
Compare
628d9e2
to
ff8aaf3
Compare
c83cf96
to
e266993
Compare
* Set output folder via environment variable. * Add tar generation. * Make builder work both in release and debug profiles. * Add version handling. * Avoid deep cloning Strings by using Rc<str>.
* Modify Alpine build to use new build system. * Fix musl arch specialization. * Set LIBDD_OUTPUT_FOLDER in alpine build image.
* Fix report paths. * Add documentation. * Rerun build if any environment variable changes. * Use determine_paths to figure out source directory.
f2586e2
to
2c438dd
Compare
* Add call to wait on spawning commands to ensure all pending modifications to the filesystem have finished. * Fix header directory assembling the tar file. * Avoid telemetry header inclusion when telemetry feature is not selected. * Fix clippy suggestions. * Run clippy through all workspace so the builder crate is included. * Fix merge conflicts.
2c438dd
to
a7f88b9
Compare
Happy to merge this when you think we have reached parity with today's build. This is already a leap forward! Some of these can be addressed as follow ups. |
@r1viollet during my testing the binaries generated are on par with what we're building now with the scripts. Also the examples are running fine as well as the tests in all the platforms we're currently using. Originally I wanted to modify the gitlab pipeline to use the builder crate but there were some problems with the Windows build. I talked with Gregory about that and we decided that we will make the changes in another separate PR, that's why the build scripts are still in place. Therefore if you're okay with the latest changes I think we can move forward, merge this one and start working on the next iteration. |
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.
Even the benchmarks approve 😉. And after the merges they didn't 😢. Something is broken with the benchmarks and their apparent flakiness.
Ok, let's go! Please give a heads up to everyone when merging and we will look out for any potential issues. |
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
ed24073
to
498e0ed
Compare
What does this PR do?
Adds a new build system as described in RFC 0002.
How to test the change?
The builder crate has been excluded from the default members in the workspace so it's not built when 'build' command is issued with no specific package.