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.

Sign up for GitHub

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

Jump to bottom

Split Rust arguments into a new scripts/Makefile.rust #1026

Closed
wants to merge 1 commit into from
Closed

Split Rust arguments into a new scripts/Makefile.rust #1026

wants to merge 1 commit into from

Conversation

tgross35
Copy link
Collaborator

This follows the discussion at #1025

@tgross35
rust: build: move Rust configuration to 'scripts/Makefile.rust'
2687b74 
Currently, all Rust flags live in the top level 'Makefile'. This patch
moves these flags to a new file at 'scripts/Makefile.rust' so that
wanted adjustments to Rust's CLI configuration can be applied without
touching the root 'Makefile'. This change is intended to reduce noise
and the potential for merge conflicts.

Making this change will simplify the goals of (1) adding additional
lints over time, and (2) changing from specifying lint groups (e.g.
'correctness', 'perf') to naming individual lints for better version
compatibility. These changes are space consuming (>100 lines) and may
have a few adjustments per cycle, so extracting these arguments out of
the shared 'Makefile' should be much less noisy for everyone.

Signed-off-by: Trevor Gross <[email protected]>
@tgross35 tgross35 force-pushed the split-rust-makefile branch from 15572e9 to 2687b74 Compare July 17, 2023 06:52
@tgross35
Copy link
Collaborator Author

I'm not sure how much splitting you were interested in @ojeda. This patch as-is currently just moves the flags, but I also experimented with moving almost everything Rust-related to this file (KBUILD_x and all the Rust-related targets), which could be cleaner overall.

(side note - I wonder if it would be possible to have a rustcheck target like cargo check that verifies but doesn't build anything)

@tgross35 tgross35 mentioned this pull request Jul 17, 2023
Open
@ojeda
Copy link
Member

ojeda commented Jul 17, 2023

I would only move the common flags to begin with and do so in the same patch series that adds the other lints/comments, because that is the motivation for this move, and keeps things simple. If we don't have a fair amount of comments/lints to add, it would be simpler to avoid the move altogether.

Unless we want to go for an actual refactor, that is. It is true that we could move other flags and other bits, and that was half my intention for using the name scripts/Makefile.rust (i.e. to allow for extra expansion/moves later). But then it falls more into Kbuild territory, and it would be done as a refactor rather than due to the lints.

@tgross35
Copy link
Collaborator Author

Understood, I was thinking that it would be preferred to keep this top-level change separate from the other Rust-specific changes - but I will merge them back together. Thanks for the look!

@tgross35 tgross35 closed this Jul 17, 2023
@ojeda
Copy link
Member

ojeda commented Jul 17, 2023

My pleasure! Thanks for working on this and taking a look at the different potential approaches, it is appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tgross35 @ojeda