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

python3Packages.matplotlib: 3.3.4 -> 3.4.1 #119008

Merged
merged 4 commits into from
Apr 21, 2021

Conversation

veprbl
Copy link
Member

@veprbl veprbl commented Apr 10, 2021

Motivation for this change
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.

@veprbl

This comment has been minimized.

@ofborg ofborg bot requested a review from lovek323 April 10, 2021 11:28
@veprbl veprbl force-pushed the pr/matplotlib_3_4_1 branch 2 times, most recently from 149412d to 4b2d828 Compare April 10, 2021 12:03
@veprbl
Copy link
Member Author

veprbl commented Apr 10, 2021

@GrahamcOfBorg build python3Packages.matplotlib python3Packages.matplotlib.tests
@GrahamcOfBorg build python2Packages.matplotlib

Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

Those ? null only hide missing packages as they can't actually be ever null.

pkgs/development/python-modules/matplotlib/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/matplotlib/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/matplotlib/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/matplotlib/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/matplotlib/default.nix Outdated Show resolved Hide resolved
@veprbl veprbl changed the title python3Packages.matplotlib: 3.3.4 -> 3.4.1, enable testing python3Packages.matplotlib: 3.3.4 -> 3.4.1 Apr 11, 2021
@veprbl
Copy link
Member Author

veprbl commented Apr 11, 2021

@GrahamcOfBorg build python3Packages.matplotlib
@GrahamcOfBorg build python2Packages.matplotlib

@FRidh
Copy link
Member

FRidh commented Apr 11, 2021

Those ? null only hide missing packages as they can't actually be ever null.

Why can't they ever be null? If I call the expression without passing that parameter, it will be null. This needs to be undone, because now, calling the expression when you do not want tk, does require you to pass in tk or at least something for tk, even though it will be unused. That's why they have ? null.

These asserts are also not pointless. They perform an actual test. You may not like them (e.g. because of how they impact overriding) but they are not pointless.

@veprbl
Copy link
Member Author

veprbl commented Apr 11, 2021

Those ? null only hide missing packages as they can't actually be ever null.

Why can't they ever be null? If I call the expression without passing that parameter, it will be null. This needs to be undone, because now, calling the expression when you do not want tk, does require you to pass in tk or at least something for tk, even though it will be unused. That's why they have ? null.

No asserts are currently triggered in the default scope:

# nix-instantiate -I nixpkgs=channel:nixpkgs-unstable -E 'with import <nixpkgs> {}; python3Packages.matplotlib.override { enableGtk3 = true; enableTk = true; enableQt = true; }'
warning: you did not specify '--add-root'; the result might be removed by the garbage collector
/nix/store/c8cvz2ps9yxv2aifdfzqsygb3a7qvcaw-python3.8-matplotlib-3.3.4.drv

This would only cover a hypothetical situation where one tries to deliberately pass null values.

These asserts are also not pointless. They perform an actual test. You may not like them (e.g. because of how they impact overriding) but they are not pointless.

IMO they are not just pointless, but also harmful. Remember #53487

@veprbl
Copy link
Member Author

veprbl commented Apr 20, 2021

If there are no immediate concerns, let's merge. I don't mind if we revert the change that removes asserts.

@SuperSandro2000
Copy link
Member

SuperSandro2000 commented Apr 20, 2021

Why can't they ever be null?

If you are not deliberately passing in null for those inputs they are never triggered in nixpkgs and do not add any real value.

If are disabling tk like python3Packages.matplotlib.override { enableGtk3 = false; enableTk = false; enableQt = false; } it works perfectly fine in nixpkgs.

Those asserts are being removed since months from darwin frameworks and packages required by boolean flags and are not allowed for new packages because of the above mentioned issues. Disabling features by overriding packages with null is also not discoverable, often not tested in updates and overriding any random input to null most likely will fail packages in unforeseen ways. This also makes spotting problems harder when packages are being removed or are only required on specific platforms as they could be shadowed behind a ? null.

@SuperSandro2000
Copy link
Member

/rebase staging

@github-actions github-actions bot changed the base branch from master to staging April 20, 2021 19:47
@github-actions github-actions bot closed this Apr 20, 2021
@github-actions
Copy link
Contributor

Rebased, please reopen the pull request to restart CI

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

Successfully merging this pull request may close these issues.

3 participants