emacs: replace fetchFromSavannah with mirror; adjust build#436020
emacs: replace fetchFromSavannah with mirror; adjust build#436020philiptaron merged 1 commit intomasterfrom unknown repository
fetchFromSavannah with mirror; adjust build#436020Conversation
ghost
mentioned this pull request
nixpkgs-ci
bot
added
10.rebuild-linux: 11-100
nix-owners
bot
requested review from
AndersonTorres,
adisbladis,
jian-lin,
jwiegley,
lovek323,
matthewbauer,
panchoh and
ttuegel
ghost
force-pushed
the
emacs-use-mirror
branch
from
8aaba84 to
029d3ec
Compare
What does this mean? That the FOD hash is incorrect and only held up by the cache? 😱 |
|
For Emacs I am a bit divided on this issue. Since less code is better code, I am fine with this. |
jian-lin
left a comment
jian-lin
left a comment
There was a problem hiding this comment.
snapshot functionality from Git forges like cgit or github is basically a pre-compiled compressed tar ball
I do not think a snapshot from cgit has been pre-compiled. Compared to the tarball from the mirror, there is no .elc files in the tarball.
Even if we choose to not use fetchFromSavannah which uses the snapshot functionality of cgit, we can still get vanilla source code using fetchgit.
As for determinism issues, apparently the unchanged savannah package still flags a determinism problem (building with --check/--rebuild).
I also don't understand what this sentence means. Please elaborate.
It comes from the
I mentioned this because mirrors can introduce determinism issues like the pre-compiled lisp files that have to be removed. I don't fully understand this and this could be an issue only on my end so I can drop that as part of the reasoning given for this change. One benefit of using a mirror is that the fetched source is smaller in size and that users can rely on mirrors rather than having to connect to savannah itself. |
|
OK, so the determinism issue you mention is about the build result of
I think
Isn't the difference is from compression? Please note that pre-compiled edit: The size diff between release tarballs and fetchFromSavannah is from added files and maybe also different compression methods. |
Yeah that can't be dropped and was taken straight from Guix, otherwise, the build would skip them as per the PR you mentioned. This snippet should have been the one used instead of using
The difference is from compression and is smaller than using |
Many people prefer not using release tarballs due to the XZ incident. I am not aware of the policy about this in Nixpkgs though. |
|
I'm not aware of any such policy in nixpkgs as a whole but I still believe you've brought up a valid concern, however, quoting #409110 (review) mirrors have their own advantages over just fetching from git. I don't think planning for XZ style attacks would be fruitful since I believe the main crux of XZ is that upstream was already compromised with a malicious actor who took control rather than being just about the delivery mechanism. A similar attack could just be done by compromising Savannah's cgit instance or the old fashioned merging bad commits. Emacs (hopefully) isn't in the state XZ was in where this could happen undetected. |
As far as I remember, a long time ago a change in some internal subroutine of Git modified the checksums of resulting artifacts, causing a noticeable hullabaloo: This is not the same as "injecting a backdoor after two years building trust", but it still qualifies as "another thing affecting the source provenance". Nonetheless, as I said before, this will not affect the current state of things. |
ghost
force-pushed
the
emacs-use-mirror
branch
from
029d3ec to
abdaad0
Compare
jian-lin
left a comment
jian-lin
left a comment
There was a problem hiding this comment.
LGTM
I will try to find some time to test it later.
ghost
force-pushed
the
emacs-use-mirror
branch
from
abdaad0 to
a019344
Compare
|
nixpkgs-ci
bot
added
12.approvals: 1
nixpkgs-ci
bot
added
12.approvals: 2
|
This hit Can we please coordinate these changes a bit better? This is far from the first time I've had to pin nixpkgs because of a minor build improvement to the emacs derivation. |
|
Quoting from @emilazy's excellent essay on the matter:
I think the coordination here of being broken and being fixed is acceptable. Nixpkgs needs to be able to evolve. The mitigation you have available (pin the previous version until you make the appropriate fix in the overlay) is a great tradeoff that enables these sorts of changes. |
Thanks for the reminder and sorry for the inconvenience. I have kept and will keep emacs-overlay in mind when changing Emacs in Nixpkgs. I did notice that this would break emacs-overlay after the merge. But I do not have enough time recently for a fix. @normalcea Could you PR a fix in Nixpkgs? The issue of this PR is that it should not touch I agree with emilazy's quote in general but I do not think that quote applies here because active maintainers of emacs-overlay (adisbladis and myself) are maintainers of Emacs in Nixpkgs. We know both parts. We want to keep emacs-overlay free of breakage due to changes of Emacs in Nixpkgs (though I fail sometimes 😅). BTW, I do find the "quick" merge a bit surprising. I said that I wanted to (have another look and) do some tests when I have more time in my preliminary review. Then before I found time to do so, this PR was merged. To be clear, I do not think this surprise is others' fault. I think it is my fault of not using GitHub's "Request changes" review type to express my intention more clearly. (So I posted a "Request changes" review in another similar PR right after the merge of this PR.) |
|
I'm happy to take the feedback. Thanks for the context. I do look for ❌ as a strong signal both of "wait" and for why that waiting is encouraged. I would push back on a norm of waiting more than a few days absent an ❌, and my merge came after I saw two maintainers say "LGTM" and waiting some more after that. 🙂 |
|
I made a fix in #437637 |
I think this is reasonable when downstream consumers are unknown. I don't think it's something we should encourage when the downstream consumer is a widely-used |
|
I'll definitely pay closer attention to non-package parts of the interface like With regard to things being in Is there any way to note down in the derivation itself or close by -- so that a reviewer on the PR would see -- that this parameter is overridden there? That seems like a cheap way of making it easier to not break these projects. Being human, I know that "just know about it" never works. |
|
I want to highlight that emacs-overlay is not just a random popular downstream project. It is created by one Nixpkgs Emacs maintainer (adisbladis), maintained by some Nixpkgs Emacs maintainers (adisbladis and myself) and used by many Nix Emacs users. It is an important piece of Nix Emacs. Since there are same maintainers of both parts, I think we can, should and will do better in coordinating Emacs changes.
It is not that cheap. emacs-overlay tightly couples with Emacs ecosystem in Nixpkgs, including not only The good news is that CI will request Emacs team for review for almost every Emacs-related PR. |
|
That is unfortunate and not my intention with any of these changes. I did note that downstreams could be negatively affected by this change but didn't realize the extent in which emacs-overlay was entwined with nixpkgs (which is why I opted for not removing the savannah fetcher codepath and just placed it behind a warning but now I realize the Emacs derivation itself was an interface). The removal of edit: I also agree with the idea that nixpkgs downstream consumers shouldn't expect package API (as in the derivation) compatibility especially when tracking the unstable branch of nixpkgs. Users that want this promise should use the stable branches of nixpkgs. Downstream nix projects should ideally maintain a stable and unstable edition of their projects or make it clear which nixpkgs they track. |
To be precise, When So removing |
4 participants
I used Guix's own Emacs derivation builder as an example for removing the pre-built files. As for determinism issues, apparently the unchanged savannah package still flags a determinism problem (building with --check/--rebuild). If it's good enough for Guix then it is probably good enough for nixpkgs.
See: #435964
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.