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

[Discussion] Should be changelog be a static, non-parameterized link? #336454

Open
AndersonTorres opened this issue Aug 22, 2024 · 23 comments
Open

Comments

@AndersonTorres
Copy link
Member

AndersonTorres commented Aug 22, 2024

Issue description

A short time ago @Pandapip1 noticed that changelogs with parameterized links are fragile: they can point out to incorrect URLs if e.g. they follow the Nixpkgs unstable schema.

@AndersonTorres AndersonTorres changed the title [Tracking issue] [Tracking issue] changelog should be a static, non-parameterized link Aug 22, 2024
@Pandapip1
Copy link
Contributor

It's not just that they're fragile. Linking to the changelog for a single version is less useful (and arguably less correct) than linking to the full changelog.

Additionally, if the changelog does not get overwritten with the version (e.g. using rec instead of finalAttrs), the changelog link is to a changelog entry that is not always indicative of the actual derivation's version.

@d-brasher
Copy link
Contributor

I'd also suggest to change the examples in the wiki (such as this) if there is consensus.

@emilazy
Copy link
Member

emilazy commented Aug 29, 2024

src.rev does not have the problems of version here.

It's not just that they're fragile. Linking to the changelog for a single version is less useful (and arguably less correct) than linking to the full changelog.

I think it’s more incorrect and misleading to show “future” changes that are not in the package version being looked up.

@Pandapip1
Copy link
Contributor

Pandapip1 commented Aug 29, 2024

I think it’s more incorrect and misleading to show “future” changes that are not in the package version being looked up.

I agree that it matters less for linking to changelog files in a repository using src.rev, since at least it's still a complete document (although what happens if src is completely overwritten, say, to an applyPatches?). I still disagree in general: A changelog should be just that, a log of changes. We shouldn't confuse users by intentionally linking to an intentionally out of date document.

I also see the meta parameter as referring to attributes of the project itself, and less about how it's been packaged (with the obvious exception of sourceProvenance). In that regard, the URL really ought not to change with the version.

@pbsds
Copy link
Member

pbsds commented Aug 29, 2024

src.rev also has problems

https://github.com/reflex-dev/reflex/releases/tag/v0.5.10

is not

https://github.com/reflex-dev/reflex/releases/tag/refs/tags/v0.5.10

but r-ryantm/nix-update is prone to make that change

@emilazy
Copy link
Member

emilazy commented Aug 29, 2024

In that case I don’t know what purpose the changelog serves in general. It seems clearly intended to allow users to check changes in packages to me, which is why it encodes the version. Otherwise it’s just a fairly random link that seems pointless for us to have. (I do think downloadPage is pointless by the same token, which is presumably why few packages set it.)

license, broken, maintainers, …, are also affected by packaging decisions, so I don’t see the argument of meta being about the project independent of packaging as particularly compelling.

FWIW, version‐specific changelogs are explicitly sanctioned by the manual:

A link or a list of links to the location of Changelog for a package. A link may use expansion to refer to the correct changelog version. Example: "https://git.savannah.gnu.org/cgit/hello.git/plain/NEWS?h=v${version}"

@emilazy
Copy link
Member

emilazy commented Aug 29, 2024

@pbsds Fair point, that’s bad. I’m thinking of instances where we point directly to a changelog file in a repository, though.

(Does anyone actually look at these changelog links? Are they surfaced in any prominent UI? Perhaps we should simply get rid of them.)

@Pandapip1
Copy link
Contributor

Perhaps we should simply get rid of them.

I would be okay with that, but that should be it's own separate issue.

@pbsds
Copy link
Member

pbsds commented Aug 29, 2024

The changelog links get added to commit messages and pr bodies when using nix-update. I usually check for breaking changes during backports and for critical services

@emilazy
Copy link
Member

emilazy commented Aug 29, 2024

Well, I can see them having some utility if they encode the version like many existing examples do, like I said, but not so much if they are just a random static link. So they’re not unrelated matters in my eyes.

Anyway I’m mostly just commenting so that people don’t prematurely assume there’s consensus about this and start bugging everyone to change it on their PRs, when even the manual’s example suggests doing it. Incorrect URLs should of course be fixed, though.

@Pandapip1
Copy link
Contributor

Okay, but can we agree for now that following cases should definitely not be allowed:

  • Using finalAttrs in changelog, at all.
  • Pointing to https://github.com/<owner>/<repo>/releases/tag/<tag> instead of https://github.com/<owner>/<repo>/releases (the former isn't a full changelog).

@emilazy
Copy link
Member

emilazy commented Aug 29, 2024

finalAttrs seems fine to me, and better than rec? Though realistically many things can break when overriding attributes and I consider it a bit of an “if you break, you get to keep both pieces” type of situation. But it seems like it should get strictly more cases correct to me.

I don’t really have an opinion about the releases URL. I think the main benefit – if there is one – is to be able to see what changed when a package updates. As a user I would probably get more benefit from the version‐specific release page. (But as I said, this whole bikeshed feels pretty pointless if we have no evidence that the changelogs are actually in any way useful to anyone. If we can figure out where – if anywhere – they are exposed, and how – if at all – people use them, then it might make sense to set policy based on what works best with those?)

This just feels like another vector for #336200 that isn’t worth slapping the “tracking issue” label on at this point. The unstable versions problem is unfortunate, but src.rev solves it for the case of in‐repository changelogs, and for GitHub releases it’s impossible to construct an accurate changelog link for unstable releases anyway (by definition, there will be changes not yet listed there).

@AndersonTorres AndersonTorres changed the title [Tracking issue] changelog should be a static, non-parameterized link [Discussion] Should be changelog be a static, non-parameterized link? Aug 29, 2024
@Atemu
Copy link
Member

Atemu commented Aug 29, 2024

Another point to consider is that we often jump more than one upstream release in one update of Nixpkgs. The only possible way to accurately represent that (and unstable versions for that matter) in meta.changelog would be to provide a link to a VCS log between the versions and that's just an unreasonable amount of effort for very little gain.

I don't think we should spend any significant time attempting to show this info to users. A link to the general upstream changelog is plenty as the user can reasonably be expected to find the relevant changes themselves on that page and even that only exists for convenience.

In any case, do not go around asking people to change to one or the other. This is not what PR authors should be spending any amount of energy on. (And I don't think we should spend significant time discussing this either.)

@emilazy
Copy link
Member

emilazy commented Aug 29, 2024

To give an example from a package I maintain, https://github.com/jsummers/imageworsener/blob/1.3.5/changelog.txt is a list of changes up to the version we package, so as long as you remember what version you were on previously it should list all the relevant changes. It’s true that GitHub releases make this hard‐to‐impossible to represent and that it’s somewhat hopeless for unstable versions, but I think the changelog field was invented precisely for these kinds of in‐repository changelogs.

Of course “so long as you remember what version you were on previously” is doing a lot of heavy lifting here… and if you’re that engaged with a package you can probably already find the appropriate changelog yourself. Hence why I’m really tempted to say that this whole field (same as downloadPage) is just historical baggage that is pure busywork for maintainers that nobody benefits from and we should drop it. Maybe if it was templated on a version range, and we had tooling that could diff the versions between system upgrades and present changelogs scoped to the versions being upgraded between, it could have some common utility for users. But I strongly suspect we’ve already spent more time on this issue than users have spent evaluating the meta.changelog field in the past while. If anyone can present any common place where these URLs are actually surfaced to users I might change my mind about removal.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented Aug 29, 2024

How can it be?
From a "less fragile link" we are now at "drop it entirely"!

Since removing things makes Nixpkgs smaller and arguably no functionality is lost, I am for deleting it - or making it deprecated and discouraged, given backwards-compatibility.

(plus a treewide removal of them)

@pbsds
Copy link
Member

pbsds commented Aug 29, 2024

I'm not in favor of dropping the urls, nor prescribing a lint/some review process to try and prepare for link breakage. If we don't care about downstream users who override the expression (and likely don't care about meta anyway), a simple CI check that validates that the url is valid should suffice. If you bump to unstable its on you to figure out the urls

@pbsds
Copy link
Member

pbsds commented Aug 30, 2024

possibly of interest: #338301

@eclairevoyant
Copy link
Contributor

eclairevoyant commented Aug 30, 2024

If anyone can present any common place where these URLs are actually surfaced to users I might change my mind about removal.

r-ryantm comments the URL, if changelog is set, see e.g. #338318

and since r-ryantm is the most prolific source of PRs by a huge factor, at nearly double the number of commits of the most prolific human contributor, I'd consider it a common usecase

I personally find it helpful for review, to identify if there's some potential runtime breakage (that can't be caught at buildtime), as well as to know whether a version bump can be backported

@emilazy
Copy link
Member

emilazy commented Aug 30, 2024

That’s a good point! And having the changelog pinned to the relevant version is especially helpful in that case.

I do wish we got full diff links too, though…

@AndersonTorres
Copy link
Member Author

I do wish we got full diff links too, though…

However this is too dependent on the "forge".
E. G. live555 upstream provides only a tarball, and only for the most recent version.

@emilazy
Copy link
Member

emilazy commented Aug 30, 2024

That can be solved by computing diffs from src derivations and putting them up in a Gist or something. Anyway nix-update already includes diff links, so there’s precedent.

@Pandapip1 Pandapip1 mentioned this issue Aug 31, 2024
13 tasks
@SigmaSquadron
Copy link
Contributor

My 0,0294056 AUD: I'm maintaining a multiple-version package that uses upstream Wiki pages that detail the changes between each version:

changelog = "https://wiki.xenproject.org/wiki/Xen_Project_${branch}_Release_Notes";

Branch 4.19 will show the changes between 4.18 and 4.19, branch 4.18 will show the changes between 4.17 and 4.18, and so on. I imagined this would be more useful for people who are pinning their versions, as they know exactly what's new in that specific release.

An alternative to this would be to use https://xenbits.xenproject.org/gitweb/?p=xen.git;a=blob_plain;f=CHANGELOG.md;hb=HEAD, but it's a bit ugly and also shows unreleased changes.

@pluiedev
Copy link
Contributor

IMHO this problem manifests differently depending on where exactly the changelog is sourced from. If it's a CHANGELOG.md/rst/txt/whatever that keeps a list of all previous changes, then I think it's perfectly reasonable to pin the version as it would not have any unreleased/unpackaged changes that would confuse any end user. But if it's just a link to a tag, then I can see the argument that it would be less useful — however, maybe then it's just not worth it to even have a changelog in that case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants