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

rfcs: add move_itm_crate #589

Merged
merged 5 commits into from
Dec 14, 2021
Merged

rfcs: add move_itm_crate #589

merged 5 commits into from
Dec 14, 2021

Conversation

tmplt
Copy link
Contributor

@tmplt tmplt commented Dec 1, 2021

As discussed in the Matrix chat (from 2021-11-31--2021-12-01) here is an RFC proposing the disown/move of the itm crate and repo for continued development outside of the WG.

Rendered RFC

@tmplt tmplt requested a review from a team as a code owner December 1, 2021 14:21
@jonathanpallant
Copy link
Contributor

Thank you for the RFC. I'd be happy move ahead with this, but let's hear from the other @rust-embedded/tools members.

Emilgardis
Emilgardis previously approved these changes Dec 1, 2021
Copy link
Member

@Emilgardis Emilgardis left a comment

Choose a reason for hiding this comment

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

LGTM!

@Emilgardis Emilgardis requested a review from a team December 1, 2021 16:25
burrbull
burrbull previously approved these changes Dec 1, 2021
@jonathanpallant
Copy link
Contributor

@adamgreig
Copy link
Member

adamgreig commented Dec 1, 2021

The itm crate is part of the @rust-embedded/cortex-m team, not tools, i.e.

I'll review this once I'm home later this evening.

@jonathanpallant
Copy link
Contributor

Oops. That is unexpected ... but correct.

@tmplt
Copy link
Contributor Author

tmplt commented Dec 5, 2021

For sake of transparency: if and when this goes though the repository will be moved to https://github.com/rtic-scope/itm (updated repo already available; so only the crates.io registery entry is necessary at this point) because of its design towards RTIC Scope. I'll amend the RFC if required.

adamgreig
adamgreig previously approved these changes Dec 6, 2021
Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

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

I have a slight preference to not abandon itm and rather encourage its development by new contributors inside the WG, but a stronger preference that it gets developed at all, so on balance I'm in favour here. I look forward to welcoming the crate back one day!

@thalesfragoso, any thoughts?

@thalesfragoso
Copy link
Member

Sorry for taking too long, I've been a bit busy...

Anyways, isn't it better to just move ownership of the crate's name and archive rust-embedded/itm (with a link to rtic-scope/itm) ?

I would also like that a new published version of itm to be semver incompatible with the current one and to have a note in the README stating that an ownership transfer happened.

Thanks for the work @tmplt and sorry for being a drag...

@tmplt
Copy link
Contributor Author

tmplt commented Dec 8, 2021

would also like that a new published version of itm to be semver incompatible with the current one

A v0.4.0 has been tagged and is queued for publication. A note can be added to the README.md sure, but do we do anything with the previous releases? Do we pull them?

@adamgreig
Copy link
Member

Anyways, isn't it better to just move ownership of the crate's name and archive rust-embedded/itm (with a link to rtic-scope/itm) ?

I'd be fine with this approach too, I guess you're right and there might not be much point transferring this repo only to merge a PR that essentially replaces all the code. We could archive this one for reference in the future and just allow the new crate to be published as a semver-incompatible version on top. What do you think @tmplt?

do we do anything with the previous releases? Do we pull them?

I don't think there's any need to pull them. It won't do anything to current users anyway, and it's not like there's some security issue in the older versions.

@tmplt
Copy link
Contributor Author

tmplt commented Dec 9, 2021

We could archive this one for reference in the future and just allow the new crate to be published as a semver-incompatible version on top. What do you think?

Sounds good to me. But what is a semver-incompatible version? A release above 0.3.X?

@adamgreig
Copy link
Member

But what is a semver-incompatible version? A release above 0.3.X?

Yes, 0.4.0 would be fine.

@tmplt
Copy link
Contributor Author

tmplt commented Dec 12, 2021

Should the RFC be amended with the following:

  • archive rust-embedded/itm with a deprecation notice instead of moving it; and
  • only move the itm crates.io registery ownership?

@therealprof
Copy link
Contributor

Should the RFC be amended with the following:

  • archive rust-embedded/itm with a deprecation notice instead of moving it; and

Clarifying what disown means would be useful.

  • only move the itm crates.io registery ownership?

It's already there (twice), no? https://github.com/rust-embedded/wg/pull/589/files#diff-6b7ea2d7569e31f5da361fe6f319517f068bbc9f38b5b5ef69700b196caebaf2R9

Co-authored-by: Daniel Egger <[email protected]>
@tmplt tmplt dismissed stale reviews from adamgreig, burrbull, and Emilgardis via 130c1c2 December 14, 2021 13:30
@tmplt
Copy link
Contributor Author

tmplt commented Dec 14, 2021

RFC amended: it now proposes archiving rust-embedded/itm instead of disowning it, and giving me publisher access to itm on crates.io after which itm v0.4.0 will be released.

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

As approved by the team and discussed in the meeting today, this is good to go and will be put in effect immediately.

Thanks @tmplt.

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 14, 2021

Build succeeded:

@bors bors bot merged commit 2b6a0f8 into rust-embedded:master Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants