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

nextcloud{28,29}Packages: update; revamp generation script #326947

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

pyrox0
Copy link
Member

@pyrox0 pyrox0 commented Jul 13, 2024

Description of changes

This updates the nextcloudPackages sets to use SRI hashes instead of sha256 hashes, and regenerates the package set using a new generation script that just uses bash, jq, and curl, as the previous generation script's tool, nc4nix, doesn't work properly anymore. This should make updating the package set in the future better and more robust. I've double-checked that the new hashes work properly, so this should not need to be reverted in future(as happened in #326912)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Jul 13, 2024
@dotlambda
Copy link
Member

Is this better than using a more recent nc4nix and rewriting fetchNextcloudApp to use fetchurl?

@pyrox0
Copy link
Member Author

pyrox0 commented Jul 14, 2024

I think this makes more sense since it doesn't require any non-core packages, and guarantees compatibility by using nix as the hasher. To me this made more sense than rewriting the fetcher, and I think rewriting the fetcher would be more trouble than it would be worth here, as you'd need to unpack it anyways. Leaving it as fetchzip means you don't have to handle that manually.

@dotlambda dotlambda merged commit f033e5f into NixOS:master Jul 18, 2024
23 checks passed
@dotlambda dotlambda added the backport release-24.05 Backport PR automatically label Jul 18, 2024
Copy link
Contributor

Backport failed for release-24.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.05
git worktree add -d .worktree/backport-326947-to-release-24.05 origin/release-24.05
cd .worktree/backport-326947-to-release-24.05
git switch --create backport-326947-to-release-24.05
git cherry-pick -x 4349a61a715bb29e0c326076f3666a8c419130ec d5d501b799ab338b5262cbe3fa771a9dc49a512a fe3e80ad3883e9ddf9b3f471692b8f5f2c3033d2

# Get all of our variables
VERSION=$(jq -r ".[] | select(.id == \"${a}\") | .releases[0].version" "$APPS_PER_VERSION")
URL=$(jq -r ".[] | select(.id == \"${a}\") | .releases[0].download" "$APPS_PER_VERSION")
HASH=$(nix store prefetch-file --json --hash-type sha256 --unpack "$URL" | jq -r .hash)
Copy link
Member

Choose a reason for hiding this comment

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

This raises

[…]
++ jq -r .hash                                                                                                                                                                                    
++ nix store prefetch-file --json --hash-type sha256 --unpack https://github.com/nextcloud/bookmarks/releases/download/v14.2.2/bookmarks-14.2.2.tar.gz                                            
error: unrecognised flag '--unpack' 

when I try to run the generation script. How does this work? My nix version is nix (Nix) 2.18.4 .

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, --unpack is nix 2.20.0/lix-exclusive. I'll send a followup that uses nix-prefetch-url instead, which has a backwards-compatible --unpack option that nix 2.18 can use.

Copy link
Member

@Emantor Emantor Jul 21, 2024

Choose a reason for hiding this comment

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

FWIW I fixed this up locally by requiring nixVersions.latest in the generate.sh script.

Copy link
Member

Choose a reason for hiding this comment

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

I think we want to pin Nix to an explicit version here. Technically, you can still use Nix 2.3 to instantiate nixpkgs and requiring newer features is fine here, but it has to be made explicit.

@Ma27
Copy link
Member

Ma27 commented Jul 20, 2024

So, this change feels a bit off and I'm considering to propose a revert now.

Didn't the old update script ignore prereleases? I now get contacts 6.0 rc1 and a nightly release for maps.
Since this was never discussed here, I can assume that this is a regression?

That said, I don't think that a hacky bash script is the best option here. What is the current issue even?
Did someone even investigate what was wrong with nc4nix? There weren't any changes code-wise, so it might've been a force-pushed release?

Also, how do we want to proceed with stable backports? Given that Nextcloud discloses vulnerabilities a few weeks after the release, we should still backport Nextcloud. But now I have to do it manually because

  • all hashes changed and there are conflicts
  • we probably don't want nightly app releases on stable

cc @dotlambda @pyrox0

@pyrox0
Copy link
Member Author

pyrox0 commented Jul 20, 2024

Looking at how nextcloud filters out pre-releases and nightly versions. Shouldn't be too hard, and will push a PR with updated packages and script.

@dotlambda
Copy link
Member

Didn't the old update script ignore prereleases? I now get contacts 6.0 rc1 and a nightly release for maps. Since this was never discussed here, I can assume that this is a regression?

No, I've seen plenty of nightly versions with nc4nix in the past.

@Ma27
Copy link
Member

Ma27 commented Jul 21, 2024

So, I did a bit of debugging:

apps.json from 29:

$ jq < apps.json '.[]|select(.id=="contacts").releases|.[]|.version'
"6.0.0-rc.1"
"6.0.0-alpha.1"
"6.0.0"
"5.5.3"
"5.5.2"
"5.5.1"
"5.5.0-rc.2"
"5.5.0"

We'll always get the RC here even though there's a 6.0.0 out already.

I'm pretty sure that's the case that helsinki-systems/nc4nix#5 is fixing. I'm not sure though how nightly versions appeared here, perhaps a bug (cc @onny).


First and foremost, this is clearly a regression from this fetcher.
And I'll ask again: did anybody bother to investigate what was wrong with nc4nix? Have we ruled out that somebody did - against all best practices - force-push an app release?
I don't want to throw out a fetcher written in a real programming language over this.

@Pizmovc
Copy link
Contributor

Pizmovc commented Jul 30, 2024

I see two possibilities for moving this forward:

  1. We fix all issues with the current script (usage of non-stable releases, nix not being included in the nix-shell, ???)
  2. We revert this MR and use a newer version of nc4nix which should've fixed the issue which preceded this PR

I would prefer the second option, since it places less maintenance burden on us. What are your thoughts on this?

Copy link
Member

@dotlambda dotlambda left a comment

Choose a reason for hiding this comment

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

I now agree we should either revert this or improve it significantly.
The former is what I do in #331507.

# Ensure the app exists in the file
if [ "$(jq -r ".[] | select(.id == \"${a}\")" "$APPS_PER_VERSION")" != "" ]; then
# Get all of our variables
VERSION=$(jq -r ".[] | select(.id == \"${a}\") | .releases[0].version" "$APPS_PER_VERSION")
Copy link
Member

Choose a reason for hiding this comment

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

This just picks the first item in releases, which might not actually be the most recent version. See e.g. the calendar app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux backport release-24.05 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants