-
Notifications
You must be signed in to change notification settings - Fork 234
Add more details about the changelog and require TC approval #7197
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
base: main
Are you sure you want to change the base?
Conversation
|
I do think this is a good thing to try and expose to users. However, I have two issues. First off, I think this is too much work for the release driver, with our current process. We need better tooling/process before requiring this. I also think this is underspecified, and it is underspecified for the same reasons as to why I think #7153 is a hard design problem, except here there is the additional issue of not overwhelming the regular changelog which is a published artifact for client consumption (so far #7153 has been focusing on TC needs). The actual full list of API changes (new methods, items, new trait impls) is quite large, typically, and not very useful, either to the user or for TC approval. Filtering by stable as proposed here helps but I am not convinced it is sufficient. For example, one design question is: "if I add a new type, should the API changelog also mention all of its trait impls?". If the answer is yes, we need to know to what detail we talk about it. Same goes for struct fields on a new struct, methods on a new type, etc. Generally, how this integrates into CHANGELOG.md is a tricky question, too, and one not yet covered by #7153. Some potential options:
I think this should be closed and made a part of #7153. My preferred process here is:
As it stands, were I required to do this as release driver, I would not know what I need to do because of the open questions, and I would also consider this to be too much work for the release driver to do. |
robertbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of the added phrasing. This checklist is an aid for the release driver to run the release, not a legal document restricting them.
|
This is also true. We should have processes that lead to a better release, we should not use the checklist as a way of foisting more work onto the release driver. |
The Unicode bylaws require that we follow certain processes. There aren't many requirements, but getting TC consensus on new APIs is one of them. So, yes, it is the responsibility of the release driver to follow certain "legal" procedures. |
|
I do not think that follows: our current process already does that via the changelog. People are able to ask clarifying questions if they wish. The changelog isn't the best way to get this information reviewed, but we collectively decided that the changelog approval would suffice when we had the new TC procedures show up. Are you saying that for the last few releases we have not been following the Unicode TC procedures? I doubt that. |
As I've noted before, this is required by the Unicode bylaws. This is only the second release we've made with the new bylaws (2.0 was the first, I believe). It's not a "nice to have when we have better tooling". It's something we need to figure out how to do even if the tooling isn't ready yet.
There is a well-specified definition in Unicode® Technical Group Procedures. ICU has a TC meeting every week and reviews changes as they come in. ICU4X has a WG meeting every week, and we bucket the Technical Decisions into a single changelog document that the TC approves all at once. It means that the changelog document is not only to inform users; it is also to satisfy a "legal" requirement we have as a Unicode Technical Committee.
I claim it is necessary for TC approval. If it is not useful for users, then it can be put in a supplementary doc, which my text in this PR suggests.
I still claim that these aren't underspecified design questions. All of the above are "Public (ie, non-internal) API signatures and semantics in production code" which means they are Technical Decisions that need to be documented. |
As Chair of the ICU4X Technical Committee, I want the list of Technical Decisions to be crystal clear. 1.5 predated the new Unicode procedures, I think. 2.0 had so many Technical Decisions that it was a ball of wax that we approved. We did do individual crate-level API reviews. I think we could have done better here. 2.1 had relatively few Technical Decisions relative to the 2.0 release. I saw it as an opportunity to do the right thing to comply with the Unicode procedures. My requests to have an API changelog for TC approval were underspecified because they were an edict coming from leadership, not a thoroughly designed deliverable coming from a delegate. As TC chair, I reviewed the changelog and felt it was qualitatively sufficient for recording Technical Decisions. But, I want to improve this process for next time. In other words, this text I'm writing is the "constitution" that says what needs to happen, but doesn't say specifically how to implement it. #7153 is one possible implementation of the constitution. |
My claim is that our current process satisfies the bylaws. We take care to ensure all public API changes are mentioned in the changelog as PR links. The bylaws say nothing about the precise format of TC approval. I don't think this is great, but I don't think it is productive to appeal to authority here and say that we must do this because the bylaws say we must. It's also a distraction: it's not like we don't want to have this. We do, the disagreement is just on whether we must. It would be nice for this to be more convenient for TC members performing approval.
Absolutely! But I do not want to diminish its usefulness to users in service of internal requirements. If we are doing this, we should do this separately. My strong preference is that we first design what we want to see in an API changelog (and please let us discuss that on #7153, I filed an issue for this purpose and it has yet to receive any engagement). We can focus on TC needs there. Once we have that design, we can consider mixing it in with the changelog. I would love to eventually not have two documents. But it will be easier to figure out what is most useful for TC members first, and then see how we can reconcile that with user needs, instead of trying to juggle both simultaneously. I think there is a potential answer here (and I have ideas), but we need to work towards it.
Would you have been satisfied with me posting the multi-thousand line output of At the end of the day, the current changelog process is clearly not enough. The existing tooling is, in my experience, too noisy. We need a middle ground, and I don't know what that is. At the moment I do not know how to satisfy this release requirement. You can claim it is sufficiently specified all you want, the fact of the matter is that I do not know what to do with this, and if I were release driver again with this requirement I would not be able to perform this task because I do not understand the requirement, and I don't want to be forced to try and pick apart this requirement while working on a release. I've spent some effort investigating this already and seeing if there is a convenient way to generate a useful API changelog, and part of the issue was that I wasn't entirely certain on where the line is for various things. I have ideas. I plan to post those ideas to #7153. It's not yet been a priority for me to do so.
As a base rule I do not want the release checklist to be vague. If you want to write vague policy requirements that establish a policy goal, write a different document, or file an issue. I am very much in favor of this goal. I just don't want something this vague and underspecified in the release checklist. The release checklist is a document to aid the release driver in performing the release; if it is vague then that means the release driver has to spend time working on legislating this. If you want the next release driver to spend time ensuring this is legislated, add #7153 to the 2.2 milestone. I fully support that, I would like this to be prioritized, but I would like this to be prioritized similar to any other release blocker: with frequent reviews during milestones, where we can properly prioritize discussions, and properly judge the tradeoff if it turns out it is slipping. This should not be something that comes up in the ninth hour when someone is working on a release and suddenly realizes the release involves much more work. Footnotes |
|
FWIW, I think this discussion will be more productive once I have time to really sketch out the challenges I see in specifying this, and potential solutions. So far I mostly have been saying there are challenges and pointing to the output of various tools as examples, and you have been saying it is not underspecified, but we've not actually really discussed the different points. |
I added details about listing new public APIs in the changelog and requiring TC approval.