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

changelogs for Cabal-3.14 and (incomplete) cabal-install-3.16 #10323

Closed
wants to merge 4 commits into from

Conversation

ulysses4ever
Copy link
Collaborator

I added a WIP changelog for the tool which is easier, I think: we just remove all the tiny changelog files and at 3.16 will generate and append an update for this WIP (and it becomes the final release-notes/cabal-install-3.16...md).

Some doubts I have:


Template B: This PR does not modify behaviour or interface

E.g. the PR only touches documentation or tests, does refactorings, etc.

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • N/A [ ] Is this a PR that fixes CI? If so, it will need to be backported to older cabal release branches (ask maintainers for directions).

@fgaz
Copy link
Member

fgaz commented Sep 6, 2024

"Missing PRs" is the list of PRs that don't have a changelog entry. You should have a quick look at them to ensure we didn't forget entries for user-relevant ("Template A" now) PRs.

"Extra (unmerged) PRs" is the list of changelog entries with an invalid PR number (that is, the PR number wasn't found in the log). These are almost always typos and should be fixed.

@ulysses4ever
Copy link
Collaborator Author

Thanks for the explanation! I wish this info was put at a very visible place. Maybe in the messages the tool prints.

"Missing PRs" is the list of PRs that don't have a changelog entry. You should have a quick look at them to ensure we didn't forget entries for user-relevant ("Template A" now) PRs.

There are 174 unique entries on this list (I updated it so that it only shows unqie ones btw). This feels a little intimidating...

@geekosaur
Copy link
Collaborator

One complication is that you will get PRs listed as missing because they're for a different component than you're dumping changelogs for.

@ulysses4ever
Copy link
Collaborator Author

Yes! It looks like a significant shortcoming of changelog-d...

@ulysses4ever
Copy link
Collaborator Author

I submitted a PR against changelog-d which will fix the missing PR message to only report PRs missing from the whole set of changelog-d/ entries, not just the entries for the given package. https://codeberg.org/fgaz/changelog-d/pulls/17

@geekosaur
Copy link
Collaborator

geekosaur commented Sep 7, 2024

Some future changelog-d work that should reduce if not eliminate duplication: allow specifying multiple packages, and check that a given changelog entry matches any of them. That way, if an entry specifies both Cabal and Cabal-syntax, it will only be output once.

@ulysses4ever
Copy link
Collaborator Author

I plan to run my fork tonight to hopefully get a more manageable list of PRs to double-check for exclusion from the release notes.

@ulysses4ever
Copy link
Collaborator Author

I plan to run my fork tonight to hopefully get a more manageable list of PRs to double-check for exclusion from the release notes.

Okay, I tried it and it only struck about 20 PRs (bringing the number down to 154). As far as I see, the issue is that changelog-d is not smart enough to account for backports. For example, take #9578, which was backported and certainly appeared in 3.12: changelog-d still reports it as missing because that particular PR, of course, did not appear in 3.12, and the tool thinks that it should be in 3.14 release notes...

At this point, I consider the "Missing PR" feature of changelog-d dysfunctional and suggest we move forward and wait until users uncover any unaccounted changes. While undesirable, missing things in release notes is not the end of the world. For future releases, it'd be great to teach changelog-d about backports.

@ulysses4ever ulysses4ever requested review from Mikolaj and fgaz September 9, 2024 03:07
@ulysses4ever ulysses4ever added attention: needs-review merge me Tell Mergify Bot to merge labels Sep 9, 2024
@geekosaur
Copy link
Collaborator

Keep in mind that changelog-d (nor any other tool that doesn't use the GitHub API) doesn't have access to information such as whether it's a backport. I also wonder about trying to get features added to changelog-d that only we would use; I was careful about that with respect to my additions, but they weren't very intrusive to the other (two, I think?) users to begin with. Injecting a passive requirement to use Mergify for backports is probably over the line.

Copy link
Collaborator

@geekosaur geekosaur left a comment

Choose a reason for hiding this comment

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

Mostly formatting, but a few questions should get some consideration. I don't think they should hold it back, though.

release-notes/Cabal-3.14.0.0.md Outdated Show resolved Hide resolved
Haskell Foundation Tech Proposal
[Replacing the Cabal Custom build-type](https://github.com/haskellfoundation/tech-proposals/pull/60).

Package authors willing to use this feature should declare `build-type: Hooks`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also mention that you need Cabal-version: 3.14?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

release-notes/Cabal-3.14.0.0.md Outdated Show resolved Hide resolved
release-notes/Cabal-3.14.0.0.md Outdated Show resolved Hide resolved
release-notes/Cabal-3.14.0.0.md Outdated Show resolved Hide resolved
release-notes/cabal-install-3.16.0.0.md Outdated Show resolved Hide resolved
release-notes/cabal-install-3.16.0.0.md Outdated Show resolved Hide resolved
release-notes/cabal-install-3.16.0.0.md Outdated Show resolved Hide resolved
release-notes/cabal-install-3.16.0.0.md Outdated Show resolved Hide resolved
release-notes/cabal-install-3.16.0.0.md Show resolved Hide resolved
@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Sep 9, 2024
@ulysses4ever ulysses4ever removed merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period attention: needs-review labels Sep 9, 2024
ulysses4ever and others added 2 commits September 9, 2024 20:57
Co-authored-by: brandon s allbery kf8nh <allbery.b@gmail.com>
Copy link
Collaborator Author

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

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

Thanks a lot @geekosaur! I tried to accept all of your suggestions.

Haskell Foundation Tech Proposal
[Replacing the Cabal Custom build-type](https://github.com/haskellfoundation/tech-proposals/pull/60).

Package authors willing to use this feature should declare `build-type: Hooks`
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes.

See Note [Symbolic paths] in `Distribution.Utils.Path` for further information
on the design of this API.

- Add `MultilineStrings` extension [#10245](https://github.com/haskell/cabal/pull/10245)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I find GHC proposal number mildly useful potentially. But it can go into the title, I guess..

@ulysses4ever ulysses4ever added squash+merge me Tell Mergify Bot to squash-merge and removed squash+merge me Tell Mergify Bot to squash-merge labels Sep 10, 2024
@ulysses4ever
Copy link
Collaborator Author

I screwed up with the PR again: it should target 3.14, not master. (At least, I did branch out of 3.14...)

@ulysses4ever ulysses4ever mentioned this pull request Sep 10, 2024
2 tasks
Copy link
Contributor

mergify bot commented Sep 10, 2024

⚠️ The sha of the head commit of this PR conflicts with #10336. Mergify cannot evaluate rules on this PR. ⚠️

@Kleidukos Kleidukos mentioned this pull request Sep 12, 2024
20 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants