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

seaweedfs: add version check to passthru.tests #119636

Merged
merged 1 commit into from
Apr 28, 2021

Conversation

raboof
Copy link
Member

@raboof raboof commented Apr 16, 2021

Motivation for this change

This is basically the extent of testing I usually do when an upgrade
is proposed, so this takes that manual step away.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@SuperSandro2000
Copy link
Member

Would it be easy to test actually functionality?

@raboof
Copy link
Member Author

raboof commented Apr 17, 2021

Would it be easy to test actually functionality?

No ;)

Converting to draft because nix-community/nixpkgs-update#260 (comment) mentioned passthru.tests might be a better place for this than installCheckPhase, so I want to check that out.

@raboof raboof marked this pull request as draft April 17, 2021 08:07
@raboof raboof force-pushed the seaweedfs-add-install-check branch from ec0b947 to 278b553 Compare April 17, 2021 16:33
@raboof raboof changed the title seaweedfs: add version check in installCheckPhase seaweedfs: add version check to passthru.tests Apr 17, 2021
@raboof raboof marked this pull request as ready for review April 17, 2021 16:35
@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 and removed 10.rebuild-darwin: 1 10.rebuild-darwin: 1-10 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Apr 17, 2021
@raboof raboof force-pushed the seaweedfs-add-install-check branch 2 times, most recently from 2275b5d to de34b2f Compare April 18, 2021 07:51
@raboof
Copy link
Member Author

raboof commented Apr 18, 2021

Updated to use passthru.tests. The diff looks more scary than it is because the indentation changed.

@raboof
Copy link
Member Author

raboof commented Apr 18, 2021

Result of nixpkgs-review pr 119636 run on x86_64-linux 1

@raboof
Copy link
Member Author

raboof commented Apr 18, 2021

let's discuss whether this check belongs in installCheckPhase or in passthru.tests in #119731 (comment) first

@raboof raboof marked this pull request as draft April 18, 2021 09:58
@davidak
Copy link
Member

davidak commented Apr 24, 2021

It does belong in passthru.tests as it was discussed earlier (#73076). Today i finally documented it (#120534)!

I would like to have a general, reusable test for this specific case.

@raboof
Copy link
Member Author

raboof commented Apr 24, 2021

I would like to have a general, reusable test for this specific case.

Me, too - but I figured I'd first "test the waters" with a specific test before generalizing to a reusable one.

@raboof raboof force-pushed the seaweedfs-add-install-check branch from de34b2f to 88d8f72 Compare April 24, 2021 19:25
@raboof raboof marked this pull request as ready for review April 24, 2021 19:25
Copy link
Member

@davidak davidak left a comment

Choose a reason for hiding this comment

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

generally looks good

pkgs/applications/networking/seaweedfs/default.nix Outdated Show resolved Hide resolved
pkgs/applications/networking/seaweedfs/default.nix Outdated Show resolved Hide resolved
passthru.tests instead of installCheckPhase as recommended in
nix-community/nixpkgs-update#260 (comment)

Inspired by
https://github.com/NixOS/nixpkgs/blob/master/pkgs/applications/science/logic/key/default.nix#L54-L57

This is basically the extent of testing I usually do when an upgrade
is proposed, so this takes that manual step away.
@raboof raboof force-pushed the seaweedfs-add-install-check branch from 88d8f72 to 4fe5281 Compare April 25, 2021 07:45
@raboof raboof requested a review from davidak April 25, 2021 07:46
@davidak
Copy link
Member

davidak commented Apr 25, 2021

hmm, not sure if i'm doing something wrong or if nixpkgs-review has issues

[davidak@gaming:~/code/nixpkgs]$ nix run -f channel:nixos-unstable nixpkgs-review -c nixpkgs-review pr --post-result --token ***** 119636 -p seaweedfs.tests
[16.7 MiB DL]
$ git -c fetch.prune=false fetch --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0 pull/119636/head:refs/nixpkgs-review/1
remote: Enumerating objects: 1629, done.
remote: Counting objects: 100% (865/865), done.
remote: Compressing objects: 100% (18/18), done.
remote: Total 1629 (delta 853), reused 849 (delta 847), pack-reused 764
Receiving objects: 100% (1629/1629), 1.13 MiB | 2.68 MiB/s, done.
Resolving deltas: 100% (1157/1157), completed with 362 local objects.
From https://github.com/NixOS/nixpkgs
   5d41312d331..14f65f0dc59  master                -> refs/nixpkgs-review/0
 + 88535b695c3...4fe52810ad5 refs/pull/119636/head -> refs/nixpkgs-review/1  (forced update)
$ git worktree add /home/davidak/.cache/nixpkgs-review/pr-119636/nixpkgs 14f65f0dc595d132a4ec6f9e6cd604e04d0a2033
Preparing worktree (detached HEAD 14f65f0dc59)
Updating files: 100% (25651/25651), done.
HEAD is now at 14f65f0dc59 Merge pull request #116365 from omasanori/sage-9.3
$ nix-env --option system x86_64-linux -f /home/davidak/.cache/nixpkgs-review/pr-119636/nixpkgs -qaP --xml --out-path --show-trace
$ git merge --no-commit --no-ff 4fe52810ad52f6af84531727204d9f636f3cec46
Automatic merge went well; stopped before committing as requested
$ nix-env --option system x86_64-linux -f /home/davidak/.cache/nixpkgs-review/pr-119636/nixpkgs -qaP --xml --out-path --show-trace --meta
error: cannot coerce a set to a string

       at /nix/store/pazb6bdswz7g84pm801zx7pigbpcw9l7-nixpkgs-review-2.6.0/lib/python3.8/site-packages/nixpkgs_review/nix/evalAttrs.nix:12:35:

           11|     pkg = lib.attrByPath attrPath null pkgs;
           12|     maybePath = builtins.tryEval "${pkg}";
             |                                   ^
           13|   in rec {
nix --experimental-features nix-command --system x86_64-linux eval --json --impure --expr (import /nix/store/pazb6bdswz7g84pm801zx7pigbpcw9l7-nixpkgs-review-2.6.0/lib/python3.8/site-packages/nixpkgs_review/nix/evalAttrs.nix /tmp/tmpzg1rbnza) failed to run, /tmp/tmpzg1rbnza was stored inspection
https://github.com/NixOS/nixpkgs/pull/119636 failed to build
$ git worktree prune

@raboof did the test run for you?

@SuperSandro2000
Copy link
Member

@ofborg test seaweedfs

@raboof
Copy link
Member Author

raboof commented Apr 26, 2021

@raboof did the test run for you?

Yes, and nixpkgs-review happy here as well (installed system-wide rather than using flakes though)

@raboof
Copy link
Member Author

raboof commented Apr 26, 2021

Result of nixpkgs-review pr 119636 run on x86_64-linux 1

@davidak
Copy link
Member

davidak commented Apr 26, 2021

It seem to be a problem with nixpkgs-review.

I checked out the pr and build it with nix-build.

[davidak@gaming:~/code/nixpkgs]$ gh pr checkout 119636
From github.com:NixOS/nixpkgs
 * [new ref]                 refs/pull/119636/head -> seaweedfs-add-install-check
Switched to branch 'seaweedfs-add-install-check'

[davidak@gaming:~/code/nixpkgs]$ nix-build -A seaweedfs.tests
these derivations will be built:
  /nix/store/j559rivz4dr3km6k8cfaj69mfc3pvvxp-weed-version.drv
these paths will be fetched (11.69 MiB download, 56.34 MiB unpacked):
  /nix/store/q7ggrlf02ycdn23d235589l59pll699q-seaweedfs-2.36
copying path '/nix/store/q7ggrlf02ycdn23d235589l59pll699q-seaweedfs-2.36' from 'https://cache.nixos.org'...
building '/nix/store/j559rivz4dr3km6k8cfaj69mfc3pvvxp-weed-version.drv'...
version 30GB 2.36  linux amd64
/nix/store/g4i2iwx9ray8mgfs77m7m37nmvviz01r-weed-version

[davidak@gaming:~/code/nixpkgs]$ ll /nix/store/g4i2iwx9ray8mgfs77m7m37nmvviz01r-weed-version
-r--r--r-- 1 root root 0 Jan  1  1970 /nix/store/g4i2iwx9ray8mgfs77m7m37nmvviz01r-weed-version

The result is an empty file in the nix store. Perfect.

Copy link
Member

@davidak davidak 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

test works

@tomberek tomberek merged commit e424c91 into NixOS:master Apr 28, 2021
@raboof raboof mentioned this pull request May 6, 2021
10 tasks
raboof added a commit to raboof/nixpkgs that referenced this pull request May 7, 2021
Extract 'version test' to a reusable test utility as discussed in
NixOS#119636 (comment) and
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 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants