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

Create a generic AVR target: avr-unknown-unknown #131651

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Create a generic AVR target: avr-unknown-unknown #131651

wants to merge 1 commit into from
+136 −59

Conversation

Patryk27
Copy link
Contributor

This commit removes the avr-unknown-gnu-atmega328 target and replaces it with a more generic avr-unknown-unknown variant that can be specialized using -C target-cpu (e.g. -C target-cpu=atmega328p).

I've decided to go with the -unknown tag (i.e. avr-unknown-unknown instead of avr-unknown-gnu), because that's the name LLVM already uses - I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target - I've been already fixing the bugs anyway, might as well make it official 🙂

Related discussion: #131171.

@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2024

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 13, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 13, 2024

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

Some changes occurred in src/tools/compiletest

cc @jieyouxu

These commits modify compiler targets.
(See the Target Tier Policy.)

@Patryk27
Copy link
Contributor Author

cc @workingjubilee?

@Patryk27 Patryk27 mentioned this pull request Oct 13, 2024
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Patryk27
Copy link
Contributor Author

Ouch, I'm not sure why the test is failing - rustc --print=all-target-specs-json -Z unstable-options does return avr-unknown-unknown.

@workingjubilee
Copy link
Member

my first instinct is "bootstrap problem?"

@workingjubilee workingjubilee added the O-AVR Target: AVR processors (ATtiny, ATmega, etc.) label Oct 13, 2024
@jieyouxu
Copy link
Member

I'll take a look

@jieyouxu jieyouxu added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 13, 2024
jieyouxu
jieyouxu approved these changes Oct 13, 2024
View reviewed changes
Copy link
Member

@jieyouxu jieyouxu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The test and bootstrap side of things LGTM modulo the compiletest header test thing. Didn't look at the compiler side.

@Patryk27 Patryk27 mentioned this pull request Oct 14, 2024
Open
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@jfrimmel jfrimmel mentioned this pull request Oct 16, 2024
Merged
@rustbot rustbot added the A-compiletest Area: The compiletest test runner label Oct 19, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@tgross35
Copy link
Contributor

tgross35 commented Oct 19, 2024 •
edited
Loading

Just checking since I don't see it linked, did this get an MCP? That is usually the approval step for changing targets.

(if not, issue template here https://github.com/rust-lang/compiler-team/issues)

@Patryk27
Copy link
Contributor Author

Patryk27 commented Oct 19, 2024 •
edited
Loading

I haven't created MCP, wasn't sure on the process (especially considering AVR is a rather unpopular target).

If you think this needs a MCP, I can create one.

@tgross35
Copy link
Contributor

I think an MCP makes sense just to make everyone aware of the change, it's the usual process even for tier 3 targets. I don't see it actually being a problem, this makes sense to me.

Overall I think PR looks pretty good, left a few small comments. Thanks for the thorough build and test instructions, that is awesome to have.

@Patryk27
Copy link
Contributor Author

Okie, I'll create an MCP then 🙂

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@Patryk27
Create a generic AVR target: avr-unknown-unknown
f8643b8 
This commit removes the `avr-unknown-gnu-atmega328` target and replaces
it with a more generic `avr-unknown-unknown` variant that can be
specialized using `-C target-cpu` (e.g. `-C target-cpu=atmega328p`).

I've decided to go with the `-unknown` tag (i.e. `avr-unknown-unknown`
instead of `avr-unknown-gnu`), because that's the name LLVM already uses
- I see no reason to diverge here.

Seizing the day, I'm adding myself as the maintainer of this target -
I've been already fixing the bugs anyway, might as well make it
official.

Related discussion:
rust-lang#131171
@Patryk27 Patryk27 mentioned this pull request Oct 20, 2024
Open
@Patryk27
Copy link
Contributor Author

^ MCP created: rust-lang/compiler-team#800.

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

@tgross35 tgross35 tgross35 left review comments

@jieyouxu jieyouxu jieyouxu approved these changes

Assignees

@compiler-errors compiler-errors

Labels
A-compiletest Area: The compiletest test runner A-testsuite Area: The testsuite used to check the correctness of rustc O-AVR Target: AVR processors (ATtiny, ATmega, etc.) S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Patryk27 @rustbot @rust-log-analyzer @workingjubilee @jieyouxu @tgross35 @compiler-errors