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

Custom-build: Document breaking change on built comps. #10311

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

alt-romes
Copy link
Collaborator

@alt-romes alt-romes commented Sep 3, 2024

As mentioned by e8c9d19, Cabal 3.12 introduced a breaking change where an unnecessary (not requested) component of a package with a "Custom" build-type is no longer built.

This caused breakage for Agda, and was undocumented.

The behaviour, despite breaking, is the desired one -- a package shouldn't assume the executable will be built if only the library was requested. This commit documents the breaking change to make sure it is more visible.

  • Patch Agda to no longer expect the executable to be built when only the library is installed.

Closes #9777 and #10235

Please read Github PR Conventions and then fill in one of these two templates.


Template Α: This PR modifies behaviour or interface

Include the following checklist in your PR:

  • Patches conform to the coding conventions.
  • Any changes that could be relevant to users have been recorded in the changelog.
  • The documentation has been updated, if necessary.
  • Manual QA notes have been included.
  • Tests have been added. (Ask for help if you don’t know how to write them! Ask for an exemption if tests are too complex for too little coverage!)

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.

Do we also want to make sure the manual is clear on this?

@alt-romes
Copy link
Collaborator Author

I've pushed the patch to Agda that updates its Setup script to account for this new behaviour in agda/agda#7471.

Copy link
Member

@fgaz fgaz left a comment

Choose a reason for hiding this comment

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

Looks good. I think we can skip the changelog.d entry and add this directly to the changelog.

@@ -0,0 +1,43 @@
synopsis: Installing a library with a Custom setup no longer requires building the executable
packages: Cabal
prs: #10311
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add #9650

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay. what does it mean to skip the changelog entry? do you want me to add this to the changelog right away?

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 thought the changelog was generated programatically from the entries

Copy link
Member

@fgaz fgaz Sep 4, 2024

Choose a reason for hiding this comment

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

what does it mean to skip the changelog entry? do you want me to add this to the changelog right away?

yes please. you can generate the markdown with https://codeberg.org/fgaz/changelog-d

I thought the changelog was generated programatically from the entries

It is, but we mostly do that to avoid merge conflicts, which shouldn't be a concern in this case. Also now there are only >=3.14 entries in changelog.d/, and we don't want to mix them up.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree with Francesco that at this point it's too late to go via changelog-files, and one of the release-notes documents should be adjusted retroactively. Not sure 3.12.0.0 or 3.12.0.1: need to double-check with the git history.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The Hackage changelogs just point to the changelog files on master.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Got it. This is ready then, please take a look again.

Copy link
Collaborator

@philderbeast philderbeast Sep 4, 2024

Choose a reason for hiding this comment

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

The Hackage changelogs just point to the changelog files on master.

With the changelog files compiled as release notes being markdown, they're easily publishable elsewhere too as I'm doing here with an update for Cabal's own website, (as an update to https://www.haskell.org/cabal/).

https://discourse.haskell.org/t/hakyll-is-awesome/9838
https://cabal.blockscope.com/releases/

Copy link
Member

@Mikolaj Mikolaj Sep 4, 2024

Choose a reason for hiding this comment

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

The ability to hotfix the release notes is soo convenient. But we are not overusing it and the notes/changelogs pertain to one release only (and we never modify any but the last one, I think), so snapshots of our release notes shall never get too outdated. Maybe we should state somewhere clearly that the only authoritative source of the changelog is the master branch in the repo. But linking to a file in a moving branch quite clearly indicates that, doesn't it?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think what might be more helpful is documentation that we do this and why. In particular, I personally found the indirection in the Hackage changelogs annoying at first, especially given that if you read on you find older actual changelogs. This would be a fixed preamble in the changelog files.

@ulysses4ever ulysses4ever added the merge me Tell Mergify Bot to merge label Sep 4, 2024
@mergify mergify bot added ready and waiting Mergify is waiting out the cooldown period merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days labels Sep 4, 2024
As mentioned by e8c9d19, Cabal 3.12 introduced a breaking change where
an unnecessary (not requested) component of a package with a "Custom"
build-type is no longer built.

This caused breakage for Agda, and was undocumented.

The behaviour, despite breaking, is the desired one -- a package
shouldn't assume the executable will be built if only the library was
requested. This commit documents the breaking change to make sure it is
more visible.

In the meantime, we've also patched Agda to no longer expect the
executable to be built when only the library is installed.

Closes haskell#9777 and haskell#10235
@mergify mergify bot merged commit db167ca into haskell:master Sep 6, 2024
49 checks passed
@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
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge me Tell Mergify Bot to merge ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change in behaviour in HEAD w.r.t copying and installation of executables in packages with Custom setups
6 participants