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

Refactor the codegen in seperate crates #197

Closed
wants to merge 9 commits into from
Closed

Conversation

arlyon
Copy link
Owner

@arlyon arlyon commented Mar 25, 2022

This PR aims to:

  • reduce compile times through parallel compilation units
  • improve the ergonomics of enabling / disabling features by enforcing crate dependencies

To do this we need to solve a few things. Rust does not allow circular crate dependencies and as it stands the generated code refers to the client directly, so we move the main apis into a 'core' crate and have the generated ones depend on that. Then have a separate crate expose both.

  • sigma

Open questions:

  • how do we author ext apis?
  • how do we handle unresolved imports?
  • what happens if (when) two crates import one-another?

Closes #196

@codecov
Copy link

codecov bot commented Mar 25, 2022

Codecov Report

Merging #197 (ba72c20) into master (c1d2850) will increase coverage by 0.56%.
The diff coverage is 0.00%.

❗ Current head ba72c20 differs from pull request most recent head c6a7cdd. Consider uploading reports for the commit c6a7cdd to get more accurate results

@@            Coverage Diff            @@
##           master    #197      +/-   ##
=========================================
+ Coverage    5.45%   6.01%   +0.56%     
=========================================
  Files         150     126      -24     
  Lines       14309   11132    -3177     
=========================================
- Hits          780     670     -110     
+ Misses      13529   10462    -3067     
Impacted Files Coverage Δ
openapi/src/crate_generator.rs 0.00% <0.00%> (ø)
openapi/src/file_generator.rs 0.00% <0.00%> (ø)
openapi/src/metadata.rs 0.00% <0.00%> (ø)
src/client/base/async_std.rs 88.63% <ø> (-2.48%) ⬇️
src/client/base/tokio.rs 93.79% <ø> (+0.32%) ⬆️
src/client/base/tokio_blocking.rs 66.66% <ø> (ø)
src/params.rs 40.40% <ø> (-28.27%) ⬇️
src/resources/balance_transaction_ext.rs 0.00% <ø> (ø)
src/resources/charge_ext.rs 0.00% <ø> (ø)
src/resources/customer_ext.rs 0.00% <ø> (ø)
... and 179 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@arlyon arlyon marked this pull request as draft March 25, 2022 11:36
@gautamg795
Copy link

hey @arlyon, just curious if you expect this to make it into 0.16.0 or what the current status is — as you mentioned in the description, the non-parallel compilation is pretty killer and is currently doubling compilation times on a project :/

@arlyon
Copy link
Owner Author

arlyon commented Sep 2, 2022

The answer is no eta. I am hacking on this when I have time but it is a fairly significant change (potentially breaking) so I want to also fix a few other major issues at the same time. I am reaching close to a 'preview' build and will update here when I finish it.

I estimate another 20-40 hours of work.

@arlyon
Copy link
Owner Author

arlyon commented Sep 11, 2022

image

Some more progress has been made here. Working on pulling out cyclic dependencies into common crates.

Cycles are as follows:

  • core - checkout - core
  • core - connect - core
  • core - fraud - core
  • core - issuing - core
  • billing - connect - core - checkout - billing

To turn this into a DAG we must break core deps on any other package

@seanpianka
Copy link
Contributor

Hello 👋 In an effort to move this along...

Would it be fair to say the remaining work here starts with removing/refactoring/rewriting the parts of core that import from checkout/connect/fraud/issuing? It sounds like this would transitively fix the issues with billing's dependency chain. After however long that takes, from there it just needs to be rebased from the latest master.

I'd really like to have this change available since compiling crates that use async-stripe heavily end up with long lto_optimize and codegen phases, it adds a lot of time to CI builds. I'll do what I can to help push this along!

@mzeitlin11 mzeitlin11 mentioned this pull request Nov 26, 2023
@arlyon
Copy link
Owner Author

arlyon commented Jan 4, 2024

closing this since it has stalled

@arlyon arlyon closed this Jan 4, 2024
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.

Split out codegen into crates
3 participants