Skip to content
This repository was archived by the owner on Apr 20, 2026. It is now read-only.

fix: preserve dynamo cargo rustflags during source install - #213

Open
weireweire wants to merge 1 commit into
ishandhanani:mainfrom
weireweire:fix/dynamo-source-install-rustflags
Open

fix: preserve dynamo cargo rustflags during source install#213
weireweire wants to merge 1 commit into
ishandhanani:mainfrom
weireweire:fix/dynamo-source-install-rustflags

Conversation

@weireweire

@weireweire weireweire commented Mar 11, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • stop overriding Cargo rustflags via RUSTFLAGS during dynamo source installs
  • append -C target-cpu=native via maturin --config so repo-defined rustflags stay intact
  • preserve .cargo/config.toml settings such as tokio_unstable for top-of-tree builds

Verification

  • minimal Python import check of DynamoConfig command generation
  • pytest not available in this environment

Summary by CodeRabbit

  • Chores
    • Optimized Python package installation build configuration to enable native CPU optimizations during the build process.

@coderabbitai

coderabbitai Bot commented Mar 11, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

The pull request modifies the Python Dynamo bindings build process in src/srtctl/core/schema.py. Instead of exporting the RUSTFLAGS environment variable before invoking maturin, the rustflags are now embedded directly within maturin's configuration using the --config flag with native CPU target optimization.

Changes

Cohort / File(s) Summary
Build Configuration
src/srtctl/core/schema.py
Modified maturin build invocation to embed build.rustflags=["-C","target-cpu=native"] via --config flag instead of exporting RUSTFLAGS environment variable.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • PR #80: Directly conflicts with this change—PR #80 adds the RUSTFLAGS export before maturin, while this PR replaces that approach by embedding the flag in maturin's configuration instead.

Poem

🐰 A rustling flag finds its home so neat,
No env vars needed, the config's complete,
Maturin builds native, swift and true,
Embedded and clever—a better breakthrough!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly describes the main change: preserving Cargo rustflags during source installation by switching from RUSTFLAGS environment variable to maturin --config approach.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/srtctl/core/schema.py`:
- Line 730: The file fails CI formatting due to the string line containing
"maturin build --config 'build.rustflags=[\"-C\",\"target-cpu=native\"]' -o /tmp
&& "; fix by running the project formatter as suggested (run: `uv run ruff
format --write src/srtctl/`) or apply equivalent ruff/black formatting to
src/srtctl/core/schema.py so the offending string and surrounding lines are
reformatted to match the code style.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: d6545e58-2609-4d7f-9769-809205666c0d

📥 Commits

Reviewing files that changed from the base of the PR and between 9fff275 and 5cd2b52.

📒 Files selected for processing (1)
  • src/srtctl/core/schema.py

Comment thread src/srtctl/core/schema.py
"cd lib/bindings/python/ && "
'export RUSTFLAGS="${RUSTFLAGS:-} -C target-cpu=native" && '
"maturin build -o /tmp && "
"maturin build --config 'build.rustflags=[\"-C\",\"target-cpu=native\"]' -o /tmp && "

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Fix formatting to pass CI pipeline.

The pipeline reports that this file needs reformatting: "1 file would be reformatted: src/srtctl/core/schema.py."

Run uv run ruff format --write src/srtctl/ to fix the formatting issue.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/srtctl/core/schema.py` at line 730, The file fails CI formatting due to
the string line containing "maturin build --config
'build.rustflags=[\"-C\",\"target-cpu=native\"]' -o /tmp && "; fix by running
the project formatter as suggested (run: `uv run ruff format --write
src/srtctl/`) or apply equivalent ruff/black formatting to
src/srtctl/core/schema.py so the offending string and surrounding lines are
reformatted to match the code style.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant