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

Improve SBT caching during code generation #1499

Merged
merged 13 commits into from
May 18, 2024

Conversation

majk-p
Copy link
Contributor

@majk-p majk-p commented Apr 22, 2024

Closes #1495.

@kubukoz's comment:

  • adds a smithy4s/ prefix to generated paths under src_managed/main/scala to avoid clashing with other codegens (in the default config for the generated code output)
  • changes the hashing of paths for the output directory to be string-based (os.Path), while inputs are still hashed based on contents (PathRef)

PR Checklist (not all items are relevant to all PRs)

  • Added unit-tests (for runtime code)
  • Added bootstrapped code + smoke tests (when the rendering logic is modified)
  • Added build-plugins integration tests (when reflection loading is required at codegen-time)
  • Added alloy compliance tests (when simpleRestJson protocol behaviour is expanded/updated)
  • Updated dynamic module to match generated-code behaviour
  • Added documentation
  • Updated changelog

@CLAassistant
Copy link

CLAassistant commented Apr 22, 2024

CLA assistant check
All committers have signed the CLA.

@majk-p majk-p force-pushed the codegen-cache-improvements branch from e732967 to 70ebc75 Compare April 22, 2024 15:00
@majk-p majk-p force-pushed the codegen-cache-improvements branch from 45f8628 to 4fcd2dd Compare April 23, 2024 16:46
@Baccata
Copy link
Contributor

Baccata commented Apr 24, 2024

Don't forget to sign the CLA

@lefou
Copy link

lefou commented Apr 24, 2024

Since it was requested in Mill's chat room, here is some input regarding Mill's cache. I have to admit, I didn't review the full PR and all comments, but from what I digested, here are my notes:

  • If you return a PathRef that points to somewhere outside of Mill's cache, there is a chance Mill will miss it's later invalidation due to removal or change. Therefore in Mill 0.11 we introduced the revalidate flag which you can enable with PathRef.withRevalidateOnce. When enabled and before re-using a targets cache, Mill will re-calculate the PathRef.sig and will only use the cached value if the sig is still identical. I think you should use this flag since you don't have exclusive control over the output directory.

  • Using an T.input or a PathRef in general for a path you intend to change from a downstream target, will most likely cause unnecessary re-evaluation. Since you intend to generate something into that path, it's sig will change. If you're not interested in the content of a path, which is the case for config values, the path itself should be enough.

@Baccata Baccata changed the title Improve cache in code generation Improve SBT caching duringin code generation May 17, 2024
@Baccata Baccata changed the title Improve SBT caching duringin code generation Improve SBT caching during code generation May 17, 2024
@kubukoz
Copy link
Member

kubukoz commented May 17, 2024

LGTM (@Baccata, you haven't committed here so I'll leave it for you to approve if, it looks OK now)

@kubukoz kubukoz merged commit 1b40612 into disneystreaming:series/0.18 May 18, 2024
11 checks passed
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.

Default value of smithy4sOutputDir can break codegen cache
5 participants