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

pyfa: 2.60.2 -> 2.61.0 #356622

Merged
merged 2 commits into from
Dec 12, 2024
Merged

Conversation

paschoal
Copy link
Contributor

@paschoal paschoal commented Nov 17, 2024

switch from appImage to pythonApplication, update from 2.60.2 to 2.61.0
Pyfa Release 2.61.0

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label Nov 17, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Nov 17, 2024
@paschoal paschoal force-pushed the update-pyfa-2.61.0 branch 3 times, most recently from c17ad67 to e2439b0 Compare November 17, 2024 02:52
@paschoal paschoal mentioned this pull request Nov 17, 2024
13 tasks
@paschoal paschoal marked this pull request as ready for review November 17, 2024 03:05
Copy link
Member

@ToasterUwU ToasterUwU left a comment

Choose a reason for hiding this comment

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

Im getting the following error when trying to build it. It also eats like a solid 12GB of RAM, not sure if that's expected.

error: evaluation aborted with the following error message: 'lib.customisation.callPackageWith: Function called without required argument "wrapGAppsHook" at /home/aki/.cache/nixpkgs-review/pr-356622-2/nixpkgs/pkgs/by-name/py/pyfa/package.nix:7, did you mean "wrapGAppsHook3" or "wrapGAppsHook4"?'

@paschoal
Copy link
Contributor Author

Im getting the following error when trying to build it. It also eats like a solid 12GB of RAM, not sure if that's expected.

error: evaluation aborted with the following error message: 'lib.customisation.callPackageWith: Function called without required argument "wrapGAppsHook" at /home/aki/.cache/nixpkgs-review/pr-356622-2/nixpkgs/pkgs/by-name/py/pyfa/package.nix:7, did you mean "wrapGAppsHook3" or "wrapGAppsHook4"?'

i'm not sure if I understand why in my local env I was able to build with wrapGAppsHook only, but will take a look at this later.

I just switched to version 4, but I wonder if would be better to validate the local version available and decide which to use - what is your take?.

in regards to memory, I'm all ear on how to optimize this, the pyinstaller method is taken from pyfa wiki

@paschoal
Copy link
Contributor Author

I just switched to version 4, but I wonder if would be better to validate the local version available and decide which to use - what is your take?.

made the wrong choice I think, pyfa is gtk3 -- switched to it.

@ToasterUwU
Copy link
Member

I'm unsure how other Python based packages handle this, so I guess that's something to look into.
Theoretically there is no need to turn this into a proper executable, it would also be possible to just run it via python through a wrapper script that takes the place of the binary.
But I'm unsure what's the norm and best practice on Nixpkgs.

I'm going to bed now, if I find time tommorow, I will look into it.

@paschoal
Copy link
Contributor Author

I'm unsure how other Python based packages handle this, so I guess that's something to look into. Theoretically there is no need to turn this into a proper executable, it would also be possible to just run it via python through a wrapper script that takes the place of the binary. But I'm unsure what's the norm and best practice on Nixpkgs.

I'm going to bed now, if I find time tommorow, I will look into it.

yeap, while I do see the benefit of compiling it and making sure that all the libraries are properly set, let me see if we can just wrap it and run.

@paschoal
Copy link
Contributor Author

let me see if we can just wrap it and run.

to no avail, getting all sorts of error and missing dependencies, and can't just install it directly, since it is an externally managed environment.

let me know if you have any luck.

meanwhile did you manage to run it locally using pyinstaller version? the wrapGAppsHook was solved?

@ToasterUwU
Copy link
Member

i just got around to test building this, and got an error.

Btw, the reason your GApps thing didnt work is because you arent using the unstable branch. At least im pretty sure you are making and testing this on stable NixOS, but when this gets merged and build, it does so on unstable, so you should use that for making PRs here.

error:
       … while querying the derivation named 'pyfa-2.61.0'

       … while evaluating the attribute 'out.outPath'
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/customisation.nix:352:13:
          351|             drvPath = assert condition; drv.${outputName}.drvPath;
          352|             outPath = assert condition; drv.${outputName}.outPath;
             |             ^
          353|           } //

       … while calling the 'getAttr' builtin
         at <nix/derivation-internal.nix>:44:19:
           43|       value = commonAttrs // {
           44|         outPath = builtins.getAttr outputName strict;
             |                   ^
           45|         drvPath = strict.drvPath;

       … while calling the 'derivationStrict' builtin
         at <nix/derivation-internal.nix>:34:12:
           33|
           34|   strict = derivationStrict drvAttrs;
             |            ^
           35|

       … while evaluating derivation 'pyfa-2.61.0'
         whose name attribute is located at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:336:7

       … while evaluating attribute 'propagatedBuildInputs' of derivation 'pyfa-2.61.0'
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:390:7:
          389|       depsHostHostPropagated      = elemAt (elemAt propagatedDependencies 1) 0;
          390|       propagatedBuildInputs       = elemAt (elemAt propagatedDependencies 1) 1;
             |       ^
          391|       depsTargetTargetPropagated  = elemAt (elemAt propagatedDependencies 2) 0;

       … while calling anonymous lambda
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:326:13:
          325|       (map (drv: getDev drv.__spliced.hostHost or drv) (checkDependencyList "depsHostHostPropagated" depsHostHostPropagated))
          326|       (map (drv: getDev drv.__spliced.hostTarget or drv) (checkDependencyList "propagatedBuildInputs" propagatedBuildInputs))
             |             ^
          327|     ]

       … from call site
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:326:18:
          325|       (map (drv: getDev drv.__spliced.hostHost or drv) (checkDependencyList "depsHostHostPropagated" depsHostHostPropagated))
          326|       (map (drv: getDev drv.__spliced.hostTarget or drv) (checkDependencyList "propagatedBuildInputs" propagatedBuildInputs))
             |                  ^
          327|     ]

       … while calling 'getOutput'
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/attrsets.nix:1796:23:
         1795|   */
         1796|   getOutput = output: pkg:
             |                       ^
         1797|     if ! pkg ? outputSpecified || ! pkg.outputSpecified

       … while evaluating a branch condition
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/attrsets.nix:1797:5:
         1796|   getOutput = output: pkg:
         1797|     if ! pkg ? outputSpecified || ! pkg.outputSpecified
             |     ^
         1798|       then pkg.${output} or pkg.out or pkg

       … in the left operand of the OR (||) operator
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/attrsets.nix:1797:32:
         1796|   getOutput = output: pkg:
         1797|     if ! pkg ? outputSpecified || ! pkg.outputSpecified
             |                                ^
         1798|       then pkg.${output} or pkg.out or pkg

       … in the argument of the not operator
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/attrsets.nix:1797:10:
         1796|   getOutput = output: pkg:
         1797|     if ! pkg ? outputSpecified || ! pkg.outputSpecified
             |          ^
         1798|       then pkg.${output} or pkg.out or pkg

       … from call site
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:326:25:
          325|       (map (drv: getDev drv.__spliced.hostHost or drv) (checkDependencyList "depsHostHostPropagated" depsHostHostPropagated))
          326|       (map (drv: getDev drv.__spliced.hostTarget or drv) (checkDependencyList "propagatedBuildInputs" propagatedBuildInputs))
             |                         ^
          327|     ]

       … while calling anonymous lambda
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/lists.nix:334:29:
          333|   */
          334|   imap1 = f: list: genList (n: f (n + 1) (elemAt list n)) (length list);
             |                             ^
          335|

       … from call site
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/lists.nix:334:32:
          333|   */
          334|   imap1 = f: list: genList (n: f (n + 1) (elemAt list n)) (length list);
             |                                ^
          335|

       … while calling anonymous lambda
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:281:15:
          280|     imap1
          281|       (index: dep:
             |               ^
          282|         if isDerivation dep || dep == null || builtins.isString dep || builtins.isPath dep then dep

       … while evaluating a branch condition
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:282:9:
          281|       (index: dep:
          282|         if isDerivation dep || dep == null || builtins.isString dep || builtins.isPath dep then dep
             |         ^
          283|         else if isList dep then checkDependencyList' ([index] ++ positions) name dep

       … in the left operand of the OR (||) operator
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:282:69:
          281|       (index: dep:
          282|         if isDerivation dep || dep == null || builtins.isString dep || builtins.isPath dep then dep
             |                                                                     ^
          283|         else if isList dep then checkDependencyList' ([index] ++ positions) name dep

       … in the left operand of the OR (||) operator
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:282:44:
          281|       (index: dep:
          282|         if isDerivation dep || dep == null || builtins.isString dep || builtins.isPath dep then dep
             |                                            ^
          283|         else if isList dep then checkDependencyList' ([index] ++ positions) name dep

       … in the left operand of the OR (||) operator
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:282:29:
          281|       (index: dep:
          282|         if isDerivation dep || dep == null || builtins.isString dep || builtins.isPath dep then dep
             |                             ^
          283|         else if isList dep then checkDependencyList' ([index] ++ positions) name dep

       … from call site
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/stdenv/generic/make-derivation.nix:282:12:
          281|       (index: dep:
          282|         if isDerivation dep || dep == null || builtins.isString dep || builtins.isPath dep then dep
             |            ^
          283|         else if isList dep then checkDependencyList' ([index] ++ positions) name dep

       … while calling 'isDerivation'
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/attrsets.nix:1282:5:
         1281|   isDerivation =
         1282|     value: value.type or null == "derivation";
             |     ^
         1283|

       … while calling the 'elemAt' builtin
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/lib/lists.nix:334:43:
          333|   */
          334|   imap1 = f: list: genList (n: f (n + 1) (elemAt list n)) (length list);
             |                                           ^
          335|

       … while calling 'checkDrv'
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix:206:26:
          205|
          206|     checkDrv = attrName: drv:
             |                          ^
          207|       if (isPythonModule drv) && (isMismatchedPython drv) then throwMismatch attrName drv

       … while evaluating a branch condition
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix:207:7:
          206|     checkDrv = attrName: drv:
          207|       if (isPythonModule drv) && (isMismatchedPython drv) then throwMismatch attrName drv
             |       ^
          208|       else drv;

       … in the left operand of the AND (&&) operator
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix:207:31:
          206|     checkDrv = attrName: drv:
          207|       if (isPythonModule drv) && (isMismatchedPython drv) then throwMismatch attrName drv
             |                               ^
          208|       else drv;

       … from call site
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix:207:11:
          206|     checkDrv = attrName: drv:
          207|       if (isPythonModule drv) && (isMismatchedPython drv) then throwMismatch attrName drv
             |           ^
          208|       else drv;

       … while calling 'isPythonModule'
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix:42:20:
           41|
           42|   isPythonModule = drv:
             |                    ^
           43|     # all pythonModules have the pythonModule attribute

       … in the left operand of the AND (&&) operator
         at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/development/interpreters/python/mk-python-derivation.nix:46:5:
           45|     # Some pythonModules are turned in to a pythonApplication by setting the field to false
           46|     && (!isBool drv.pythonModule);
             |     ^
           47|

       error: undefined variable 'dateutil'
       at /home/aki/.cache/nixpkgs-review/pr-356622/nixpkgs/pkgs/by-name/py/pyfa/package.nix:30:5:
           29|     matplotlib
           30|     dateutil
             |     ^

@paschoal
Copy link
Contributor Author

paschoal commented Nov 21, 2024

i just got around to test building this, and got an error.

Btw, the reason your GApps thing didnt work is because you arent using the unstable branch. At least im pretty sure you are making and testing this on stable NixOS, but when this gets merged and build, it does so on unstable, so you should use that for making PRs here.

Oh, so while my system is running 100% unstable, the branch that I trigger the build process on nixos fork locally was stable, ha! did not realized that.

probably also related to this other error you have, gonna switch the branch and test it out later.

so putting this one together, the update-pyfa-2.61.0 should not be merged to master, but to unstable? (and since I branch out from master to write this one, also gonna rebase it against nixos-unstable)

btw I do appreciate the the help/directions on it! thanks.

@paschoal
Copy link
Contributor Author

paschoal commented Nov 21, 2024

oddly, rebased the branch against nixos-unstable and built it again without errors, I can't reproduce the errors you got locally.

2024-11-21_12-03

got any other clue of what might be going on?

btw, I do have the feeling that I'm building it wrong locally and that is why works here but not for you, I'm using $ nix-build -A pyfa, should I add any other param or use any other command to build it?

@paschoal
Copy link
Contributor Author

manually did a check on the subpackages that was included from python3Packages (python312Packages) and the dateutil was indeed incorrect, adjusted to python-dateutil.

so that error you had of undefined variable 'dateutil' should be gone, but the question on how we make this reproducible behaviours is still open.

@paschoal
Copy link
Contributor Author

and then, just did the nixpkgs-review step (hopefully that is the answer to my previous question about making it reproducible?)

maintainers/maintainer-list.nix Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/py/pyfa/package.nix Outdated Show resolved Hide resolved
@ToasterUwU
Copy link
Member

oddly, rebased the branch against nixos-unstable and built it again without errors, I can't reproduce the errors you got locally.

I'm pretty sure you are supposed to rebase this just against master, that's what it gets merged into when this PR is accepted.

PS: Hi Leah c: fancy seeing you here

@paschoal
Copy link
Contributor Author

[:/data/git/nixpkgs] update-pyfa-2.61.0 ± nix-shell -p nixfmt-rfc-style --run "nixfmt --check 'pkgs/by-name/py/pyfa/package.nix'"
[:/data/git/nixpkgs] update-pyfa-2.61.0 ± nix-shell -p nixfmt-classic --run "nixfmt --check 'pkgs/by-name/py/pyfa/package.nix'"
pkgs/by-name/py/pyfa/package.nix: not formatted

😕

@gepbird
Copy link
Contributor

gepbird commented Nov 22, 2024

[:/data/git/nixpkgs] update-pyfa-2.61.0 ± nix-shell -p nixfmt-rfc-style --run "nixfmt --check 'pkgs/by-name/py/pyfa/package.nix'"
[:/data/git/nixpkgs] update-pyfa-2.61.0 ± nix-shell -p nixfmt-classic --run "nixfmt --check 'pkgs/by-name/py/pyfa/package.nix'"
pkgs/by-name/py/pyfa/package.nix: not formatted

😕

You can format it with nixfmt-rfc-style (omit the --check) and commit the changes: nix-shell -p nixfmt-rfc-style --run "nixfmt 'pkgs/by-name/py/pyfa/package.nix'".

Edit: if it still doesn't work, maybe your nixfmt-rfc-style is outdated, you can run a pretty recent version with nix run github:NixOS/nixpkgs/nixos-unstable#nixfmt-rfc-style --extra-experimental-features nix-command --extra-experimental-features flakes -- 'pkgs/by-name/py/pyfa/package.nix'.

Nit: can you configure your editor to use 2 space indentation in nixpkgs? There are tabs in configurePhase and likely that's why you didn't spot the formatting issue.

@paschoal
Copy link
Contributor Author

Nit: can you configure your editor to use 2 space indentation in nixpkgs? There are tabs in configurePhase and likely that's why you didn't spot the formatting issue.

interesting, the code before the review was nixfmt-rfc-style complaint, and my local configuration do use 2 space width; but the last change commited directly from the suggestions 9eba09 was the one.

my fault was not rebasing to the very last commit that do introduce the changes, I was under the impression that cc4d769 broke it, but it was 9eba09. Rebasing with the github changes do identify the issue using --check.

@gepbird
Copy link
Contributor

gepbird commented Nov 22, 2024

Oh I'm just seeing that the editorconfig check is failing, and nixfmt doesn't fix that. You fixed some whitespace issues but there are 2 tabs which it doesn't like. To convert tabs to 2 spaces you can run sed -i 's/\t/ /g' pkgs/by-name/py/pyfa/package.nix and then you should run the nixfmt command as before. I'm happy to help with configuring this in your editor to not have these issues in the future, I just don't know what you're using :)

@gepbird gepbird added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people labels Nov 23, 2024
@ofborg ofborg bot requested review from ToasterUwU and Daholli November 24, 2024 01:35
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2132

@pbsbot
Copy link

pbsbot commented Nov 29, 2024

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 356622


x86_64-linux

✅ 1 package built:
  • pyfa

@paschoal
Copy link
Contributor Author

paschoal commented Dec 1, 2024

rebase against master and push to solve conflict @ maintainers/maintainers-list.nix

@wegank wegank removed 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Dec 1, 2024
@ofborg ofborg bot requested review from Daholli and ToasterUwU December 1, 2024 19:25
@wegank wegank added 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Dec 1, 2024
@Daholli Daholli mentioned this pull request Dec 8, 2024
@wolfgangwalther wolfgangwalther merged commit 2cf6241 into NixOS:master Dec 12, 2024
36 of 37 checks passed
@paschoal
Copy link
Contributor Author

@wolfgangwalther thank you! much appreciated

@paschoal paschoal deleted the update-pyfa-2.61.0 branch December 13, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 3+ This PR was reviewed and approved by three or more reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.